[PATCH v2 5/5] builtin/config: introduce `color` type specifier

[Taylor Blau]

As of this commit, the canonical way to retreive an ANSI-compatible
color escape sequence from a configuration file is with the
`--get-color` action.

This is to allow Git to "fall back" on a default value for the color
should the given section not exist in the specified configuration(s).

With the addition of `--default`, this is no longer needed since:

  $ git config --default red --type=color core.section

will be have exactly as:

  $ git config --get-color core.section red

For consistency, let's introduce `--type=color` and encourage its use
with `--default` together over `--get-color` alone.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |  6 ++
 builtin/config.c | 22 ++
 t/t1300-repo-config.sh   | 30 ++
 3 files changed, 58 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index c3adafd78a..18ddc78f42 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -177,6 +177,10 @@ Valid ``'s include:
   ~/` from the command line to let your shell do the expansion.)
 - 'expiry-date': canonicalize by converting from a fixed or relative 
date-string
   to a timestamp. This specifier has no effect when setting the value.
+- 'color': When getting a value, canonicalize by converting to an ANSI color
+  escape sequence. When setting a value, a sanity-check is performed to ensure
+  that the given value is canonicalize-able as an ANSI color, but it is written
+  as-is.
 +
 
 --bool::
@@ -228,6 +232,8 @@ Valid ``'s include:
output it as the ANSI color escape sequence to the standard
output.  The optional `default` parameter is used instead, if
there is no color configured for `name`.
++
+`--type=color [--default=]` is preferred over `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 4f222a0ca9..bb62816bba 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT   3
 #define TYPE_PATH  4
 #define TYPE_EXPIRY_DATE   5
+#define TYPE_COLOR 6
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
@@ -94,6 +95,8 @@ static int option_parse_type(const struct option *opt, const 
char *arg,
new_type = TYPE_PATH;
else if (!strcmp(arg, "expiry-date"))
new_type = TYPE_EXPIRY_DATE;
+   else if (!strcmp(arg, "color"))
+   new_type = TYPE_COLOR;
else
die(_("unrecognized --type argument, %s"), arg);
}
@@ -229,6 +232,11 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (git_config_expiry_date(, key_, value_) < 0)
return -1;
strbuf_addf(buf, "%"PRItime, t);
+   } else if (type == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (git_config_color(v, key_, value_) < 0)
+   return -1;
+   strbuf_addstr(buf, v);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -374,6 +382,20 @@ static char *normalize_value(const char *key, const char 
*value)
else
return xstrdup(v ? "true" : "false");
}
+   if (type == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (git_config_color(v, key, value))
+   die("cannot parse color '%s'", value);
+
+   /*
+* The contents of `v` now contain an ANSI escape
+* sequence, not suitable for including within a
+* configuration file. Treat the above as a
+* "sanity-check", and return the given value, which we
+* know is representable as valid color code.
+*/
+   return xstrdup(value);
+   }
 
die("BUG: cannot normalize type %d", type);
 }
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 1e3a1ace14..2acfd513f1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
test_must_fail git config --expiry-date date.invalid1
 '
 
+test_expect_success 'get --type=color' '
+   rm .git/config &&
+   git config foo.color "red" &&
+   git config --get --type=color foo.color >actual.raw &&
+   test_decode_color actual &&
+   echo "" >expect &&
+   test_cmp expect actual
+'
+
+cat >expect << EOF
+[foo]
+   color = red
+EOF
+
+test_expect_success 'set --type=color' '
+   rm .git/config &&
+   git config --type=color foo.color "red" &&
+   test_cmp expect .git/config
+'
+

[PATCH v2 0/5] builtin/config.c: combined series '--type', '--default'

[Taylor Blau]

Hi,

Here is an amended version of my combined series to add '--type' and
'--default'. My apologies for the re-roll, I thought that I had looked
everything over closely enough :-).

Since last time:

  * Correct an obviously-wrong assignment into '*to_type' [1]. I have
moved both of these assignments into the top-line declaration of
those variables.

  * Re-add a removed hunk to support '--type=color' correctly [2].

Thanks,
Taylor

[1]: https://public-inbox.org/git/xmqq7eou35ev@gitster-ct.c.googlers.com
[2]: https://public-inbox.org/git/xmqq36zi352x@gitster-ct.c.googlers.com

Taylor Blau (5):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: support `--type=` as preferred alias for
`--`
  builtin/config: introduce `--default`
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `color` type specifier

 Documentation/git-config.txt |  81 
 builtin/config.c | 143 ---
 config.c |  10 +++
 config.h |   1 +
 t/t1300-repo-config.sh   |  93 +++
 t/t1310-config-default.sh|  36 +
 6 files changed, 305 insertions(+), 59 deletions(-)
 create mode 100755 t/t1310-config-default.sh

Inter-diff (since v1):

diff --git a/builtin/config.c b/builtin/config.c
index ec5c11293b..bb62816bba 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -72,19 +72,18 @@ static struct option builtin_config_options[];
 static int option_parse_type(const struct option *opt, const char *arg,
 int unset)
 {
-   int new_type;
-   int *to_type;
+   /*
+* To support '--' style flags, begin with new_type equal to
+* opt->defval.
+*/
+   int new_type = opt->defval;
+   int *to_type = opt->value;

if (unset) {
*((int *) opt->value) = 0;
return 0;
}

-   /*
-* To support '--' style flags, begin with new_type equal to
-* opt->defval.
-*/
-   new_type = opt->defval;
if (!new_type) {
if (!strcmp(arg, "bool"))
new_type = TYPE_BOOL;
@@ -96,11 +95,12 @@ static int option_parse_type(const struct option *opt, 
const char *arg,
new_type = TYPE_PATH;
else if (!strcmp(arg, "expiry-date"))
new_type = TYPE_EXPIRY_DATE;
+   else if (!strcmp(arg, "color"))
+   new_type = TYPE_COLOR;
else
die(_("unrecognized --type argument, %s"), arg);
}

-   *to_type = opt->value;
if (*to_type && *to_type != new_type) {
/*
 * Complain when there is a new type not equal to the old type.

--
2.17.0

[PATCH v2 2/5] builtin/config.c: support `--type=` as preferred alias for `--`

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values can be interpreted as that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid squatting on this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we support `--type=` in
addition to `--int`, `--bool`, and etc. This allows the aforementioned
upcoming patch to support querying a color value with a default via
`--type=color --default=...`, without squandering `--color`.

We retain the historic behavior of complaining when multiple,
legacy-style `--` flags are given, as well as extend this to
conflicting new-style `--type=` flags. `--int --type=int` (and its
commutative pair) does not complain, but `--bool --type=int` (and its
commutative pair) does.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 71 
 builtin/config.c | 64 +---
 t/t1300-repo-config.sh   | 58 +++--
 3 files changed, 153 insertions(+), 40 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d5..b759009c75 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
-'git config' [] [type] --add name value
-'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
-'git config' [] [type] [-z|--null] --get-urlmatch name URL
+'git config' [] [--type=] [--show-origin] [-z|--null] name 
[value [value_regex]]
+'git config' [] [--type=] --add name value
+'git config' [] [--type=] --replace-all name value 
[value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] --get 
name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
--get-all name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
[--name-only] --get-regexp name_regex [value_regex]
+'git config' [] [--type=] [-z|--null] --get-urlmatch name 
URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset. 
 If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given .  If no
+`--type=` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,39 @@ See also <>.
 --list::
List all variables set in config file, along with their values.
 
---bool::
-   'git config' will ensure that the output is "true" or "false"
+--type ::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in ``'s
+  canonical form.
++
+Valid ``'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value 

[PATCH v2 4/5] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--type=color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_`.

Signed-off-by: Taylor Blau 
---
 config.c | 10 ++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index 62c56099bf..d14813bef7 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const 
char *var, const char *
return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+   if (!value)
+   return config_error_nonbool(var);
+   if (color_parse(value, dest) < 0)
+   return -1;
+   return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac1..0e060779d9 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const 
char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.17.0

[PATCH v2 1/5] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau 
---
 builtin/config.c   | 49 +++---
 t/t1300-repo-config.sh | 11 ++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd628..92fb8d56b1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL  1
+#define TYPE_INT   2
+#define TYPE_BOOL_OR_INT   3
+#define TYPE_PATH  4
+#define TYPE_EXPIRY_DATE   5
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "get-color", , N_("find the color configured: slot 
[default]"), ACTION_GET_COLOR),
OPT_BIT(0, "get-colorbool", , N_("find the color setting: slot 
[stdout-is-tty]"), ACTION_GET_COLORBOOL),
OPT_GROUP(N_("Type")),
-   OPT_BIT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
-   OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT),
-   OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
-   OPT_BIT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
-   OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
+   OPT_SET_INT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
+   OPT_SET_INT(0, "int", , N_("value is decimal number"), TYPE_INT),
+   OPT_SET_INT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
+   OPT_SET_INT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
+   OPT_SET_INT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (show_keys)
strbuf_addch(buf, key_delim);
 
-   if (types == TYPE_INT)
+   if (type == TYPE_INT)
strbuf_addf(buf, "%"PRId64,
git_config_int64(key_, value_ ? value_ : 
""));
-   else if (types == TYPE_BOOL)
+   else if (type == TYPE_BOOL)
strbuf_addstr(buf, git_config_bool(key_, value_) ?
  "true" : "false");
-   else if (types == TYPE_BOOL_OR_INT) {
+   else if (type == TYPE_BOOL_OR_INT) {
int is_bool, v;
v = git_config_bool_or_int(key_, value_, _bool);
if (is_bool)
strbuf_addstr(buf, v ? "true" : "false");
else
strbuf_addf(buf, "%d", v);
-   } else if (types == TYPE_PATH) {
+   } else if (type == TYPE_PATH) {
const char *v;
if (git_config_pathname(, key_, value_) < 0)
return -1;
strbuf_addstr(buf, v);
free((char 

[PATCH v2 3/5] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the variable asked for does not exist.
In addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
variable and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--type=color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color variable [default] [...]

with:

  $ git config --default default --type=color variable [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--type=color`, which (in
conjunction with `--default`) will be sufficient to replace
`--get-color`.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |  4 
 builtin/config.c | 18 ++
 t/t1310-config-default.sh| 36 
 3 files changed, 58 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index b759009c75..c3adafd78a 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -240,6 +240,10 @@ Valid ``'s include:
using `--file`, `--global`, etc) and `on` when searching all
config files.
 
+--default ::
+  When using `--get`, and the requested variable is not found, behave as if
+   were the value assigned to the that variable.
+
 CONFIGURATION
 -
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index 9eda01615b..4f222a0ca9 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -148,6 +149,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes_opt, N_("respect include 
directives on lookup")),
OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, standard input, blob, command line)")),
+   OPT_STRING(0, "default", _value, N_("value"), N_("with --get, 
use default value when missing entry")),
OPT_END(),
 };
 
@@ -312,6 +314,16 @@ static int get_value(const char *key_, const char *regex_)
config_with_options(collect_config, ,
_config_source, _options);
 
+   if (!values.nr && default_value) {
+   struct strbuf *item;
+   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+   item = [values.nr++];
+   strbuf_init(item, 0);
+   if (format_config(item, key_, default_value) < 0)
+   die(_("failed to format default config value: %s"),
+   default_value);
+   }
+
ret = !values.nr;
 
for (i = 0; i < values.nr; i++) {
@@ -650,6 +662,12 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
 
+   if (default_value && !(actions & ACTION_GET)) {
+   error("--default is only applicable to --get");
+   usage_with_options(builtin_config_usage,
+   builtin_config_options);
+   }
+
if (actions & PAGING_ACTIONS)
setup_auto_pager("config", 1);
 
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 00..6049d91708
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ 

Re: [PATCH 5/5] builtin/config: introduce `color` type specifier

[Junio C Hamano]

Taylor Blau  writes:

> diff --git a/builtin/config.c b/builtin/config.c
> index d7acf912cd..ec5c11293b 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -61,6 +61,7 @@ static int show_origin;
>  #define TYPE_BOOL_OR_INT 3
>  #define TYPE_PATH4
>  #define TYPE_EXPIRY_DATE 5
> +#define TYPE_COLOR   6
>  
>  #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
>   { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
> @@ -231,6 +232,11 @@ static int format_config(struct strbuf *buf, const char 
> *key_, const char *value
>   if (git_config_expiry_date(, key_, value_) < 0)
>   return -1;
>   strbuf_addf(buf, "%"PRItime, t);
> + } else if (type == TYPE_COLOR) {
> + char v[COLOR_MAXLEN];
> + if (git_config_color(v, key_, value_) < 0)
> + return -1;
> + strbuf_addstr(buf, v);
>   } else if (value_) {
>   strbuf_addstr(buf, value_);
>   } else {
> @@ -376,6 +382,20 @@ static char *normalize_value(const char *key, const char 
> *value)
>   else
>   return xstrdup(v ? "true" : "false");
>   }
> + if (type == TYPE_COLOR) {
> + char v[COLOR_MAXLEN];
> + if (git_config_color(v, key, value))
> + die("cannot parse color '%s'", value);
> +
> + /*
> +  * The contents of `v` now contain an ANSI escape
> +  * sequence, not suitable for including within a
> +  * configuration file. Treat the above as a
> +  * "sanity-check", and return the given value, which we
> +  * know is representable as valid color code.
> +  */
> + return xstrdup(value);
> + }
>  
>   die("BUG: cannot normalize type %d", type);
>  }

Hmmm, option_parse_type() introduced in [PATCH 2/5] used to learn
to parse "color" in this step, but it no longer does.  

Here is the difference between what has been queued and the result
of applying these latest patches I found.

diff --git a/builtin/config.c b/builtin/config.c
index 69e7270356..ec5c11293b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -72,7 +72,8 @@ static struct option builtin_config_options[];
 static int option_parse_type(const struct option *opt, const char *arg,
 int unset)
 {
-   int new_type, *to_type;
+   int new_type;
+   int *to_type;
 
if (unset) {
*((int *) opt->value) = 0;
@@ -95,13 +96,11 @@ static int option_parse_type(const struct option *opt, 
const char *arg,
new_type = TYPE_PATH;
else if (!strcmp(arg, "expiry-date"))
new_type = TYPE_EXPIRY_DATE;
-   else if (!strcmp(arg, "color"))
-   new_type = TYPE_COLOR;
else
die(_("unrecognized --type argument, %s"), arg);
}
 
-   to_type = opt->value;
+   *to_type = opt->value;
if (*to_type && *to_type != new_type) {
/*
 * Complain when there is a new type not equal to the old type.

Re: [PATCH 2/5] builtin/config.c: support `--type=` as preferred alias for `--type`

[Junio C Hamano]

Taylor Blau  writes:

> Subject: Re: [PATCH 2/5] builtin/config.c: support `--type=` as 
> preferred alias for `--type`

I'd retitle while queuing, as the last 'type' is a placeholder for
concrete types like  above.

> +static int option_parse_type(const struct option *opt, const char *arg,
> +  int unset)
> +{
> + int new_type;
> + int *to_type;

Splitting these into two lines (unlike placing on a single same
line, which was how the previous round was queued) like this is
good.

> +...
> + new_type = opt->defval;
> + if (!new_type) {
> +...
> + }
> +
> + *to_type = opt->value;

But this is wrong, no?  You meant opt->value points at an integer
variable that receives the type we discover by parsing, i.e.

to_type = opt->value;

Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[Junio C Hamano]

Elijah Newren  writes:

> On Wed, Apr 25, 2018 at 1:37 AM, Junio C Hamano  wrote:
>
>> * en/unpack-trees-split-index-fix (2018-04-24) 1 commit
>> * en/rename-directory-detection-reboot (2018-04-25) 36 commits
> ...
> Would you still like a re-roll?

Not really.  We can just merge them together and move on.

[PATCH 0/5] builtin/config.c: combined series '--type', '--default'

[Taylor Blau]

Hi,

Attached is a combination of my two series to add [--type=] and
[--default=] to 'git-config(1)'.

I have not changed anything in these patches since their last review in
[1], [2], other than to:

  * Ensure that they merge cleanly into 'master' and,

  * Incorporate the patch that I sent to Junio in [3], to remove any
decl-after-statement's.

My intention is that these patches will be easier to queue together and
at the same time, rather than individually. This was suggested to me by
Eric in [4].


Thanks,
Taylor

[1]: https://public-inbox.org/git/xmqq8t9jgbe1@gitster-ct.c.googlers.com
[2]: https://public-inbox.org/git/xmqqk1tf4yhl@gitster-ct.c.googlers.com
[3]: https://public-inbox.org/git/20180419030142.GA28273@syl.local
[4]: 
https://public-inbox.org/git/CAPig+cSr744Y293qvgLG8jLHdNsGypkHU6QUQ-AcOyk=-ja...@mail.gmail.com

Taylor Blau (5):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: support `--type=` as preferred alias for
`--type`
  builtin/config: introduce `--default`
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `color` type specifier

 Documentation/git-config.txt |  81 
 builtin/config.c | 143 ---
 config.c |  10 +++
 config.h |   1 +
 t/t1300-repo-config.sh   |  93 +++
 t/t1310-config-default.sh|  36 +
 6 files changed, 305 insertions(+), 59 deletions(-)
 create mode 100755 t/t1310-config-default.sh

--
2.17.0

[PATCH 4/5] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--type=color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_`.

Signed-off-by: Taylor Blau 
---
 config.c | 10 ++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index 62c56099bf..d14813bef7 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const 
char *var, const char *
return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+   if (!value)
+   return config_error_nonbool(var);
+   if (color_parse(value, dest) < 0)
+   return -1;
+   return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac1..0e060779d9 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const 
char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.17.0

[PATCH 5/5] builtin/config: introduce `color` type specifier

[Taylor Blau]

As of this commit, the canonical way to retreive an ANSI-compatible
color escape sequence from a configuration file is with the
`--get-color` action.

This is to allow Git to "fall back" on a default value for the color
should the given section not exist in the specified configuration(s).

With the addition of `--default`, this is no longer needed since:

  $ git config --default red --type=color core.section

will be have exactly as:

  $ git config --get-color core.section red

For consistency, let's introduce `--type=color` and encourage its use
with `--default` together over `--get-color` alone.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |  6 ++
 builtin/config.c | 20 
 t/t1300-repo-config.sh   | 30 ++
 3 files changed, 56 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index c3adafd78a..18ddc78f42 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -177,6 +177,10 @@ Valid ``'s include:
   ~/` from the command line to let your shell do the expansion.)
 - 'expiry-date': canonicalize by converting from a fixed or relative 
date-string
   to a timestamp. This specifier has no effect when setting the value.
+- 'color': When getting a value, canonicalize by converting to an ANSI color
+  escape sequence. When setting a value, a sanity-check is performed to ensure
+  that the given value is canonicalize-able as an ANSI color, but it is written
+  as-is.
 +
 
 --bool::
@@ -228,6 +232,8 @@ Valid ``'s include:
output it as the ANSI color escape sequence to the standard
output.  The optional `default` parameter is used instead, if
there is no color configured for `name`.
++
+`--type=color [--default=]` is preferred over `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index d7acf912cd..ec5c11293b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT   3
 #define TYPE_PATH  4
 #define TYPE_EXPIRY_DATE   5
+#define TYPE_COLOR 6
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
@@ -231,6 +232,11 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (git_config_expiry_date(, key_, value_) < 0)
return -1;
strbuf_addf(buf, "%"PRItime, t);
+   } else if (type == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (git_config_color(v, key_, value_) < 0)
+   return -1;
+   strbuf_addstr(buf, v);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -376,6 +382,20 @@ static char *normalize_value(const char *key, const char 
*value)
else
return xstrdup(v ? "true" : "false");
}
+   if (type == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (git_config_color(v, key, value))
+   die("cannot parse color '%s'", value);
+
+   /*
+* The contents of `v` now contain an ANSI escape
+* sequence, not suitable for including within a
+* configuration file. Treat the above as a
+* "sanity-check", and return the given value, which we
+* know is representable as valid color code.
+*/
+   return xstrdup(value);
+   }
 
die("BUG: cannot normalize type %d", type);
 }
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 1e3a1ace14..2acfd513f1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
test_must_fail git config --expiry-date date.invalid1
 '
 
+test_expect_success 'get --type=color' '
+   rm .git/config &&
+   git config foo.color "red" &&
+   git config --get --type=color foo.color >actual.raw &&
+   test_decode_color actual &&
+   echo "" >expect &&
+   test_cmp expect actual
+'
+
+cat >expect << EOF
+[foo]
+   color = red
+EOF
+
+test_expect_success 'set --type=color' '
+   rm .git/config &&
+   git config --type=color foo.color "red" &&
+   test_cmp expect .git/config
+'
+
+test_expect_success 'get --type=color barfs on non-color' '
+   echo "[foo]bar=not-a-color" >.git/config &&
+   test_must_fail git config --get --type=color foo.bar
+'
+
+test_expect_success 'set --type=color barfs on non-color' '
+   test_must_fail git config --type=color foo.color "not-a-color" 2>error 
&&
+   test_i18ngrep "cannot parse color" error
+'
+
 cat > expect << EOF
 [quote]
leading = " test"
-- 
2.17.0

[PATCH 3/5] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the variable asked for does not exist.
In addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
variable and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--type=color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color variable [default] [...]

with:

  $ git config --default default --type=color variable [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--type=color`, which (in
conjunction with `--default`) will be sufficient to replace
`--get-color`.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |  4 
 builtin/config.c | 18 ++
 t/t1310-config-default.sh| 36 
 3 files changed, 58 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index b759009c75..c3adafd78a 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -240,6 +240,10 @@ Valid ``'s include:
using `--file`, `--global`, etc) and `on` when searching all
config files.
 
+--default ::
+  When using `--get`, and the requested variable is not found, behave as if
+   were the value assigned to the that variable.
+
 CONFIGURATION
 -
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index 2f91ef15a4..d7acf912cd 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -150,6 +151,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes_opt, N_("respect include 
directives on lookup")),
OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, standard input, blob, command line)")),
+   OPT_STRING(0, "default", _value, N_("value"), N_("with --get, 
use default value when missing entry")),
OPT_END(),
 };
 
@@ -314,6 +316,16 @@ static int get_value(const char *key_, const char *regex_)
config_with_options(collect_config, ,
_config_source, _options);
 
+   if (!values.nr && default_value) {
+   struct strbuf *item;
+   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+   item = [values.nr++];
+   strbuf_init(item, 0);
+   if (format_config(item, key_, default_value) < 0)
+   die(_("failed to format default config value: %s"),
+   default_value);
+   }
+
ret = !values.nr;
 
for (i = 0; i < values.nr; i++) {
@@ -652,6 +664,12 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
 
+   if (default_value && !(actions & ACTION_GET)) {
+   error("--default is only applicable to --get");
+   usage_with_options(builtin_config_usage,
+   builtin_config_options);
+   }
+
if (actions & PAGING_ACTIONS)
setup_auto_pager("config", 1);
 
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 00..6049d91708
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ 

[PATCH 1/5] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau 
---
 builtin/config.c   | 49 +++---
 t/t1300-repo-config.sh | 11 ++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd628..92fb8d56b1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL  1
+#define TYPE_INT   2
+#define TYPE_BOOL_OR_INT   3
+#define TYPE_PATH  4
+#define TYPE_EXPIRY_DATE   5
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "get-color", , N_("find the color configured: slot 
[default]"), ACTION_GET_COLOR),
OPT_BIT(0, "get-colorbool", , N_("find the color setting: slot 
[stdout-is-tty]"), ACTION_GET_COLORBOOL),
OPT_GROUP(N_("Type")),
-   OPT_BIT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
-   OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT),
-   OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
-   OPT_BIT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
-   OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
+   OPT_SET_INT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
+   OPT_SET_INT(0, "int", , N_("value is decimal number"), TYPE_INT),
+   OPT_SET_INT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
+   OPT_SET_INT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
+   OPT_SET_INT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (show_keys)
strbuf_addch(buf, key_delim);
 
-   if (types == TYPE_INT)
+   if (type == TYPE_INT)
strbuf_addf(buf, "%"PRId64,
git_config_int64(key_, value_ ? value_ : 
""));
-   else if (types == TYPE_BOOL)
+   else if (type == TYPE_BOOL)
strbuf_addstr(buf, git_config_bool(key_, value_) ?
  "true" : "false");
-   else if (types == TYPE_BOOL_OR_INT) {
+   else if (type == TYPE_BOOL_OR_INT) {
int is_bool, v;
v = git_config_bool_or_int(key_, value_, _bool);
if (is_bool)
strbuf_addstr(buf, v ? "true" : "false");
else
strbuf_addf(buf, "%d", v);
-   } else if (types == TYPE_PATH) {
+   } else if (type == TYPE_PATH) {
const char *v;
if (git_config_pathname(, key_, value_) < 0)
return -1;
strbuf_addstr(buf, v);
free((char 

[PATCH 2/5] builtin/config.c: support `--type=` as preferred alias for `--type`

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values can be interpreted as that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid squatting on this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we support `--type=` in
addition to `--int`, `--bool`, and etc. This allows the aforementioned
upcoming patch to support querying a color value with a default via
`--type=color --default=...`, without squandering `--color`.

We retain the historic behavior of complaining when multiple,
legacy-style `--` flags are given, as well as extend this to
conflicting new-style `--type=` flags. `--int --type=int` (and its
commutative pair) does not complain, but `--bool --type=int` (and its
commutative pair) does.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 71 
 builtin/config.c | 66 ++---
 t/t1300-repo-config.sh   | 58 +++--
 3 files changed, 155 insertions(+), 40 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d5..b759009c75 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
-'git config' [] [type] --add name value
-'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
-'git config' [] [type] [-z|--null] --get-urlmatch name URL
+'git config' [] [--type=] [--show-origin] [-z|--null] name 
[value [value_regex]]
+'git config' [] [--type=] --add name value
+'git config' [] [--type=] --replace-all name value 
[value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] --get 
name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
--get-all name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
[--name-only] --get-regexp name_regex [value_regex]
+'git config' [] [--type=] [-z|--null] --get-urlmatch name 
URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset. 
 If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given .  If no
+`--type=` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,39 @@ See also <>.
 --list::
List all variables set in config file, along with their values.
 
---bool::
-   'git config' will ensure that the output is "true" or "false"
+--type ::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in ``'s
+  canonical form.
++
+Valid ``'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value 

Re: [PATCH v5 00/11] Deprecate .git/info/grafts

[Junio C Hamano]

Johannes Schindelin  writes:

>  -if (export_object(_oid, type, raw, tmpfile))
>  -return -1;
>  -if (launch_editor(tmpfile, NULL, NULL) < 0)
>  -return error("editing object file failed");
>  -if (import_object(_oid, type, raw, tmpfile))
>  +tmpfile = git_pathdup("REPLACE_EDITOBJ");
>  +if (export_object(_oid, type, raw, tmpfile) ||
>  +(launch_editor(tmpfile, NULL, NULL) < 0 &&
>  + error("editing object file failed")) ||
>  +import_object(_oid, type, raw, tmpfile)) {
>  +free(tmpfile);
>   return -1;
>  -
>  +}

I know the above is to avoid leaking tmpfile, but a single if ()
condition that makes multiple calls to functions primarily for their
side effects is too ugly to live.  Perhaps something like

if (export_object(...))
goto clean_fail;
if (launch_editor(...)) {
error("editing object file failed");
goto clean_fail;
}
if (import_object(...)) {
clean_fail:
free(tmpfile);
return -1;
}

would keep the ease-of-reading of the original while plugging the
leak.  It would even be cleaner to move the body of clean_fail:
completely out of line, instead of having it in the last of three
steps like the above.

Other than that, looked cleanly done.  Thanks for a pleasant read.

Will queue.

Re: [PATCH v9 00/17] rebase -i: offer to recreate commit topology by rebasing merges

[Junio C Hamano]

Johannes Schindelin  writes:

> Changes since v8:
>
> - Disentangled the patch introducing `label`/`reset` from the one
>   introducing `merge` again (this was one stupid, tired `git commit
>   --amend` too many).
>
> - Augmented the commit message of "introduce the `merge` command" to
>   describe what the `label onto` is all about.
>
> - Fixed the error message when `reset` would overwrite untracked files to
>   actually say that a "reset" failed (not a "merge").
>
> - Clarified the rationale for `label onto` in the commit message of
>   "rebase-helper --make-script: introduce a flag to rebase merges".
>
> - Edited the description of `--rebase-merges` heavily, for clarity, in
>   "rebase: introduce the --rebase-merges option".
>
> - Edited the commit message of (and the documentation introduced by) " rebase
>   -i: introduce --rebase-merges=[no-]rebase-cousins" for clarity (also
>   mentioning the `--ancestry-path` option).
>
> - When run_git_commit() fails after a successful merge, we now take pains
>   not to reschedule the `merge` command.
>
> - Rebased the patch series on top of current `master`, i.e. both
>   `pw/rebase-keep-empty-fixes` and `pw/rebase-signoff`, to resolve merge
>   conflicts myself.

Good to see the last item, as this gave me a chance to make sure
that the conflict resolution I've been carrying matches how you
would have resolved as the original author.  Applying these on the
old base (with minor conflict resolution) to match the old iteration
and merging the result to the new base1f1cddd5 ("The fourth batch
for 2.18", 2018-04-25) resulted in the same tree as the tree that
results from applying these on top of the new base.

That was done only to validate the result of the past resolution
(and also seeing the interdiff from the old iteration).  There is no
reason to keep this series back-portable to older tip of 'master',
so I'll queue the result of applying the patches to the new base.

Re: [PATCH v4 04/10] commit: use generations in paint_down_to_common()

[Junio C Hamano]

Derrick Stolee  writes:

> Define compare_commits_by_gen_then_commit_date(), which uses generation
> numbers as a primary comparison and commit date to break ties (or as a
> comparison when both commits do not have computed generation numbers).
>
> Since the commit-graph file is closed under reachability, we know that
> all commits in the file have generation at most GENERATION_NUMBER_MAX
> which is less than GENERATION_NUMBER_INFINITY.

I suspect that my puzzlement may be coming from my not "getting"
what you meant by "closed under reachability", but could you also
explain how _INF and _ZERO interact with commits with normal
generation numbers?  I've always assumed that genno will be used
only when comparing two commits with valid genno and otherwise we'd
fall back to the traditional date based one, but...

> +int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
> void *unused)
> +{
> + const struct commit *a = a_, *b = b_;
> +
> + /* newer commits first */
> + if (a->generation < b->generation)
> + return 1;
> + else if (a->generation > b->generation)
> + return -1;

... this does not check if a->generation is _ZERO or _INF.  

Both being _MAX is OK (the control will fall through and use the
dates below).  One being _MAX and the other being a normal value is
also OK (the above comparisons will declare the commit with _MAX is
farther than less-than-max one from a root).

Or is the assumption that if one has _ZERO, that must have come from
an ancient commit-graph file and none of the commits have anything
but _ZERO?

> + /* use date as a heuristic when generations are equal */
> + if (a->date < b->date)
> + return 1;
> + else if (a->date > b->date)
> + return -1;
> + return 0;
> +}
> +
>  int compare_commits_by_commit_date(const void *a_, const void *b_, void 
> *unused)
>  {
>   const struct commit *a = a_, *b = b_;
> @@ -789,7 +807,7 @@ static int queue_has_nonstale(struct prio_queue *queue)
>  /* all input commits in one and twos[] must have been parsed! */
>  static struct commit_list *paint_down_to_common(struct commit *one, int n, 
> struct commit **twos)
>  {
> - struct prio_queue queue = { compare_commits_by_commit_date };
> + struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
>   struct commit_list *result = NULL;
>   int i;
>  
> diff --git a/commit.h b/commit.h
> index aac3b8c56f..64436ff44e 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -341,6 +341,7 @@ extern int remove_signature(struct strbuf *buf);
>  extern int check_commit_signature(const struct commit *commit, struct 
> signature_check *sigc);
>  
>  int compare_commits_by_commit_date(const void *a_, const void *b_, void 
> *unused);
> +int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
> void *unused);
>  
>  LAST_ARG_MUST_BE_NULL
>  extern int run_commit_hook(int editor_is_used, const char *index_file, const 
> char *name, ...);

Re: [PATCH v4 03/10] commit-graph: compute generation numbers

[Junio C Hamano]

Derrick Stolee  writes:

> @@ -439,6 +439,9 @@ static void write_graph_chunk_data(struct hashfile *f, 
> int hash_len,
>   else
>   packedDate[0] = 0;
>  
> + if ((*list)->generation != GENERATION_NUMBER_INFINITY)
> + packedDate[0] |= htonl((*list)->generation << 2);
> +
>   packedDate[1] = htonl((*list)->date);
>   hashwrite(f, packedDate, 8);

The ones that have infinity are written as zero here.  The code that
reads the generation field off of a file in fill_commit_graph_info()
and fill_commit_in_graph() both leave such a record in file as-is,
so the reader of what we write out will think it is _ZERO, not _INF.

Not that it matters, as it seems that most of the code being added
by this series treat _ZERO and _INF more or less interchangeably.
But it does raise another question, i.e. do we need both _ZERO and
_INF, or is it sufficient to have just a single _UNKNOWN?

> @@ -571,6 +574,46 @@ static void close_reachable(struct packed_oid_list *oids)
>   }
>  }
>  
> +static void compute_generation_numbers(struct commit** commits,
> +int nr_commits)
> +{
> + int i;
> + struct commit_list *list = NULL;
> +
> + for (i = 0; i < nr_commits; i++) {
> + if (commits[i]->generation != GENERATION_NUMBER_INFINITY &&
> + commits[i]->generation != GENERATION_NUMBER_ZERO)
> + continue;
> +
> + commit_list_insert(commits[i], );
> + while (list) {
> + struct commit *current = list->item;
> + struct commit_list *parent;
> + int all_parents_computed = 1;
> + uint32_t max_generation = 0;
> +
> + for (parent = current->parents; parent; parent = 
> parent->next) {
> + if (parent->item->generation == 
> GENERATION_NUMBER_INFINITY ||
> + parent->item->generation == 
> GENERATION_NUMBER_ZERO) {
> + all_parents_computed = 0;
> + commit_list_insert(parent->item, );
> + break;
> + } else if (parent->item->generation > 
> max_generation) {
> + max_generation = 
> parent->item->generation;
> + }
> + }
> +
> + if (all_parents_computed) {
> + current->generation = max_generation + 1;
> + pop_commit();
> + }

If we haven't computed all parents' generations yet,
current->generation is undefined (or at least "left as
initialized"), so it does not make much sense to attempt to clip it
at _MAX at this point.  At leat not yet.

IOW, shouldn't the following two lines be inside the "we now know
genno of all parents, so we can compute genno for commit" block
above?

> + if (current->generation > GENERATION_NUMBER_MAX)
> + current->generation = GENERATION_NUMBER_MAX;
> + }
> + }
> +}
> +
>  void write_commit_graph(const char *obj_dir,
>   const char **pack_indexes,
>   int nr_packs,
> @@ -694,6 +737,8 @@ void write_commit_graph(const char *obj_dir,
>   if (commits.nr >= GRAPH_PARENT_MISSING)
>   die(_("too many commits to write graph"));
>  
> + compute_generation_numbers(commits.list, commits.nr);
> +
>   graph_name = get_commit_graph_filename(obj_dir);
>   fd = hold_lock_file_for_update(, graph_name, 0);

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[Junio C Hamano]

SZEDER Gábor  writes:

> These new tests, however, are primarily interested in the inner
> workings of __git_complete_index_file() in the presence of escapes
> and/or quotes in the path to be completed and/or in the output of 'git
> ls-files'.  For these kind of tests we could simply invoke
> __git_complete_index_file() directly, like we call __git_refs()
> directly to test refs completion.  Then we could set the current path
> to be completed to whatever we want, including spaces, because it
> won't be subject to field splitting like the command line given to
> 'test_completion'.
>
> So, I think for v2 I will rewrite these tests to call
> __git_complete_index_file() directly instead of using
> 'test_completion', and will include a test with spaces in path names.

Quite well thought-out reasoning.  Thanks.

Re: [PATCH v3] Make running git under other debugger-like programs easy

[Junio C Hamano]

Johannes Schindelin  writes:

>> ...
>> 
>> There is also a handy shortcut of GIT_DEBUGGER=1 meaning the same as
>> GIT_DEBUGGER="gdb --args"
>> 
>> Original-patch-by: Johannes Schindelin 
>> Signed-off-by: Elijah Newren 
>
> Looks good to me!
> Dscho

Thanks, both.

Re: [PATCH v2 0/2] add additional config settings for merge

[Junio C Hamano]

Ben Peart  writes:

> To be clear, this documentation change isn't trying to document any
> changes to the code or behavior - it is just an attempt to clarify
> what the existing behavior is.

Yeah, I know, and I do think the new text is a good first step in
the right direction; I merely was trying to help in the clarifying
effort ;-)

Re: [RFC PATCH] checkout: Force matching mtime between files

[Junio C Hamano]

Marc Branchaud  writes:

>> But Git is not an archiver (tar), but is a source code control
>> system, so I do not think we should spend any extra cycles to
>> "improve" its behaviour wrt the relative ordering, at least for the
>> default case.  Only those who rely on having build artifact *and*
>> source should pay the runtime (and preferrably also the
>> maintainance) cost.
>
> Anyone who uses "make" or some other mtime-based tool is affected by
> this.  I agree that it's not "Everyone" but it sure is a lot of
> people.

That's an exaggerated misrepresentation.  Only those who put build
artifacts as well as source to SCM *AND* depend on mtime are
affected.

A shipped tarball often contain configure.in as well as generated
configure, so that consumers can just say ./configure without having
the whole autoconf toolchain to regenerate it (I also heard horror
stories that this is done to control the exact version of autoconf
to avoid compatibility issues), but do people arrange configure to
be regenerated from configure.in in their Makefile of such a project
automatically when building the default target?  In any case, that is
a tarball usecase, not a SCM one.

> Are we all that sure that the performance hit is that drastic?  After
> all, we've just done write_entry().  Calling utime() at that point
> should just hit the filesystem cache.

I do not know about others, but I personally am more disburbed by
the conceptual ugliness that comes from having to have such a piece
of code in the codebase.

Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[brian m. carlson]

On Wed, Apr 25, 2018 at 05:37:52PM +0900, Junio C Hamano wrote:
> * bc/object-id (2018-04-24) 41 commits
>  - merge-one-file: compute empty blob object ID
>  - add--interactive: compute the empty tree value
>  - Update shell scripts to compute empty tree object ID
>  - sha1_file: only expose empty object constants through git_hash_algo
>  - dir: use the_hash_algo for empty blob object ID
>  - sequencer: use the_hash_algo for empty tree object ID
>  - cache-tree: use is_empty_tree_oid
>  - sha1_file: convert cached object code to struct object_id
>  - builtin/reset: convert use of EMPTY_TREE_SHA1_BIN
>  - builtin/receive-pack: convert one use of EMPTY_TREE_SHA1_HEX
>  - wt-status: convert two uses of EMPTY_TREE_SHA1_HEX
>  - submodule: convert several uses of EMPTY_TREE_SHA1_HEX
>  - sequencer: convert one use of EMPTY_TREE_SHA1_HEX
>  - merge: convert empty tree constant to the_hash_algo
>  - builtin/merge: switch tree functions to use object_id
>  - builtin/am: convert uses of EMPTY_TREE_SHA1_BIN to the_hash_algo
>  - builtin/receive-pack: avoid hard-coded constants for push certs
>  - diff: specify abbreviation size in terms of the_hash_algo
>  - upload-pack: replace use of several hard-coded constants
>  - revision: replace use of hard-coded constants
>  - http: eliminate hard-coded constants
>  - dir: convert struct untracked_cache_dir to object_id
>  - commit: convert uses of get_sha1_hex to get_oid_hex
>  - index-pack: abstract away hash function constant
>  - pack-redundant: convert linked lists to use struct object_id
>  - Update struct index_state to use struct object_id
>  - split-index: convert struct split_index to object_id
>  - submodule-config: convert structures to object_id
>  - fsck: convert static functions to struct object_id
>  - tree-walk: convert get_tree_entry_follow_symlinks to object_id
>  - tree-walk: avoid hard-coded 20 constant
>  - pack-redundant: abstract away hash algorithm
>  - pack-objects: abstract away hash algorithm
>  - packfile: abstract away hash constant values
>  - packfile: convert find_pack_entry to object_id
>  - sha1_file: convert freshen functions to object_id
>  - packfile: convert has_sha1_pack to object_id
>  - packfile: remove unused member from struct pack_entry
>  - Remove unused member in struct object_context
>  - server-info: remove unused members from struct pack_info
>  - cache: add a function to read an object ID from a buffer
> 
>  Conversion from uchar[20] to struct object_id continues.

I do plan to reroll this, and if you want to forge my sign-off on the
patch I missed in the mean time, please do.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[SZEDER Gábor]

On Wed, Apr 18, 2018 at 3:22 AM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>>> Do we want to test a more common case of a filename that is two
>>> words with SP in between, i.e.
>>>
>>> $ >'hello world' && git add hel
>>>
>>> or is it known to work just fine without quoting/escaping (because
>>> the funny we care about is output from ls-files and SP is not special
>>> in its one-item-at-a-time-on-a-line output) and not worth checking?
>>
>> This particular case already works, even without this patch series.
>
> I was more wondering about preventing regressions---"it worked
> without this patch series, but now it is broken" is what I was
> worried about.
>
>> The problems start when you want to complete the filename after a space,
>> e.g. 'hello\ w> was the first thing I tried to write a test for, but it didn't work out:
>> inside the 'test_completion' helper function the space acts as
>> separator, and the completion script then sees 'hello\' and 'w' as two
>> separate words.
>
> Hmph.  That is somewhat unfortunate.

Actually, I used 'test_completion' in these new tests, because there
is that big test checking file completion for various commands, and it
already uses 'test_completion', so I just followed suit.  Now, that
test checks that the right type(s) of files are listed for various git
commands, e.g. modified and untracked for 'git add', IOW that the
caller of __git_complete_index_file() specifies the appropriate 'git
ls-files' options.  For those kind of checks 'test_completion' is
great.

These new tests, however, are primarily interested in the inner
workings of __git_complete_index_file() in the presence of escapes
and/or quotes in the path to be completed and/or in the output of 'git
ls-files'.  For these kind of tests we could simply invoke
__git_complete_index_file() directly, like we call __git_refs()
directly to test refs completion.  Then we could set the current path
to be completed to whatever we want, including spaces, because it
won't be subject to field splitting like the command line given to
'test_completion'.

So, I think for v2 I will rewrite these tests to call
__git_complete_index_file() directly instead of using
'test_completion', and will include a test with spaces in path names.

Re: [PATCHv3 0/9] object store: oid_object_info is the next contender

[Jonathan Tan]

On Wed, 25 Apr 2018 11:20:57 -0700
Stefan Beller  wrote:

> v3:
> * fixed and extended the commit message of last commit
> * fixed the last patch, as Jonathan Tan suggested, see interdiff:

Thanks, all my comments have been addressed.

Reviewed-by: Jonathan Tan 

Re: [RFC PATCH] checkout: Force matching mtime between files

[Robin H. Johnson]

On Wed, Apr 25, 2018 at 11:18:26AM -0400, Marc Branchaud wrote:
> > The best approach to do so is to have those people do the "touch"
> > thing in their own post-checkout hook.  People who use Git as the
> > source control system won't have to pay runtime cost of doing the
> > touch thing, and we do not have to maintain such a hook script.
> > Only those who use the "feature" would.
> 
> The post-checkout hook approach is not exactly straightforward.
> 
> Naively, it's simply
> 
>   for F in `git diff --name-only $1 $2`; do touch "$F"; done
Even this naive attempt gets it wrong: successive files have increasing
times.  You need to capture the time at the start, and use it as the time
for the files.
  touch /tmp/ref && \
  for F in `git diff --name-only $1 $2`; do touch -r /tmp/ref "$F"; done && \
  rm /tmp/ref
(or pass a fixed time into touch).

> But consider:
> 
> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's 
> proposed patch also doesn't deal with symlinks.) 
Yes, it blindly touches the file, and rather than trying to do
utimensat's AT_SYMLINK_NOFOLLOW flag.

> Let's assume that a 
> hook can be crafted will all possible sophistication.  There are still 
> some fundamental problems:
>
> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are 
> identical so the above loop does nothing.  Offhand I'm not even sure how 
> a hook might get the right files in this case.
Yes, this would need to be a new hook that knows more than post-checkout
presently does.

post-checkout right now runs AFTER the worktree has been updated, and
only gets the refs of old/new HEAD and if the branch was changed.
It does NOT know which files were actually modified, and since it

If a hook is added for that, then this behavior can be implemented in
the hook. Alternatively adding a pre-checkout hook to absorb some state
of the unmodified worktree (this could be a bit expensive).

> * The hook has to be set up in every repo and submodule (at least until 
> something like Ævar's experiments come to fruition).
> 
> * A fresh clone can't run the hook.  This is especially important when 
> dealing with submodules.  (In one case where we were bit by this, make 
> though that half of a fresh submodule clone's files were stale, and 
> decided to re-autoconf the entire thing.)
The fresh clone case really matters for my usage, where new clones are
firing in CI/CD processes.

> I just don't think the hook approach can completely solve the problem.
> 
> I appreciate Ævar's concern that there are more than just two mtime 
> requests floating around.  But I think git's users are best served by a 
> built-in approach, with a config setting to control the desired mtime 
> handling (defaulting to the current behaviour).  People who want a 
> different mtime solution will at least have a clear place in the code to 
> propose a patch.
+1 as long as we can set the behavior during the clone.

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robb...@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 11:21 schrieb Phillip Wood:

On 24/04/18 17:59, Johannes Sixt wrote:


In modern setups, less, the pager, uses alternate screen to show
the content. When it is closed, it switches back to the original
screen, and all content is gone.


Are you setting LESS explicitly in the environment?

 From the git config man page:
When the LESS environment variable is unset, Git sets it to FRX (if LESS 
environment variable is set, Git does not change it at all).


 From the less man page the X option:
Disables  sending the termcap initialization and deinitialization 
strings to the terminal.  This is sometimes desirable if the 
deinitialization string does something unnecessary, like clearing the 
screen.


So with the default setup the output should remain on the screen.


Right. But I have LESS=RS because I do *not* want it to remain on screen 
by default.


-- Hannes

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 09:41 schrieb Johannes Schindelin:

Hi Hannes,

On Wed, 25 Apr 2018, Johannes Sixt wrote:


Am 25.04.2018 um 02:05 schrieb Junio C Hamano:

Johannes Sixt  writes:

It is not uncommon to request that the output remains visible in
the terminal.


I ran `git log` and then hit `q`, and the latest screen contents were
still visible in all of these scenarios:

- in a Linux VM via SSH client

- in Git Bash on Windows

- in Git CMD on Windows

Granted, this is only the latest screen, but that is usually good enough
here.

Is anything different happening in your setups?


I have LESS=RS in my environment, because I do want that the alternate 
screen is used by the pager. Nevertheless, occasionally I want git's 
output to remain visible.


-- Hannes

Re: [PATCH 18/41] index-pack: abstract away hash function constant

[Martin Ågren]

On 25 April 2018 at 01:51, brian m. carlson
 wrote:
> On Tue, Apr 24, 2018 at 11:50:16AM +0200, Martin Ågren wrote:
>> On 24 April 2018 at 01:39, brian m. carlson
>>  wrote:
>> > The code for reading certain pack v2 offsets had a hard-coded 5
>> > representing the number of uint32_t words that we needed to skip over.
>> > Specify this value in terms of a value from the_hash_algo.
>> >
>> > Signed-off-by: brian m. carlson 
>> > ---
>> >  builtin/index-pack.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> > index d81473e722..c1f94a7da6 100644
>> > --- a/builtin/index-pack.c
>> > +++ b/builtin/index-pack.c
>> > @@ -1543,12 +1543,13 @@ static void read_v2_anomalous_offsets(struct 
>> > packed_git *p,
>> >  {
>> > const uint32_t *idx1, *idx2;
>> > uint32_t i;
>> > +   const uint32_t hashwords = the_hash_algo->rawsz / sizeof(uint32_t);
>>
>> Should we round up? Or just what should we do if a length is not
>> divisible by 4? (I am not aware of any such hash functions, but one
>> could exist for all I know.) Another question is whether such an
>> index-pack v2 will ever contain non-SHA-1 oids to begin with. I can't
>> find anything suggesting that it could, but this is unfamiliar code to
>> me.
>
> I opted not to simply because I know that our current hash is 20 bytes
> and the new one will be 32, and I know those are both divisible by 4.  I
> feel confident that any future hash we choose will also be divisible by
> 4, and the code is going to be complicated if it isn't.
>
> I agree that pack v2 is not going to have anything but SHA-1.  However,
> writing all the code such that it's algorithm agnostic means that we can
> do testing of new algorithms by wholesale replacing the algorithm with a
> new one, which simplifies things considerably.

Ok. I do sort of wonder if a "successful" test run after globally
substituting Hash-Foo for SHA-1 (regardless of whether the size changes
or not) hints at a problem. That is, nowhere do we test that this code
uses 20-byte SHA-1s, regardless of what other hash functions are
available and configured. Of course, until soon, that did not really
have to be tested since there was only one hash function available to
choose from. As for identifying all the places that matter ... no idea.

Of course I can see how this helps get things to a point where Git does
not crash and burn because the hash has a different size, and where the
test suite doesn't spew failures because the initial chaining value of
"SHA-1" is changed.

Once that is accomplished, I sort of suspect that this code will want to
be updated to not always blindly use the_hash_algo, but to always work
with SHA-1 sizes. Or rather, this would turn into more generic code to
handle both "v2 with SHA-1" and "v3 with some hash function(s)". This
commit might be a good first step in that direction.

Long rambling short, yeah, I see your point.

Martin

Re: [PATCH v4/wip 06/12] git: accept multiple --list-cmds options

[Eric Sunshine]

On Wed, Apr 25, 2018 at 12:31 PM, Nguyễn Thái Ngọc Duy
 wrote:
> Later on we may support non-overlapping command groups to
> --list-cmds. Allow the user to execute just one "git" process and get
> multiple groups. This may matter for git-completion.bash on Windows
> because we don't want the user to way for long when TAB-ing and

s/way/wait/

> Windows is slow on launching new processes.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

[PATCHv3 7/9] packfile: add repository argument to unpack_entry

[Stefan Beller]

Add a repository argument to allow the callers of unpack_entry
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 fast-import.c | 2 +-
 pack-check.c  | 3 ++-
 packfile.c| 7 ---
 packfile.h| 3 ++-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index afe06bd7c1..b009353e93 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1376,7 +1376,7 @@ static void *gfi_unpack_entry(
 */
p->pack_size = pack_size + the_hash_algo->rawsz;
}
-   return unpack_entry(p, oe->idx.offset, , sizep);
+   return unpack_entry(the_repository, p, oe->idx.offset, , sizep);
 }
 
 static const char *get_mode(const char *str, uint16_t *modep)
diff --git a/pack-check.c b/pack-check.c
index 385d964bdd..d3a57df34f 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "repository.h"
 #include "pack.h"
 #include "pack-revindex.h"
 #include "progress.h"
@@ -134,7 +135,7 @@ static int verify_packfile(struct packed_git *p,
data = NULL;
data_valid = 0;
} else {
-   data = unpack_entry(p, entries[i].offset, , );
+   data = unpack_entry(the_repository, p, 
entries[i].offset, , );
data_valid = 1;
}
 
diff --git a/packfile.c b/packfile.c
index 2876e04bb1..d5ac48ef18 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1279,7 +1279,7 @@ static void *cache_or_unpack_entry(struct packed_git *p, 
off_t base_offset,
 
ent = get_delta_base_cache_entry(p, base_offset);
if (!ent)
-   return unpack_entry(p, base_offset, type, base_size);
+   return unpack_entry(the_repository, p, base_offset, type, 
base_size);
 
if (type)
*type = ent->type;
@@ -1485,8 +1485,9 @@ static void *read_object_the_repository(const struct 
object_id *oid,
return content;
 }
 
-void *unpack_entry(struct packed_git *p, off_t obj_offset,
-  enum object_type *final_type, unsigned long *final_size)
+void *unpack_entry_the_repository(struct packed_git *p, off_t obj_offset,
+ enum object_type *final_type,
+ unsigned long *final_size)
 {
struct pack_window *w_curs = NULL;
off_t curpos = obj_offset;
diff --git a/packfile.h b/packfile.h
index bc8d840b1b..1efa57a90e 100644
--- a/packfile.h
+++ b/packfile.h
@@ -115,7 +115,8 @@ extern off_t nth_packed_object_offset(const struct 
packed_git *, uint32_t n);
 extern off_t find_pack_entry_one(const unsigned char *sha1, struct packed_git 
*);
 
 extern int is_pack_valid(struct packed_git *);
-extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, 
unsigned long *);
+#define unpack_entry(r, p, of, ot, s) unpack_entry_##r(p, of, ot, s)
+extern void *unpack_entry_the_repository(struct packed_git *, off_t, enum 
object_type *, unsigned long *);
 extern unsigned long unpack_object_header_buffer(const unsigned char *buf, 
unsigned long len, enum object_type *type, unsigned long *sizep);
 extern unsigned long get_size_from_delta(struct packed_git *, struct 
pack_window **, off_t);
 extern int unpack_object_header(struct packed_git *, struct pack_window **, 
off_t *, unsigned long *);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCHv3 8/9] packfile: add repository argument to cache_or_unpack_entry

[Stefan Beller]

Add a repository argument to allow the callers of cache_or_unpack_entry
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 packfile.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/packfile.c b/packfile.c
index d5ac48ef18..8de87f904b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1272,7 +1272,8 @@ static void detach_delta_base_cache_entry(struct 
delta_base_cache_entry *ent)
free(ent);
 }
 
-static void *cache_or_unpack_entry(struct packed_git *p, off_t base_offset,
+#define cache_or_unpack_entry(r, p, bo, bs, t) cache_or_unpack_entry_##r(p, 
bo, bs, t)
+static void *cache_or_unpack_entry_the_repository(struct packed_git *p, off_t 
base_offset,
unsigned long *base_size, enum object_type *type)
 {
struct delta_base_cache_entry *ent;
@@ -1346,7 +1347,7 @@ int packed_object_info_the_repository(struct packed_git 
*p, off_t obj_offset,
 * a "real" type later if the caller is interested.
 */
if (oi->contentp) {
-   *oi->contentp = cache_or_unpack_entry(p, obj_offset, oi->sizep,
+   *oi->contentp = cache_or_unpack_entry(the_repository, p, 
obj_offset, oi->sizep,
  );
if (!*oi->contentp)
type = OBJ_BAD;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCHv3 4/9] packfile: add repository argument to packed_to_object_type

[Stefan Beller]

Add a repository argument to allow the callers of packed_to_object_type
to be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 packfile.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/packfile.c b/packfile.c
index d2b3f3503b..3ecfba66af 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1124,11 +1124,13 @@ static int 
retry_bad_packed_offset_the_repository(struct packed_git *p, off_t ob
 
 #define POI_STACK_PREALLOC 64
 
-static enum object_type packed_to_object_type(struct packed_git *p,
- off_t obj_offset,
- enum object_type type,
- struct pack_window **w_curs,
- off_t curpos)
+#define packed_to_object_type(r, p, o, t, w, c) \
+   packed_to_object_type_##r(p, o, t, w, c)
+static enum object_type packed_to_object_type_the_repository(struct packed_git 
*p,
+off_t obj_offset,
+enum object_type 
type,
+struct pack_window 
**w_curs,
+off_t curpos)
 {
off_t small_poi_stack[POI_STACK_PREALLOC];
off_t *poi_stack = small_poi_stack;
@@ -1378,8 +1380,8 @@ int packed_object_info(struct packed_git *p, off_t 
obj_offset,
 
if (oi->typep || oi->type_name) {
enum object_type ptot;
-   ptot = packed_to_object_type(p, obj_offset, type, _curs,
-curpos);
+   ptot = packed_to_object_type(the_repository, p, obj_offset,
+type, _curs, curpos);
if (oi->typep)
*oi->typep = ptot;
if (oi->type_name) {
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCHv3 5/9] packfile: add repository argument to packed_object_info

[Stefan Beller]

From: Jonathan Nieder 

Add a repository argument to allow callers of packed_object_info to be
more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 builtin/pack-objects.c | 3 ++-
 packfile.c | 4 ++--
 packfile.h | 3 ++-
 sha1_file.c| 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8d4111f748..d65eb4a947 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1572,7 +1572,8 @@ static void drop_reused_delta(struct object_entry *entry)
 
oi.sizep = >size;
oi.typep = >type;
-   if (packed_object_info(entry->in_pack, entry->in_pack_offset, ) < 0) 
{
+   if (packed_object_info(the_repository, entry->in_pack,
+  entry->in_pack_offset, ) < 0) {
/*
 * We failed to get the info from this pack for some reason;
 * fall back to sha1_object_info, which may find another copy.
diff --git a/packfile.c b/packfile.c
index 3ecfba66af..5fa7d27d3b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1333,8 +1333,8 @@ static void add_delta_base_cache(struct packed_git *p, 
off_t base_offset,
hashmap_add(_base_cache, ent);
 }
 
-int packed_object_info(struct packed_git *p, off_t obj_offset,
-  struct object_info *oi)
+int packed_object_info_the_repository(struct packed_git *p, off_t obj_offset,
+ struct object_info *oi)
 {
struct pack_window *w_curs = NULL;
unsigned long size;
diff --git a/packfile.h b/packfile.h
index a92c0b241c..bc8d840b1b 100644
--- a/packfile.h
+++ b/packfile.h
@@ -125,7 +125,8 @@ extern void release_pack_memory(size_t);
 /* global flag to enable extra checks when accessing packed objects */
 extern int do_check_packed_object_crc;
 
-extern int packed_object_info(struct packed_git *pack, off_t offset, struct 
object_info *);
+#define packed_object_info(r, p, o, oi) packed_object_info_##r(p, o, oi)
+extern int packed_object_info_the_repository(struct packed_git *pack, off_t 
offset, struct object_info *);
 
 extern void mark_bad_packed_object(struct packed_git *p, const unsigned char 
*sha1);
 extern const struct packed_git *has_packed_and_bad(const unsigned char *sha1);
diff --git a/sha1_file.c b/sha1_file.c
index 93f25c6c6a..746ff8297a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1307,7 +1307,7 @@ int oid_object_info_extended_the_repository(const struct 
object_id *oid, struct
 * information below, so return early.
 */
return 0;
-   rtype = packed_object_info(e.p, e.offset, oi);
+   rtype = packed_object_info(the_repository, e.p, e.offset, oi);
if (rtype < 0) {
mark_bad_packed_object(e.p, real->hash);
return oid_object_info_extended(the_repository, real, oi, 0);
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH v4/wip 10/12] help: use command-list.txt for the source of guides

[Eric Sunshine]

On Wed, Apr 25, 2018 at 12:31 PM, Nguyễn Thái Ngọc Duy
 wrote:
> The help command currently hard codes the list of guides and their
> summary in C. Let's move this list to command-list.txt. This lets us
> extract summary lines from Documentation/git*.txt. This also
> potentially lets us lists guides in git.txt, but I'll leave that for

s/lists/list/

> now.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

[PATCHv3 2/9] cache.h: add repository argument to oid_object_info

[Stefan Beller]

Add a repository argument to allow the callers of oid_object_info
to be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 archive-tar.c|  2 +-
 archive-zip.c|  3 ++-
 blame.c  |  4 ++--
 builtin/blame.c  |  2 +-
 builtin/cat-file.c   |  6 +++---
 builtin/describe.c   |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fetch.c  |  2 +-
 builtin/fsck.c   |  3 ++-
 builtin/index-pack.c |  4 ++--
 builtin/ls-tree.c|  2 +-
 builtin/mktree.c |  2 +-
 builtin/pack-objects.c   |  8 +---
 builtin/prune.c  |  3 ++-
 builtin/replace.c| 11 ++-
 builtin/tag.c|  4 ++--
 builtin/unpack-objects.c |  2 +-
 cache.h  |  3 ++-
 diff.c   |  3 ++-
 fast-import.c| 14 +-
 list-objects-filter.c|  2 +-
 object.c |  2 +-
 pack-bitmap-write.c  |  3 ++-
 packfile.c   |  2 +-
 reachable.c  |  2 +-
 refs.c   |  2 +-
 remote.c |  2 +-
 sequencer.c  |  3 ++-
 sha1_file.c  |  4 ++--
 sha1_name.c  | 12 ++--
 submodule.c  |  2 +-
 tag.c|  2 +-
 32 files changed, 67 insertions(+), 53 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 3563bcb9f2..f93409324f 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -276,7 +276,7 @@ static int write_tar_entry(struct archiver_args *args,
memcpy(header.name, path, pathlen);
 
if (S_ISREG(mode) && !args->convert &&
-   oid_object_info(oid, ) == OBJ_BLOB &&
+   oid_object_info(the_repository, oid, ) == OBJ_BLOB &&
size > big_file_threshold)
buffer = NULL;
else if (S_ISLNK(mode) || S_ISREG(mode)) {
diff --git a/archive-zip.c b/archive-zip.c
index 6b20bce4d1..74f3fe9103 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -325,7 +325,8 @@ static int write_zip_entry(struct archiver_args *args,
compressed_size = 0;
buffer = NULL;
} else if (S_ISREG(mode) || S_ISLNK(mode)) {
-   enum object_type type = oid_object_info(oid, );
+   enum object_type type = oid_object_info(the_repository, oid,
+   );
 
method = 0;
attr2 = S_ISLNK(mode) ? ((mode | 0777) << 16) :
diff --git a/blame.c b/blame.c
index 78c9808bd1..dfa24473dc 100644
--- a/blame.c
+++ b/blame.c
@@ -81,7 +81,7 @@ static void verify_working_tree_path(struct commit 
*work_tree, const char *path)
unsigned mode;
 
if (!get_tree_entry(commit_oid, path, _oid, ) &&
-   oid_object_info(_oid, NULL) == OBJ_BLOB)
+   oid_object_info(the_repository, _oid, NULL) == 
OBJ_BLOB)
return;
}
 
@@ -504,7 +504,7 @@ static int fill_blob_sha1_and_mode(struct blame_origin 
*origin)
return 0;
if (get_tree_entry(>commit->object.oid, origin->path, 
>blob_oid, >mode))
goto error_out;
-   if (oid_object_info(>blob_oid, NULL) != OBJ_BLOB)
+   if (oid_object_info(the_repository, >blob_oid, NULL) != 
OBJ_BLOB)
goto error_out;
return 0;
  error_out:
diff --git a/builtin/blame.c b/builtin/blame.c
index db38c0b307..bfdf7cc132 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -655,7 +655,7 @@ static int is_a_rev(const char *name)
 
if (get_oid(name, ))
return 0;
-   return OBJ_NONE < oid_object_info(, NULL);
+   return OBJ_NONE < oid_object_info(the_repository, , NULL);
 }
 
 int cmd_blame(int argc, const char **argv, const char *prefix)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 4ecdb9ff54..b8ecbea98e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -116,7 +116,7 @@ static int cat_one_file(int opt, const char *exp_type, 
const char *obj_name,
/* else fallthrough */
 
case 'p':
-   type = oid_object_info(, NULL);
+   type = oid_object_info(the_repository, , NULL);
if (type < 0)
die("Not a valid object name %s", obj_name);
 
@@ -140,7 +140,7 @@ static int cat_one_file(int opt, const char *exp_type, 
const char *obj_name,
case 0:
if (type_from_string(exp_type) == OBJ_BLOB) {
struct object_id blob_oid;
-   if (oid_object_info(, NULL) == OBJ_TAG) {
+   if (oid_object_info(the_repository, , NULL) == 
OBJ_TAG) {

[PATCHv3 9/9] cache.h: allow oid_object_info to handle arbitrary repositories

[Stefan Beller]

This involves also adapting oid_object_info_extended and a some
internal functions that are used to implement these. It all has to
happen in one patch, because of a single recursive chain of calls visits
all these functions.

oid_object_info_extended is also used in partial clones, which allow
fetching missing objects. As this series will not add the repository
struct to the transport code and fetch_object(), add a TODO note and
omit fetching if a user tries to use a partial clone in a repository
other than the_repository.

Among the functions modified to handle arbitrary repositories,
unpack_entry() is one of them. Note that it still references the globals
"delta_base_cache" and "delta_base_cached", but those are safe to be
referenced (the former is indexed partly by "struct packed_git *", which
is repo-specific, and the latter is only used to limit the size of the
former as an optimization).

Helped-by: Brandon Williams 
Helped-by: Jonathan Tan 
Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 cache.h |  9 -
 packfile.c  | 58 ++---
 packfile.h  |  8 
 sha1_file.c | 31 
 4 files changed, 54 insertions(+), 52 deletions(-)

diff --git a/cache.h b/cache.h
index 6340b2c572..3a4d80e92b 100644
--- a/cache.h
+++ b/cache.h
@@ -1192,8 +1192,7 @@ static inline void *read_object_file(const struct 
object_id *oid, enum object_ty
 }
 
 /* Read and unpack an object file into memory, write memory to an object file 
*/
-#define oid_object_info(r, o, f) oid_object_info_##r(o, f)
-int oid_object_info_the_repository(const struct object_id *, unsigned long *);
+int oid_object_info(struct repository *r, const struct object_id *, unsigned 
long *);
 
 extern int hash_object_file(const void *buf, unsigned long len,
const char *type, struct object_id *oid);
@@ -1675,9 +1674,9 @@ struct object_info {
 /* Do not check loose object */
 #define OBJECT_INFO_IGNORE_LOOSE 16
 
-#define oid_object_info_extended(r, oid, oi, flags) \
-   oid_object_info_extended_##r(oid, oi, flags)
-int oid_object_info_extended_the_repository(const struct object_id *, struct 
object_info *, unsigned flags);
+int oid_object_info_extended(struct repository *r,
+const struct object_id *,
+struct object_info *, unsigned flags);
 
 /*
  * Set this to 0 to prevent sha1_object_info_extended() from fetching missing
diff --git a/packfile.c b/packfile.c
index 8de87f904b..55d383ed0a 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1104,9 +1104,9 @@ static const unsigned char *get_delta_base_sha1(struct 
packed_git *p,
return NULL;
 }
 
-#define retry_bad_packed_offset(r, p, o) \
-   retry_bad_packed_offset_##r(p, o)
-static int retry_bad_packed_offset_the_repository(struct packed_git *p, off_t 
obj_offset)
+static int retry_bad_packed_offset(struct repository *r,
+  struct packed_git *p,
+  off_t obj_offset)
 {
int type;
struct revindex_entry *revidx;
@@ -1116,7 +1116,7 @@ static int retry_bad_packed_offset_the_repository(struct 
packed_git *p, off_t ob
return OBJ_BAD;
nth_packed_object_oid(, p, revidx->nr);
mark_bad_packed_object(p, oid.hash);
-   type = oid_object_info(the_repository, , NULL);
+   type = oid_object_info(r, , NULL);
if (type <= OBJ_NONE)
return OBJ_BAD;
return type;
@@ -1124,13 +1124,12 @@ static int 
retry_bad_packed_offset_the_repository(struct packed_git *p, off_t ob
 
 #define POI_STACK_PREALLOC 64
 
-#define packed_to_object_type(r, p, o, t, w, c) \
-   packed_to_object_type_##r(p, o, t, w, c)
-static enum object_type packed_to_object_type_the_repository(struct packed_git 
*p,
-off_t obj_offset,
-enum object_type 
type,
-struct pack_window 
**w_curs,
-off_t curpos)
+static enum object_type packed_to_object_type(struct repository *r,
+ struct packed_git *p,
+ off_t obj_offset,
+ enum object_type type,
+ struct pack_window **w_curs,
+ off_t curpos)
 {
off_t small_poi_stack[POI_STACK_PREALLOC];
off_t *poi_stack = small_poi_stack;
@@ -1157,7 +1156,7 @@ static enum object_type 
packed_to_object_type_the_repository(struct packed_git *
if (type <= OBJ_NONE) {
/* If getting the base itself 

[PATCHv3 6/9] packfile: add repository argument to read_object

[Stefan Beller]

Add a repository argument to allow the callers of read_object
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Stefan Beller 
---
 packfile.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/packfile.c b/packfile.c
index 5fa7d27d3b..2876e04bb1 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1469,8 +1469,10 @@ struct unpack_entry_stack_ent {
unsigned long size;
 };
 
-static void *read_object(const struct object_id *oid, enum object_type *type,
-unsigned long *size)
+#define read_object(r, o, t, s) read_object_##r(o, t, s)
+static void *read_object_the_repository(const struct object_id *oid,
+   enum object_type *type,
+   unsigned long *size)
 {
struct object_info oi = OBJECT_INFO_INIT;
void *content;
@@ -1614,7 +1616,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
  oid_to_hex(_oid), 
(uintmax_t)obj_offset,
  p->pack_name);
mark_bad_packed_object(p, base_oid.hash);
-   base = read_object(_oid, , 
_size);
+   base = read_object(the_repository, _oid, 
, _size);
external_base = base;
}
}
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCHv3 3/9] packfile: add repository argument to retry_bad_packed_offset

[Stefan Beller]

Add a repository argument to allow the callers of retry_bad_packed_offset
to be more specific about which repository to handle. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

As with the previous commits, use a macro to catch callers passing a
repository other than the_repository at compile time.

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 packfile.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/packfile.c b/packfile.c
index 80c7fa734f..d2b3f3503b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1104,7 +1104,9 @@ static const unsigned char *get_delta_base_sha1(struct 
packed_git *p,
return NULL;
 }
 
-static int retry_bad_packed_offset(struct packed_git *p, off_t obj_offset)
+#define retry_bad_packed_offset(r, p, o) \
+   retry_bad_packed_offset_##r(p, o)
+static int retry_bad_packed_offset_the_repository(struct packed_git *p, off_t 
obj_offset)
 {
int type;
struct revindex_entry *revidx;
@@ -1153,7 +1155,7 @@ static enum object_type packed_to_object_type(struct 
packed_git *p,
if (type <= OBJ_NONE) {
/* If getting the base itself fails, we first
 * retry the base, otherwise unwind */
-   type = retry_bad_packed_offset(p, base_offset);
+   type = retry_bad_packed_offset(the_repository, p, 
base_offset);
if (type > OBJ_NONE)
goto out;
goto unwind;
@@ -1181,7 +1183,7 @@ static enum object_type packed_to_object_type(struct 
packed_git *p,
 unwind:
while (poi_stack_nr) {
obj_offset = poi_stack[--poi_stack_nr];
-   type = retry_bad_packed_offset(p, obj_offset);
+   type = retry_bad_packed_offset(the_repository, p, obj_offset);
if (type > OBJ_NONE)
goto out;
}
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCHv3 0/9] object store: oid_object_info is the next contender

[Stefan Beller]

v3:
* fixed and extended the commit message of last commit
* fixed the last patch, as Jonathan Tan suggested, see interdiff:

$ git diff remotes/origin/sb/oid-object-info (which is v2)
diff --git c/sha1_file.c w/sha1_file.c
index 94123e0299..dcd6b879ac 100644
--- c/sha1_file.c
+++ w/sha1_file.c
@@ -1289,14 +1289,13 @@ int oid_object_info_extended(struct repository *r, 
const struct object_id *oid,
 
/* Check if it is a missing object */
if (fetch_if_missing && repository_format_partial_clone &&
-   !already_retried) {
+   !already_retried && r == the_repository) {
/*
 * TODO Investigate having fetch_object() return
 * TODO error/success and stopping the music here.
-* TODO Pass a repository struct through 
fetch_object.
+* TODO Pass a repository struct through 
fetch_object,
+* such that arbitrary repositories work.
 */
-   if (r != the_repository)
-   die(_("partial clones only supported in 
the_repository"));
fetch_object(repository_format_partial_clone, 
real->hash);
already_retried = 1;
continue;

Thanks,
Stefan

v2:

* fixed the sha1/oid typo
* removed spurious new line
* Brandon and Jonthan discovered another dependency that I missed due
  to cherrypicking that commit from a tree before partial clone was a thing.
  We error out when attempting to use fetch_object for repos that are not
  the_repository.

Thanks,
Stefan

v1:
This applies on top of origin/sb/object-store-replace and is available as
https://github.com/stefanbeller/git/tree/oid_object_info

This continues the work of sb/packfiles-in-repository,
extending the layer at which we have to pass in an explicit
repository object to oid_object_info.

A test merge to next shows only a minor merge conflicit (adding
different #include lines in one c file), so this might be a good next
step for the object store series.

Notes on further object store series:
I plan on converting the "parsed object store" next,
which would be {alloc, object, tree, commit, tag}.c as that is a prerequisite
for migrating shallow (which is intermingled with grafts) information to the
object store.

There is currently work going on in allocation (mempool - Jameson Miller)
and grafts (deprecate grafts - DScho), which is why I am sending this
series first. I think it can go in parallel to the "parsed object store"
that is coming next.

Thanks,
Stefan

Jonathan Nieder (1):
  packfile: add repository argument to packed_object_info

Stefan Beller (8):
  cache.h: add repository argument to oid_object_info_extended
  cache.h: add repository argument to oid_object_info
  packfile: add repository argument to retry_bad_packed_offset
  packfile: add repository argument to packed_to_object_type
  packfile: add repository argument to read_object
  packfile: add repository argument to unpack_entry
  packfile: add repository argument to cache_or_unpack_entry
  cache.h: allow oid_object_info to handle arbitrary repositories

 archive-tar.c|  2 +-
 archive-zip.c|  3 ++-
 blame.c  |  4 ++--
 builtin/blame.c  |  2 +-
 builtin/cat-file.c   | 12 ++--
 builtin/describe.c   |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/fetch.c  |  2 +-
 builtin/fsck.c   |  3 ++-
 builtin/index-pack.c |  4 ++--
 builtin/ls-tree.c|  2 +-
 builtin/mktree.c |  2 +-
 builtin/pack-objects.c   | 11 +++
 builtin/prune.c  |  3 ++-
 builtin/replace.c| 11 ++-
 builtin/tag.c|  4 ++--
 builtin/unpack-objects.c |  2 +-
 cache.h  |  7 +--
 diff.c   |  3 ++-
 fast-import.c| 16 ++--
 list-objects-filter.c|  2 +-
 object.c |  2 +-
 pack-bitmap-write.c  |  3 ++-
 pack-check.c |  3 ++-
 packfile.c   | 40 +++-
 packfile.h   |  6 --
 reachable.c  |  2 +-
 refs.c   |  2 +-
 remote.c |  2 +-
 sequencer.c  |  3 ++-
 sha1_file.c  | 37 +
 sha1_name.c  | 12 ++--
 streaming.c  |  2 +-
 submodule.c  |  2 +-
 tag.c|  2 +-
 35 files changed, 124 insertions(+), 93 deletions(-)

-- 
2.17.0.441.gb46fe60e1d-goog

[PATCHv3 1/9] cache.h: add repository argument to oid_object_info_extended

[Stefan Beller]

Add a repository argument to allow oid_object_info_extended callers
to be more specific about which repository to act on. This is a small
mechanical change; it doesn't change the implementation to handle
repositories other than the_repository yet.

Signed-off-by: Stefan Beller 
---
 builtin/cat-file.c |  6 +++---
 cache.h|  5 -
 packfile.c |  2 +-
 sha1_file.c| 10 +-
 streaming.c|  2 +-
 5 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 2c46d257cd..4ecdb9ff54 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -77,7 +77,7 @@ static int cat_one_file(int opt, const char *exp_type, const 
char *obj_name,
switch (opt) {
case 't':
oi.type_name = 
-   if (oid_object_info_extended(, , flags) < 0)
+   if (oid_object_info_extended(the_repository, , , flags) 
< 0)
die("git cat-file: could not get object info");
if (sb.len) {
printf("%s\n", sb.buf);
@@ -88,7 +88,7 @@ static int cat_one_file(int opt, const char *exp_type, const 
char *obj_name,
 
case 's':
oi.sizep = 
-   if (oid_object_info_extended(, , flags) < 0)
+   if (oid_object_info_extended(the_repository, , , flags) 
< 0)
die("git cat-file: could not get object info");
printf("%lu\n", size);
return 0;
@@ -342,7 +342,7 @@ static void batch_object_write(const char *obj_name, struct 
batch_options *opt,
struct strbuf buf = STRBUF_INIT;
 
if (!data->skip_object_info &&
-   oid_object_info_extended(>oid, >info,
+   oid_object_info_extended(the_repository, >oid, >info,
 OBJECT_INFO_LOOKUP_REPLACE) < 0) {
printf("%s missing\n",
   obj_name ? obj_name : oid_to_hex(>oid));
diff --git a/cache.h b/cache.h
index 027bd7ffc8..588c4fff9a 100644
--- a/cache.h
+++ b/cache.h
@@ -1673,7 +1673,10 @@ struct object_info {
 #define OBJECT_INFO_QUICK 8
 /* Do not check loose object */
 #define OBJECT_INFO_IGNORE_LOOSE 16
-extern int oid_object_info_extended(const struct object_id *, struct 
object_info *, unsigned flags);
+
+#define oid_object_info_extended(r, oid, oi, flags) \
+   oid_object_info_extended_##r(oid, oi, flags)
+int oid_object_info_extended_the_repository(const struct object_id *, struct 
object_info *, unsigned flags);
 
 /*
  * Set this to 0 to prevent sha1_object_info_extended() from fetching missing
diff --git a/packfile.c b/packfile.c
index 0bc67d0e00..d9914ba723 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1474,7 +1474,7 @@ static void *read_object(const struct object_id *oid, 
enum object_type *type,
oi.sizep = size;
oi.contentp = 
 
-   if (oid_object_info_extended(oid, , 0) < 0)
+   if (oid_object_info_extended(the_repository, oid, , 0) < 0)
return NULL;
return content;
 }
diff --git a/sha1_file.c b/sha1_file.c
index 64a5bd7d87..50a2dc5f0a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1231,7 +1231,7 @@ static int sha1_loose_object_info(struct repository *r,
 
 int fetch_if_missing = 1;
 
-int oid_object_info_extended(const struct object_id *oid, struct object_info 
*oi, unsigned flags)
+int oid_object_info_extended_the_repository(const struct object_id *oid, 
struct object_info *oi, unsigned flags)
 {
static struct object_info blank_oi = OBJECT_INFO_INIT;
struct pack_entry e;
@@ -1310,7 +1310,7 @@ int oid_object_info_extended(const struct object_id *oid, 
struct object_info *oi
rtype = packed_object_info(e.p, e.offset, oi);
if (rtype < 0) {
mark_bad_packed_object(e.p, real->hash);
-   return oid_object_info_extended(real, oi, 0);
+   return oid_object_info_extended(the_repository, real, oi, 0);
} else if (oi->whence == OI_PACKED) {
oi->u.packed.offset = e.offset;
oi->u.packed.pack = e.p;
@@ -1329,7 +1329,7 @@ int oid_object_info(const struct object_id *oid, unsigned 
long *sizep)
 
oi.typep = 
oi.sizep = sizep;
-   if (oid_object_info_extended(oid, ,
+   if (oid_object_info_extended(the_repository, oid, ,
 OBJECT_INFO_LOOKUP_REPLACE) < 0)
return -1;
return type;
@@ -1347,7 +1347,7 @@ static void *read_object(const unsigned char *sha1, enum 
object_type *type,
 
hashcpy(oid.hash, sha1);
 
-   if (oid_object_info_extended(, , 0) < 0)
+   if (oid_object_info_extended(the_repository, , , 0) < 0)
return NULL;
return content;
 }
@@ -1745,7 +1745,7 @@ int has_sha1_file_with_flags(const unsigned char *sha1, 
int flags)
if (!startup_info->have_repository)
return 0;
hashcpy(oid.hash, sha1);
-  

Re: [PATCH v4/wip 08/12] git: support --list-cmds=

[Eric Sunshine]

On Wed, Apr 25, 2018 at 12:31 PM, Nguyễn Thái Ngọc Duy
 wrote:
> This allows us to select any group of commands by a category defined
> in command-list.txt. This is an internal/hidden option so we don't
> have to be picky about the category name or worried about exposing too
> much.
>
> This will be used later by git-completion.bash to retrieve certain
> command groups.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/help.c b/help.c
> @@ -305,6 +305,25 @@ void list_all_cmds(void)
> +void list_cmds_by_category(const char *cat)
> +{
> +   int i;
> +   int cat_id = 0;

Should 'cat_id' be unsigned...

> +
> +   for (i = 0; category_names[i]; i++) {
> +   if (!strcmp(cat, category_names[i])) {
> +   cat_id = 1 << i;

...since you're shifting it here?

> +   break;
> +   }
> +   }
> +   if (!cat_id)
> +   die("unsupported command listing type '%s'", cat);
> +
> +   for (i = 0; command_list[i].name; i++)
> +   if (command_list[i].category & cat_id)
> +   puts(command_list[i].name);
> +}

Re: [PATCH v4/wip 01/12] generate-cmds.sh: factor out synopsis extract code

[SZEDER Gábor]

On Wed, Apr 25, 2018 at 7:59 PM Eric Sunshine 
wrote:

> On Wed, Apr 25, 2018 at 12:30 PM, Nguyễn Thái Ngọc Duy
>  wrote:
> > This makes it easier to reuse the same code in another place (very
> > soon).
> >
> > Signed-off-by: Nguyễn Thái Ngọc Duy 
> > ---
> > diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> > @@ -1,5 +1,17 @@
> > +get_synopsis () {
> > +   local cmd="$1"

> 'local' is a Bash-ism, isn't it?

Well, strictly speaking it isn't, because many shells support it
besides Bash.

I don't remember seeing any complaints about 01d3a526ad (t: check
whether the shell supports the "local" keyword, 2017-10-26), but we
only have that commit for a couple or months / two releases, so it's
still too early to start using 'local' in build scripts.

Re: [PATCH v4/wip 02/12] generate-cmds.sh: export all commands to command-list.h

[Eric Sunshine]

On Wed, Apr 25, 2018 at 12:30 PM, Nguyễn Thái Ngọc Duy
 wrote:
> The current generate-cmds.sh generates just enough to print "git help"
> output. That is, it only extracts help text for common commands.
>
> The script is now updated to extract help text for all commands and
> keep command classification a new file, command-list.h. This will be
> useful later:
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> @@ -12,14 +34,51 @@ get_synopsis () {
> +define_categories() {
> +   echo
> +   echo "/* Command categories */"
> +   bit=0
> +   category_list "$1" |
> +   while read cat
> +   do
> +   echo "#define CAT_$cat (1UL << $bit)"
> +   bit=$(($bit+1))
> +   done
> +   test "$bit" -gt 32 && die "Urgh.. too many categories?"

Should this be '-ge' rather than '-gt'?

> +}

Re: [PATCH v4/wip 01/12] generate-cmds.sh: factor out synopsis extract code

[Eric Sunshine]

On Wed, Apr 25, 2018 at 12:30 PM, Nguyễn Thái Ngọc Duy
 wrote:
> This makes it easier to reuse the same code in another place (very
> soon).
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> @@ -1,5 +1,17 @@
> +get_synopsis () {
> +   local cmd="$1"

'local' is a Bash-ism, isn't it?

> +   sed -n '
> +   /^NAME/,/'"$cmd"'/H
> +   ${
> +   x
> +   s/.*'"$cmd"' - \(.*\)/N_("\1")/
> +   p
> +   }' "Documentation/$cmd.txt"
> +}
> +
>  echo "/* Automatically generated by generate-cmdlist.sh */
>  struct cmdname_help {
> char name[16];
> @@ -39,12 +51,6 @@ sort |
>  while read cmd tags
>  do
> tag=$(echo "$tags" | sed "$substnum; s/[^0-9]//g")
> -   sed -n '
> -   /^NAME/,/git-'"$cmd"'/H
> -   ${
> -   x
> -   s/.*git-'"$cmd"' - \(.*\)/  {"'"$cmd"'", 
> N_("\1"), '$tag'},/
> -   p
> -   }' "Documentation/git-$cmd.txt"
> +   echo "  {\"$cmd\", $(get_synopsis git-$cmd), $tag},"
>  done
>  echo "};"

Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[Brandon Williams]

On 04/25, Ævar Arnfjörð Bjarmason wrote:
> > * bw/protocol-v2 (2018-03-15) 35 commits
> >   (merged to 'next' on 2018-04-11 at 23ee234a2c)
> >  + remote-curl: don't request v2 when pushing
> >  + remote-curl: implement stateless-connect command
> >  + http: eliminate "# service" line when using protocol v2
> >  + http: don't always add Git-Protocol header
> >  + http: allow providing extra headers for http requests
> >  + remote-curl: store the protocol version the server responded with
> >  + remote-curl: create copy of the service name
> >  + pkt-line: add packet_buf_write_len function
> >  + transport-helper: introduce stateless-connect
> >  + transport-helper: refactor process_connect_service
> >  + transport-helper: remove name parameter
> >  + connect: don't request v2 when pushing
> >  + connect: refactor git_connect to only get the protocol version once
> >  + fetch-pack: support shallow requests
> >  + fetch-pack: perform a fetch using v2
> >  + upload-pack: introduce fetch server command
> >  + push: pass ref prefixes when pushing
> >  + fetch: pass ref prefixes when fetching
> >  + ls-remote: pass ref prefixes when requesting a remote's refs
> >  + transport: convert transport_get_remote_refs to take a list of ref 
> > prefixes
> >  + transport: convert get_refs_list to take a list of ref prefixes
> >  + connect: request remote refs using v2
> >  + ls-refs: introduce ls-refs server command
> >  + serve: introduce git-serve
> >  + test-pkt-line: introduce a packet-line test helper
> >  + protocol: introduce enum protocol_version value protocol_v2
> >  + transport: store protocol version
> >  + connect: discover protocol version outside of get_remote_heads
> >  + connect: convert get_remote_heads to use struct packet_reader
> >  + transport: use get_refs_via_connect to get refs
> >  + upload-pack: factor out processing lines
> >  + upload-pack: convert to a builtin
> >  + pkt-line: add delim packet support
> >  + pkt-line: allow peeking a packet line without consuming it
> >  + pkt-line: introduce packet_read_with_status
> >  (this branch is used by bw/server-options.)
> >
> >  The beginning of the next-gen transfer protocol.
> >
> >  Will cook in 'next'.
> 
> With a month & 10 days of no updates & this looking stable it would be
> great to have it in master sooner than later to build on top of it in
> the 2.18 window.

I personally think that this series is ready to graduate to master but I
mentioned to Junio off-list that it might be a good idea to wait until
it has undergone a little more stress testing.  We've been in the
process of trying to get this rolled out to our internal server but due
to a few roadblocks and people being out of the office its taken a bit
longer than I would have liked to get it up and running.  I'm hoping in
another few days/a week I'll have some more data on live traffic.  At
that point I think I'll be more convinced that it will be safe to merge it.

I may be overly cautions but I'm hoping that we can get this right
without needing to do another protocol version bump for a very long
time.  Technically using v2 is hidden behind an "experimental" config
flag so we should still be able to tweak it after the fact if we
absolutely needed to, but I'd like to avoid that if necessary.

-- 
Brandon Williams

Re: git https and github

[Beat Bolli]

On 25.04.18 02:32, Lev wrote:
> Hi list,
> 
> 
> I'm struggling with git connecting to Github.
> 
> The problem might be SSL/TLS related.
> 
> https://githubengineering.com/crypto-removal-notice/
> 
> I suspect that my setup still uses tlsv1 or tlsv1.1.
> 
> I've tried to explicitly set git to use tlsv1.2 in my .gitconfig file
> like this:
> 
> [http]
>   sslVersion = tlsv1.2

This is the default, so this setting should not be needed, unless it's
overridden in some higher prioritized git config file. Have you tried

git -c http.sslVersion=tlsv1.2 clone 

? This should override any settings files.

> I've tried to re-compile git with OpenSSL and GnuTLS. All give the
> same error.
> 
> git clone https://github.com/OnionIoT/source.git
> Cloning into 'source'...
> * Couldn't find host github.com in the .netrc file; using defaults
> *   Trying 192.30.253.112...
> * TCP_NODELAY set
> * Connected to github.com (192.30.253.112) port 443 (#0)
> * ALPN, offering http/1.1
> * Cipher selection:
> ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
> * successfully set certificate verify locations:
> *   CAfile: /etc/ssl/certs/ca-certificates.crt
>   CApath: /etc/ssl/certs
> * error:1409442E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol
> version
> * Curl_http_done: called premature == 1
> * stopped the pause stream!
> * Closing connection 0
> fatal: unable to access 'https://github.com/OnionIoT/source.git/':
> error:1409442E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol
> version lev@jive:~/git$ unset GIT_SSL_VERSION lev@jive:~/git$ git clone
> https://github.com/OnionIoT/source.git Cloning into 'source'...
> * Couldn't find host github.com in the .netrc file; using defaults
> *   Trying 192.30.253.112...
> * TCP_NODELAY set
> * Connected to github.com (192.30.253.112) port 443 (#0)
> * ALPN, offering http/1.1
> * Cipher selection:
> ALL:!EXPORT:!EXPORT40:!EXPORT56:!aNULL:!LOW:!RC4:@STRENGTH
> * successfully set certificate verify locations:
> *   CAfile: /etc/ssl/certs/ca-certificates.crt
>   CApath: /etc/ssl/certs
> * error:1409442E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol
> version
> * Curl_http_done: called premature == 1
> * stopped the pause stream!
> * Closing connection 0
> fatal: unable to access 'https://github.com/OnionIoT/source.git/':
> error:1409442E:SSL routines:ssl3_read_bytes:tlsv1 alert protocol version
> 
> 
> I can connect to other git servers without any error. This is a debian
> stable system with the following components:
> 
>   git version 2.11.0
>   libcurl 7.52.1
>   OpenSSL 1.0.2l

This OpenSSL version is certainly recent enough to support TLSv1.2. Are
you sure you ran the newly compiled git binary?

(Sorry for asking stupid questions; it's sometimes difficult to get to
the root of a problem)

> 
> 
> Is there any way to know what is the exact protocol used? Are there any
> workaround, fix for this issue?
> 
> Any help welcome. Thank you,
> Levente
> 


Cheers,
Beat

Re: [PATCH v3 0/4] rebase -i: avoid stale "# This is a combinationof" in commit messages

[Phillip Wood]

On 25/04/18 13:48, Johannes Schindelin wrote:

Hi Phillip,

On Mon, 23 Apr 2018, Phillip Wood wrote:


On 23/04/18 19:11, Stefan Beller wrote:


On Sat, Apr 21, 2018 at 12:34 AM, Johannes Schindelin
 wrote:

Eric Sunshine pointed out that I had such a commit message in
https://public-inbox.org/git/CAPig+cRrS0_nYJJY=o6cbov630snqhpv5qgrqdd8mw-syzn...@mail.gmail.com/
and I went on a hunt to figure out how the heck this happened.

Turns out that if there is a fixup/squash chain where the *last* command
fails with merge conflicts, and we either --skip ahead or resolve the
conflict to a clean tree and then --continue, our code does not do a
final cleanup.

Contrary to my initial gut feeling, this bug was not introduced by my
rewrite in C of the core parts of rebase -i, but it looks to me as if
that bug was with us for a very long time (at least the --skip part).

The developer (read: user of rebase -i) in me says that we would want to
fast-track this, but the author of rebase -i in me says that we should
be cautious and cook this in `next` for a while.


I looked through the patches again and think this series is good to go.


I've just realized I commented on an outdated version as the new version was
posted there rather than as a reply to v1. I've just looked through it and I'm
not sure it addresses the unnecessary editing of the commit message of the
previous commit if a single squash command is skipped as outlined in
https://public-inbox.org/git/b6512eae-e214-9699-4d69-77117a0da...@talktalk.net/


I have not forgotten about this! I simply did not find the time yet, is
all...


I wondered if that was the case but I wanted to check as I wasn't sure 
if you'd seen the original message as it was on an obsolete version of 
the series



The patch series still has not been merged to `next`, but I plan on
working on your suggested changes as an add-on commit anyway. I am not
quite sure yet how I want to handle the "avoid running commit for the
first fixup/squash in the series" problem, but I think we will have to add
*yet another* file that is written (in the "we already have comments in
the commit message" conditional block in error_failed_squash())...


I wonder if creating the file in update_squash_messages() rather than 
error_failed_squash() would be a better approach as then it is easy to 
only create rebase_path_amend_type() when there has already been a 
squash or fixup. The file is removed in the loop that picks commits in 
pick_commits() so it would be cleaned up at the beginning of the next 
pick if it's not needed.


Best Wishes

Phillip



Ciao,
Dscho

[PATCH v4/wip 05/12] git.c: convert --list-*builtins to --list-cmds=*

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

Even if these are hidden options, let's make them a bit more generic
since we're introducing more listing types shortly.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash |  2 +-
 git.c  | 12 +++-
 t/t0012-help.sh|  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index a757073945..3556838759 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3049,7 +3049,7 @@ __git_complete_common () {
 __git_cmds_with_parseopt_helper=
 __git_support_parseopt_helper () {
test -n "$__git_cmds_with_parseopt_helper" ||
-   __git_cmds_with_parseopt_helper="$(__git 
--list-parseopt-builtins)"
+   __git_cmds_with_parseopt_helper="$(__git --list-cmds=parseopt)"
 
case " $__git_cmds_with_parseopt_helper " in
*" $1 "*)
diff --git a/git.c b/git.c
index 3a89893712..28bfa96d87 100644
--- a/git.c
+++ b/git.c
@@ -223,11 +223,13 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
}
(*argv)++;
(*argc)--;
-   } else if (!strcmp(cmd, "--list-builtins")) {
-   list_builtins(0, '\n');
-   exit(0);
-   } else if (!strcmp(cmd, "--list-parseopt-builtins")) {
-   list_builtins(NO_PARSEOPT, ' ');
+   } else if (skip_prefix(cmd, "--list-cmds=", )) {
+   if (!strcmp(cmd, "builtins"))
+   list_builtins(0, '\n');
+   else if (!strcmp(cmd, "parseopt"))
+   list_builtins(NO_PARSEOPT, ' ');
+   else
+   die("unsupported command listing type '%s'", 
cmd);
exit(0);
} else {
fprintf(stderr, _("unknown option: %s\n"), cmd);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 487b92a5de..fd2a7f27dc 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -50,7 +50,7 @@ test_expect_success "--help does not work for guides" "
 "
 
 test_expect_success 'generate builtin list' '
-   git --list-builtins >builtins
+   git --list-cmds=builtins >builtins
 '
 
 while read builtin
-- 
2.17.0.519.gb89679a4aa

[PATCH v4/wip 09/12] help: add "-a --verbose" to list all commands with synopsis

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

This lists all recognized commands [1] by category. The group order
follows closely git.txt.

[1] We may actually show commands that are not built (e.g. if you set
NO_PERL you don't have git-instaweb but it's still listed here). I
ignore the problem because on Linux a git package could be split
anyway. The "git-core" package may not contain git-instaweb even if
it's built because it may end up in a separate package. We can't know
anyway.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-help.txt |  4 +++-
 builtin/help.c |  7 +++
 help.c | 16 
 help.h |  1 +
 t/t0012-help.sh|  9 +
 5 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 40d328a4b3..a40fc38d8b 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 
 [verse]
-'git help' [-a|--all] [-g|--guide]
+'git help' [-a|--all [--verbose]] [-g|--guide]
   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION
@@ -42,6 +42,8 @@ OPTIONS
 --all::
Prints all the available commands on the standard output. This
option overrides any given command or guide name.
+   When used with `--verbose` print description for all recognized
+   commands.
 
 -g::
 --guides::
diff --git a/builtin/help.c b/builtin/help.c
index 598867cfea..0e0af8426a 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -36,6 +36,7 @@ static const char *html_path;
 
 static int show_all = 0;
 static int show_guides = 0;
+static int verbose;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
@@ -48,6 +49,7 @@ static struct option builtin_help_options[] = {
HELP_FORMAT_WEB),
OPT_SET_INT('i', "info", _format, N_("show info page"),
HELP_FORMAT_INFO),
+   OPT__VERBOSE(, N_("print command description")),
OPT_END(),
 };
 
@@ -463,6 +465,11 @@ int cmd_help(int argc, const char **argv, const char 
*prefix)
 
if (show_all) {
git_config(git_help_config, NULL);
+   if (verbose) {
+   setup_pager();
+   list_all_cmds_help();
+   return 0;
+   }
printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
load_command_list("git-", _cmds, _cmds);
list_commands(colopts, _cmds, _cmds);
diff --git a/help.c b/help.c
index affa8e4343..1f27d94661 100644
--- a/help.c
+++ b/help.c
@@ -27,6 +27,17 @@ static struct category_description common_categories[] = {
{ CAT_remote, N_("collaborate (see also: git help workflows)") },
{ 0, NULL }
 };
+static struct category_description main_categories[] = {
+   { CAT_mainporcelain, N_("Main Porcelain Commands") },
+   { CAT_ancillarymanipulators, N_("Ancillary Commands / Manipulators") },
+   { CAT_ancillaryinterrogators, N_("Ancillary Commands / Interrogators") 
},
+   { CAT_foreignscminterface, N_("Interacting with Others") },
+   { CAT_plumbingmanipulators, N_("Low-level Commands / Manipulators") },
+   { CAT_plumbinginterrogators, N_("Low-level Commands / Interrogators") },
+   { CAT_synchingrepositories, N_("Low-level Commands / Synching 
Repositories") },
+   { CAT_purehelpers, N_("Low-level Commands / Internal Helpers") },
+   { 0, NULL }
+};
 
 static void extract_cmds(struct cmdname_help **p_cmds, uint32_t mask)
 {
@@ -324,6 +335,11 @@ void list_cmds_by_category(const char *cat)
puts(command_list[i].name);
 }
 
+void list_all_cmds_help(void)
+{
+   print_cmd_by_category(main_categories);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index 3e30542927..118b7d9947 100644
--- a/help.h
+++ b/help.h
@@ -17,6 +17,7 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 extern void list_common_cmds_help(void);
+extern void list_all_cmds_help(void);
 extern void list_all_cmds(void);
 extern void list_cmds_by_category(const char *category);
 extern const char *help_unknown_cmd(const char *cmd);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index fd2a7f27dc..53208ab20e 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -25,6 +25,15 @@ test_expect_success "setup" '
EOF
 '
 
+# make sure to exercise these code paths, the output is a bit tricky
+# to verify
+test_expect_success 'basic help commands' '
+   git help >/dev/null &&
+   git help -a >/dev/null &&
+   git help -g >/dev/null &&
+   git help -av >/dev/null
+'
+
 test_expect_success "works for commands and guides by default" '
configure_help &&
git help status &&
-- 
2.17.0.519.gb89679a4aa

[PATCH v4/wip 11/12] command-list.txt: add new category "complete"

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

This category, combined with 'external' and 'mainporcelain', is
intended to replace the "porcelain command list" in git-completion.bash.
In other words, these are the commands that will show up by default
when you type "git ".

Compared to the current list in git-completion.bash (which is
basically "git help -a" with a black list), the following commands no
longer show up:

- annotate  obsolete, discouraged to use
- difftool-helper   not an end user command
- filter-branch not often used
- get-tar-commit-id not often used
- imap-send not often used
- instaweb  does anybody still run this interactively?
- interpreter-trailers not an interactive command
- lost-foundobsolete
- mergetool ???
- p4too short to complete?
- peek-remote   not often used??
- svn   ???
- tar-tree  obsolete, use archive instead
- verify-commit not often used

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 command-list.txt | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/command-list.txt b/command-list.txt
index a57bcb64a1..40b56c57f5 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -3,11 +3,11 @@
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
-git-apply   plumbingmanipulators
+git-apply   plumbingmanipulators
complete
 git-archimport  foreignscminterface
 git-archive mainporcelain
 git-bisect  mainporcelain   info
-git-blame   ancillaryinterrogators
+git-blame   ancillaryinterrogators  
complete
 git-branch  mainporcelain   history
 git-bundle  mainporcelain
 git-cat-fileplumbinginterrogators
@@ -17,7 +17,7 @@ git-check-mailmap   purehelpers
 git-checkoutmainporcelain   history
 git-checkout-index  plumbingmanipulators
 git-check-ref-formatpurehelpers
-git-cherry  ancillaryinterrogators
+git-cherry  ancillaryinterrogators  
complete
 git-cherry-pick mainporcelain
 git-citool  mainporcelain
 git-clean   mainporcelain
@@ -25,7 +25,7 @@ git-clone   mainporcelain 
  init
 git-column  purehelpers
 git-commit  mainporcelain   history
 git-commit-tree plumbingmanipulators
-git-config  ancillarymanipulators
+git-config  ancillarymanipulators   
complete
 git-count-objects   ancillaryinterrogators
 git-credential  purehelpers
 git-credential-cachepurehelpers
@@ -39,7 +39,7 @@ git-diffmainporcelain 
  history
 git-diff-files  plumbinginterrogators
 git-diff-index  plumbinginterrogators
 git-diff-tree   plumbinginterrogators
-git-difftoolancillaryinterrogators
+git-difftoolancillaryinterrogators  
complete
 git-fast-export ancillarymanipulators
 git-fast-import ancillarymanipulators
 git-fetch   mainporcelain   remote
@@ -48,13 +48,13 @@ git-filter-branch   
ancillarymanipulators
 git-fmt-merge-msg   purehelpers
 git-for-each-refplumbinginterrogators
 git-format-patchmainporcelain
-git-fsckancillaryinterrogators
+git-fsckancillaryinterrogators  
complete
 git-gc  mainporcelain
 git-get-tar-commit-id   ancillaryinterrogators
 git-grepmainporcelain   info
 git-gui mainporcelain
 git-hash-object plumbingmanipulators
-git-helpancillaryinterrogators
+git-helpancillaryinterrogators  
complete
 git-http-backendsynchingrepositories
 git-http-fetch  synchelpers
 git-http-push   synchelpers
@@ -80,7 +80,7 @@ git-merge-tree  

[PATCH v4/wip 10/12] help: use command-list.txt for the source of guides

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

The help command currently hard codes the list of guides and their
summary in C. Let's move this list to command-list.txt. This lets us
extract summary lines from Documentation/git*.txt. This also
potentially lets us lists guides in git.txt, but I'll leave that for
now.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/gitattributes.txt |  2 +-
 Documentation/gitmodules.txt|  2 +-
 Documentation/gitrevisions.txt  |  2 +-
 Makefile|  2 +-
 builtin/help.c  | 32 
 command-list.txt|  8 
 help.c  | 14 +-
 help.h  |  1 +
 8 files changed, 26 insertions(+), 37 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 1094fe2b5b..083c2f380d 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -3,7 +3,7 @@ gitattributes(5)
 
 NAME
 
-gitattributes - defining attributes per path
+gitattributes - Defining attributes per path
 
 SYNOPSIS
 
diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index db5d47eb19..4d63def206 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -3,7 +3,7 @@ gitmodules(5)
 
 NAME
 
-gitmodules - defining submodule properties
+gitmodules - Defining submodule properties
 
 SYNOPSIS
 
diff --git a/Documentation/gitrevisions.txt b/Documentation/gitrevisions.txt
index 27dec5b91d..1f6cceaefb 100644
--- a/Documentation/gitrevisions.txt
+++ b/Documentation/gitrevisions.txt
@@ -3,7 +3,7 @@ gitrevisions(7)
 
 NAME
 
-gitrevisions - specifying revisions and ranges for Git
+gitrevisions - Specifying revisions and ranges for Git
 
 SYNOPSIS
 
diff --git a/Makefile b/Makefile
index a60a78ee67..1efb751e46 100644
--- a/Makefile
+++ b/Makefile
@@ -1937,7 +1937,7 @@ $(BUILT_INS): git$X
 
 command-list.h: generate-cmdlist.sh command-list.txt
 
-command-list.h: $(wildcard Documentation/git-*.txt)
+command-list.h: $(wildcard Documentation/git*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
diff --git a/builtin/help.c b/builtin/help.c
index 0e0af8426a..5727fb5e51 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -402,38 +402,6 @@ static void show_html_page(const char *git_cmd)
open_html(page_path.buf);
 }
 
-static struct {
-   const char *name;
-   const char *help;
-} common_guides[] = {
-   { "attributes", N_("Defining attributes per path") },
-   { "everyday", N_("Everyday Git With 20 Commands Or So") },
-   { "glossary", N_("A Git glossary") },
-   { "ignore", N_("Specifies intentionally untracked files to ignore") },
-   { "modules", N_("Defining submodule properties") },
-   { "revisions", N_("Specifying revisions and ranges for Git") },
-   { "tutorial", N_("A tutorial introduction to Git (for version 1.5.1 or 
newer)") },
-   { "workflows", N_("An overview of recommended workflows with Git") },
-};
-
-static void list_common_guides_help(void)
-{
-   int i, longest = 0;
-
-   for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
-   if (longest < strlen(common_guides[i].name))
-   longest = strlen(common_guides[i].name);
-   }
-
-   puts(_("The common Git guides are:\n"));
-   for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
-   printf("   %s   ", common_guides[i].name);
-   mput_char(' ', longest - strlen(common_guides[i].name));
-   puts(_(common_guides[i].help));
-   }
-   putchar('\n');
-}
-
 static const char *check_git_cmd(const char* cmd)
 {
char *alias;
diff --git a/command-list.txt b/command-list.txt
index 3bd23201a6..a57bcb64a1 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -139,3 +139,11 @@ gitweb  
ancillaryinterrogators
 git-whatchanged ancillaryinterrogators
 git-worktreemainporcelain
 git-write-tree  plumbingmanipulators
+gitattributes   guide
+giteveryday guide
+gitglossary guide
+gitignore   guide
+gitmodules  guide
+gitrevisionsguide
+gittutorial guide
+gitworkflowsguide
diff --git a/help.c b/help.c
index 1f27d94661..d5a2aff4cb 100644
--- a/help.c
+++ b/help.c
@@ -55,7 +55,9 @@ static void extract_cmds(struct cmdname_help **p_cmds, 
uint32_t mask)
 
cmds[nr] = *cmd;
 
-   if (skip_prefix(cmd->name, "git-", ))
+   if (skip_prefix(cmd->name, "git-", ) ||
+   (mask == CAT_guide &&
+

[PATCH v4/wip 04/12] Remove common-cmds.h

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

After the last patch, common-cmds.h is no longer used (and it was
actually broken). Remove all related code. command-list.h will take
its place from now on.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .gitignore  |  1 -
 Makefile| 17 ++---
 generate-cmdlist.sh | 46 +++--
 3 files changed, 9 insertions(+), 55 deletions(-)

diff --git a/.gitignore b/.gitignore
index d4c3914167..0836083992 100644
--- a/.gitignore
+++ b/.gitignore
@@ -179,7 +179,6 @@
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
-/common-cmds.h
 /command-list.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index 5c58b0b692..a60a78ee67 100644
--- a/Makefile
+++ b/Makefile
@@ -757,7 +757,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
-GENERATED_H += common-cmds.h command-list.h
+GENERATED_H += command-list.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1914,9 +1914,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)
 
-help.sp help.s help.o: common-cmds.h command-list.h
+help.sp help.s help.o: command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h command-list.h 
GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: command-list.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ -1935,11 +1935,6 @@ $(BUILT_INS): git$X
ln -s $< $@ 2>/dev/null || \
cp $< $@
 
-common-cmds.h: generate-cmdlist.sh command-list.txt
-
-common-cmds.h: $(wildcard Documentation/git-*.txt)
-   $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt COMMON 
>$@+ && mv $@+ $@
-
 command-list.h: generate-cmdlist.sh command-list.txt
 
 command-list.h: $(wildcard Documentation/git-*.txt)
@@ -2153,7 +2148,7 @@ else
 # Dependencies on header files, for platforms that do not support
 # the gcc -MMD option.
 #
-# Dependencies on automatically generated headers such as common-cmds.h or 
command-list.h
+# Dependencies on automatically generated headers such as command-list.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
@@ -2532,7 +2527,7 @@ sparse: $(SP_OBJ)
 style:
git clang-format --style file --diff --extensions c,h
 
-check: common-cmds.h command-list.h
+check: command-list.h
@if sparse; \
then \
echo >&2 "Use 'make sparse' instead"; \
@@ -2780,7 +2775,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h command-list.h 
$(ETAGS_TARGET) tags cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo command-list.h $(ETAGS_TARGET) tags 
cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 88968160e3..57f9800123 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -70,46 +70,6 @@ struct cmdname_help {
uint32_t category;
 };
 "
-if [ -z "$2" ]
-then
-   define_categories "$1"
-   echo
-   print_command_list "$1"
-   exit 0
-fi
-
-echo "static const char *common_cmd_groups[] = {"
-
-grps=grps$$.tmp
-match=match$$.tmp
-trap "rm -f '$grps' '$match'" 0 1 2 3 15
-
-sed -n '
-   1,/^### common groups/b
-   /^### command list/q
-   /^#/b
-   /^[ ]*$/b
-   h;s/^[^ ][^ ]*[ ][  ]*\(.*\)/   N_("\1"),/p
-   g;s/^\([^   ][^ ]*\)[   ].*/\1/w '$grps'
-   ' "$1"
-printf '};\n\n'
-
-n=0
-substnum=
-while read grp
-do
-   echo "^git-..*[ ]$grp"
-   substnum="$substnum${substnum:+;}s/[]$grp/$n/"
-   n=$(($n+1))
-done <"$grps" >"$match"
-
-printf 'static struct cmdname_help common_cmds[] = {\n'
-grep -f "$match" "$1" |
-sed 's/^git-//' |
-sort |
-while read cmd tags
-do
-   tag=$(echo "$tags" | sed "$substnum; s/[^0-9]//g")
-   echo "  {\"$cmd\", $(get_synopsis git-$cmd), $tag},"
-done
-echo "};"
+define_categories "$1"
+echo
+print_command_list "$1"
-- 
2.17.0.519.gb89679a4aa

[PATCH v4/wip 12/12] completion: let git provide the completable command list

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

Instead of maintaining a separate list of command classification,
which often could go out of date, let's centralize the information
back in git.

Note that the current completion script incorrectly classifies
filter-branch as porcelain and t9902 tests this behavior. We keep it
this way in t9902 because this test does not really care which
particular command is porcelain or plubmbing, they're just names.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 119 +
 t/t9902-completion.sh  |   5 +-
 2 files changed, 25 insertions(+), 99 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index a5f13ade20..c74d1d7684 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -835,18 +835,30 @@ __git_complete_strategy ()
 }
 
 __git_commands () {
-   if test -n "${GIT_TESTING_COMMAND_COMPLETION:-}"
-   then
-   printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
-   else
-   git --list-cmds=all
-   fi
+   case "$1" in
+   porcelain)
+   if test -n "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
+   then
+   printf "%s" "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
+   else
+   git --list-cmds=mainporcelain --list-cmds=complete
+   fi
+   ;;
+   all)
+   if test -n "$GIT_TESTING_ALL_COMMAND_LIST"
+   then
+   printf "%s" "$GIT_TESTING_ALL_COMMAND_LIST";;
+   else
+   git --list-cmds=all
+   fi
+   ;;
+   esac
 }
 
-__git_list_all_commands ()
+__git_list_commands ()
 {
local i IFS=" "$'\n'
-   for i in $(__git_commands)
+   for i in $(__git_commands $1)
do
case $i in
*--*) : helper pattern;;
@@ -859,101 +871,14 @@ __git_all_commands=
 __git_compute_all_commands ()
 {
test -n "$__git_all_commands" ||
-   __git_all_commands=$(__git_list_all_commands)
-}
-
-__git_list_porcelain_commands ()
-{
-   local i IFS=" "$'\n'
-   __git_compute_all_commands
-   for i in $__git_all_commands
-   do
-   case $i in
-   *--*) : helper pattern;;
-   applymbox): ask gittus;;
-   applypatch)   : ask gittus;;
-   archimport)   : import;;
-   cat-file) : plumbing;;
-   check-attr)   : plumbing;;
-   check-ignore) : plumbing;;
-   check-mailmap): plumbing;;
-   check-ref-format) : plumbing;;
-   checkout-index)   : plumbing;;
-   column)   : internal helper;;
-   commit-tree)  : plumbing;;
-   count-objects): infrequent;;
-   credential)   : credentials;;
-   credential-*) : credentials helper;;
-   cvsexportcommit)  : export;;
-   cvsimport): import;;
-   cvsserver): daemon;;
-   daemon)   : daemon;;
-   diff-files)   : plumbing;;
-   diff-index)   : plumbing;;
-   diff-tree): plumbing;;
-   fast-import)  : import;;
-   fast-export)  : export;;
-   fsck-objects) : plumbing;;
-   fetch-pack)   : plumbing;;
-   fmt-merge-msg): plumbing;;
-   for-each-ref) : plumbing;;
-   hash-object)  : plumbing;;
-   http-*)   : transport;;
-   index-pack)   : plumbing;;
-   init-db)  : deprecated;;
-   local-fetch)  : plumbing;;
-   ls-files) : plumbing;;
-   ls-remote): plumbing;;
-   ls-tree)  : plumbing;;
-   mailinfo) : plumbing;;
-   mailsplit): plumbing;;
-   merge-*)  : plumbing;;
-   mktree)   : plumbing;;
-   mktag): plumbing;;
-   pack-objects) : plumbing;;
-   pack-redundant)   : plumbing;;
-   pack-refs): plumbing;;
-   parse-remote) : plumbing;;
-   patch-id) : plumbing;;
-   prune): plumbing;;
-   prune-packed) : plumbing;;
-   quiltimport)  : import;;
-   read-tree): plumbing;;
-   receive-pack) : plumbing;;
-   remote-*) : transport;;
-   rerere)   : plumbing;;
-   rev-list) : plumbing;;
-   rev-parse): plumbing;;
-   runstatus): 

[PATCH v4/wip 06/12] git: accept multiple --list-cmds options

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

Later on we may support non-overlapping command groups to
--list-cmds. Allow the user to execute just one "git" process and get
multiple groups. This may matter for git-completion.bash on Windows
because we don't want the user to way for long when TAB-ing and
Windows is slow on launching new processes.

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

diff --git a/git.c b/git.c
index 28bfa96d87..a46263306d 100644
--- a/git.c
+++ b/git.c
@@ -64,6 +64,7 @@ void setup_auto_pager(const char *cmd, int def)
 static int handle_options(const char ***argv, int *argc, int *envchanged)
 {
const char **orig_argv = *argv;
+   int commands_listed = 0;
 
while (*argc > 0) {
const char *cmd = (*argv)[0];
@@ -230,7 +231,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
list_builtins(NO_PARSEOPT, ' ');
else
die("unsupported command listing type '%s'", 
cmd);
-   exit(0);
+   commands_listed++;
} else {
fprintf(stderr, _("unknown option: %s\n"), cmd);
usage(git_usage_string);
@@ -239,6 +240,8 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
(*argv)++;
(*argc)--;
}
+   if (commands_listed)
+   exit(0);
return (*argv) - orig_argv;
 }
 
-- 
2.17.0.519.gb89679a4aa

[PATCH v4/wip 02/12] generate-cmds.sh: export all commands to command-list.h

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

The current generate-cmds.sh generates just enough to print "git help"
output. That is, it only extracts help text for common commands.

The script is now updated to extract help text for all commands and
keep command classification a new file, command-list.h. This will be
useful later:

- "git help -a" could print a short summary of all commands instead of
  just the common ones.

- "git" could produce a list of commands of one or more category. One
  of its use is to reduce another command classification embedded in
  git-completion.bash.

The new file can be generated but is not used anywhere yet. The plan
is we migrate away from common-cmds.h. Then we can kill off
common-cmds.h build rules and generation code (and also delete
duplicate content in command-list.h which we keep for now to not mess
generate-cmds.sh up too much).

PS. The new fixed column requirement on command-list.txt is
technically not needed. But it helps simplify the code a bit at this
stage. We could lift this restriction later if we want to.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .gitignore  |  1 +
 Makefile| 13 ++---
 command-list.txt|  4 +--
 generate-cmdlist.sh | 67 ++---
 4 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..d4c3914167 100644
--- a/.gitignore
+++ b/.gitignore
@@ -180,6 +180,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /common-cmds.h
+/command-list.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index f181687250..2a8913ea21 100644
--- a/Makefile
+++ b/Makefile
@@ -757,7 +757,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
-GENERATED_H += common-cmds.h
+GENERATED_H += common-cmds.h command-list.h
 
 LIB_H = $(shell $(FIND) . \
-name .git -prune -o \
@@ -1938,6 +1938,11 @@ $(BUILT_INS): git$X
 common-cmds.h: generate-cmdlist.sh command-list.txt
 
 common-cmds.h: $(wildcard Documentation/git-*.txt)
+   $(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt COMMON 
>$@+ && mv $@+ $@
+
+command-list.h: generate-cmdlist.sh command-list.txt
+
+command-list.h: $(wildcard Documentation/git-*.txt)
$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ 
&& mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
@@ -2148,7 +2153,7 @@ else
 # Dependencies on header files, for platforms that do not support
 # the gcc -MMD option.
 #
-# Dependencies on automatically generated headers such as common-cmds.h
+# Dependencies on automatically generated headers such as common-cmds.h or 
command-list.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
 
@@ -2527,7 +2532,7 @@ sparse: $(SP_OBJ)
 style:
git clang-format --style file --diff --extensions c,h
 
-check: common-cmds.h
+check: common-cmds.h command-list.h
@if sparse; \
then \
echo >&2 "Use 'make sparse' instead"; \
@@ -2775,7 +2780,7 @@ clean: profile-clean coverage-clean
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
$(RM) -r bin-wrappers $(dep_dirs)
$(RM) -r po/build/
-   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags 
cscope*
+   $(RM) *.pyc *.pyo */*.pyc */*.pyo common-cmds.h command-list.h 
$(ETAGS_TARGET) tags cscope*
$(RM) -r $(GIT_TARNAME) .doc-tmp-dir
$(RM) $(GIT_TARNAME).tar.gz git-core_$(GIT_VERSION)-*.tar.gz
$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
diff --git a/command-list.txt b/command-list.txt
index a1fad28fd8..786536aba0 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -8,8 +8,8 @@ info examine the history and state (see also: git help 
revisions)
 history  grow, mark and tweak your common history
 remote   collaborate (see also: git help workflows)
 
-### command list (do not change this line)
-# command name  category [deprecated] [common]
+### command list (do not change this line, also do not change alignment)
+# command name  category [category] [category]
 git-add mainporcelain   worktree
 git-am  mainporcelain
 git-annotateancillaryinterrogators
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 17d6809ef5..9ba9911f09 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -1,5 +1,27 @@
 #!/bin/sh
 
+die () {
+   echo "$@" >&2
+   exit 1
+}
+
+command_list () {
+   sed '1,/^### command list/d;/^#/d' "$1"
+}
+
+get_categories() {
+   tr ' ' '\n'|
+   grep -v '^$' |
+   sort |
+   uniq
+}
+
+category_list () {
+   command_list "$1" |
+   cut -c 40- |
+   get_categories
+}
+
 get_synopsis () {
local cmd="$1"
 
@@ -12,14 +34,51 @@ get_synopsis () {
}' "Documentation/$cmd.txt"
 }
 

[PATCH v4/wip 08/12] git: support --list-cmds=

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

This allows us to select any group of commands by a category defined
in command-list.txt. This is an internal/hidden option so we don't
have to be picky about the category name or worried about exposing too
much.

This will be used later by git-completion.bash to retrieve certain
command groups.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 generate-cmdlist.sh | 17 +
 git.c   |  2 +-
 help.c  | 19 +++
 help.h  |  1 +
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 57f9800123..c14c0a7adc 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -47,6 +47,21 @@ define_categories() {
test "$bit" -gt 32 && die "Urgh.. too many categories?"
 }
 
+define_category_names() {
+   echo
+   echo "/* Category names */"
+   echo "static const char *category_names[] = {"
+   bit=0
+   category_list "$1" |
+   while read cat
+   do
+   echo "  \"$cat\", /* (1UL << $bit) */"
+   bit=$(($bit+1))
+   done
+   echo "  NULL"
+   echo "};"
+}
+
 print_command_list() {
echo "static struct cmdname_help command_list[] = {"
 
@@ -72,4 +87,6 @@ struct cmdname_help {
 "
 define_categories "$1"
 echo
+define_category_names "$1"
+echo
 print_command_list "$1"
diff --git a/git.c b/git.c
index fa6e542d06..6c071d44f5 100644
--- a/git.c
+++ b/git.c
@@ -232,7 +232,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
else if (!strcmp(cmd, "all"))
list_all_cmds();
else
-   die("unsupported command listing type '%s'", 
cmd);
+   list_cmds_by_category(cmd);
commands_listed++;
} else {
fprintf(stderr, _("unknown option: %s\n"), cmd);
diff --git a/help.c b/help.c
index 217864999e..affa8e4343 100644
--- a/help.c
+++ b/help.c
@@ -305,6 +305,25 @@ void list_all_cmds(void)
clean_cmdnames(_cmds);
 }
 
+void list_cmds_by_category(const char *cat)
+{
+   int i;
+   int cat_id = 0;
+
+   for (i = 0; category_names[i]; i++) {
+   if (!strcmp(cat, category_names[i])) {
+   cat_id = 1 << i;
+   break;
+   }
+   }
+   if (!cat_id)
+   die("unsupported command listing type '%s'", cat);
+
+   for (i = 0; command_list[i].name; i++)
+   if (command_list[i].category & cat_id)
+   puts(command_list[i].name);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index 0bf29f8dc5..3e30542927 100644
--- a/help.h
+++ b/help.h
@@ -18,6 +18,7 @@ static inline void mput_char(char c, unsigned int num)
 
 extern void list_common_cmds_help(void);
 extern void list_all_cmds(void);
+extern void list_cmds_by_category(const char *category);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.519.gb89679a4aa

[PATCH v4/wip 07/12] completion: implement and use --list-cmds=all

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

Instead of parsing "git help -a" output, which is tricky to get right,
less elegant and also slow, make git provide the list in
machine-friendly form.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash |  2 +-
 git.c  |  2 ++
 help.c | 18 ++
 help.h |  1 +
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3556838759..a5f13ade20 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -839,7 +839,7 @@ __git_commands () {
then
printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
else
-   git help -a|egrep '^  [a-zA-Z0-9]'
+   git --list-cmds=all
fi
 }
 
diff --git a/git.c b/git.c
index a46263306d..fa6e542d06 100644
--- a/git.c
+++ b/git.c
@@ -229,6 +229,8 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
list_builtins(0, '\n');
else if (!strcmp(cmd, "parseopt"))
list_builtins(NO_PARSEOPT, ' ');
+   else if (!strcmp(cmd, "all"))
+   list_all_cmds();
else
die("unsupported command listing type '%s'", 
cmd);
commands_listed++;
diff --git a/help.c b/help.c
index 5b4a2f1b4f..217864999e 100644
--- a/help.c
+++ b/help.c
@@ -287,6 +287,24 @@ void list_common_cmds_help(void)
print_cmd_by_category(common_categories);
 }
 
+void list_all_cmds(void)
+{
+   struct cmdnames main_cmds, other_cmds;
+   int i;
+
+   memset(_cmds, 0, sizeof(main_cmds));
+   memset(_cmds, 0, sizeof(other_cmds));
+   load_command_list("git-", _cmds, _cmds);
+
+   for (i = 0; i < main_cmds.cnt; i++)
+   puts(main_cmds.names[i]->name);
+   for (i = 0; i < other_cmds.cnt; i++)
+   puts(other_cmds.names[i]->name);
+
+   clean_cmdnames(_cmds);
+   clean_cmdnames(_cmds);
+}
+
 int is_in_cmdlist(struct cmdnames *c, const char *s)
 {
int i;
diff --git a/help.h b/help.h
index b21d7c94e8..0bf29f8dc5 100644
--- a/help.h
+++ b/help.h
@@ -17,6 +17,7 @@ static inline void mput_char(char c, unsigned int num)
 }
 
 extern void list_common_cmds_help(void);
+extern void list_all_cmds(void);
 extern const char *help_unknown_cmd(const char *cmd);
 extern void load_command_list(const char *prefix,
  struct cmdnames *main_cmds,
-- 
2.17.0.519.gb89679a4aa

[PATCH v4/wip 03/12] help: use command-list.h for common command list

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

The previous commit added code generation for all_cmd_desc[] which
includes almost everything we need to generate common command list.
Convert help code to use that array instead and drop common_cmds[] array.

The description of each common command group is removed from
command-list.txt. This keeps this file format simpler. common-cmds.h
will not be generated correctly after this change due to the
command-list.txt format change. But it does not matter and
common-cmds.h will be removed.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile|   4 +-
 command-list.txt|  10 
 generate-cmdlist.sh |   4 +-
 help.c  | 135 
 4 files changed, 103 insertions(+), 50 deletions(-)

diff --git a/Makefile b/Makefile
index 2a8913ea21..5c58b0b692 100644
--- a/Makefile
+++ b/Makefile
@@ -1914,9 +1914,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
$(filter %.o,$^) $(LIBS)
 
-help.sp help.s help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h command-list.h 
GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
diff --git a/command-list.txt b/command-list.txt
index 786536aba0..3bd23201a6 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,13 +1,3 @@
-# common commands are grouped by themes
-# these groups are output by 'git help' in the order declared here.
-# map each common command in the command list to one of these groups.
-### common groups (do not change this line)
-init start a working area (see also: git help tutorial)
-worktree work on the current change (see also: git help everyday)
-info examine the history and state (see also: git help revisions)
-history  grow, mark and tweak your common history
-remote   collaborate (see also: git help workflows)
-
 ### command list (do not change this line, also do not change alignment)
 # command name  category [category] [category]
 git-add mainporcelain   worktree
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 9ba9911f09..88968160e3 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -6,7 +6,7 @@ die () {
 }
 
 command_list () {
-   sed '1,/^### command list/d;/^#/d' "$1"
+   grep -v '^#' "$1"
 }
 
 get_categories() {
@@ -67,7 +67,7 @@ echo "/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
const char *name;
const char *help;
-   uint32_t group;
+   uint32_t category;
 };
 "
 if [ -z "$2" ]
diff --git a/help.c b/help.c
index 60071a9bea..5b4a2f1b4f 100644
--- a/help.c
+++ b/help.c
@@ -5,13 +5,104 @@
 #include "run-command.h"
 #include "levenshtein.h"
 #include "help.h"
-#include "common-cmds.h"
+#include "command-list.h"
 #include "string-list.h"
 #include "column.h"
 #include "version.h"
 #include "refs.h"
 #include "parse-options.h"
 
+struct category_description {
+   uint32_t category;
+   const char *desc;
+};
+static uint32_t common_mask =
+   CAT_init | CAT_worktree | CAT_info |
+   CAT_history | CAT_remote;
+static struct category_description common_categories[] = {
+   { CAT_init, N_("start a working area (see also: git help tutorial)") },
+   { CAT_worktree, N_("work on the current change (see also: git help 
everyday)") },
+   { CAT_info, N_("examine the history and state (see also: git help 
revisions)") },
+   { CAT_history, N_("grow, mark and tweak your common history") },
+   { CAT_remote, N_("collaborate (see also: git help workflows)") },
+   { 0, NULL }
+};
+
+static void extract_cmds(struct cmdname_help **p_cmds, uint32_t mask)
+{
+   int i, nr = 0;
+   struct cmdname_help *cmds;
+
+   ALLOC_ARRAY(cmds, ARRAY_SIZE(command_list) + 1);
+
+   for (i = 0; i < ARRAY_SIZE(command_list); i++) {
+   const struct cmdname_help *cmd = command_list + i;
+   const char *name;
+
+   if (!(cmd->category & mask))
+   continue;
+
+   cmds[nr] = *cmd;
+
+   if (skip_prefix(cmd->name, "git-", ))
+   cmds[nr].name = name;
+
+   nr++;
+   }
+   cmds[nr].name = NULL;
+   *p_cmds = cmds;
+}
+
+static void print_command_list(const struct cmdname_help *cmds,
+  uint32_t mask, int longest)
+{
+   int i;
+
+   for (i = 0; cmds[i].name; i++) {
+   if (cmds[i].category & mask) {
+   printf("   %s   ", cmds[i].name);
+   mput_char(' ', longest - strlen(cmds[i].name));
+ 

[PATCH v4/wip 01/12] generate-cmds.sh: factor out synopsis extract code

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

This makes it easier to reuse the same code in another place (very
soon).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 generate-cmdlist.sh | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index eeea4b67ea..17d6809ef5 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -1,5 +1,17 @@
 #!/bin/sh
 
+get_synopsis () {
+   local cmd="$1"
+
+   sed -n '
+   /^NAME/,/'"$cmd"'/H
+   ${
+   x
+   s/.*'"$cmd"' - \(.*\)/N_("\1")/
+   p
+   }' "Documentation/$cmd.txt"
+}
+
 echo "/* Automatically generated by generate-cmdlist.sh */
 struct cmdname_help {
char name[16];
@@ -39,12 +51,6 @@ sort |
 while read cmd tags
 do
tag=$(echo "$tags" | sed "$substnum; s/[^0-9]//g")
-   sed -n '
-   /^NAME/,/git-'"$cmd"'/H
-   ${
-   x
-   s/.*git-'"$cmd"' - \(.*\)/  {"'"$cmd"'", N_("\1"), 
'$tag'},/
-   p
-   }' "Documentation/git-$cmd.txt"
+   echo "  {\"$cmd\", $(get_synopsis git-$cmd), $tag},"
 done
 echo "};"
-- 
2.17.0.519.gb89679a4aa

[PATCH v4/wip 00/12] Keep all info in command-list.txt in git binary

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

This is not exactly v4 and likely broken. But I've made several
debatable changes and would like your opinions before making even more
changes in this direction.

In 03/12 I made a format change in command-list.txt. The group
description is no longer in this file but in help.c instead. This
simplifies the format. However it may be harder for people to know
what category means what (but then it's already so for a lot more
categories).

In 11/12 I added the new "complete" category to command-list.txt so
that we could replace the giant list in git-completion.bash. This is
really questionable because several commands will NOT be completable
anymore. These are listed there so we can discuss.

Another thing I wonder is, whether I should tag "nocomplete" instead
of "complete" in command-list.txt, similar to parseopt's nocomplete
strategy: commands are completable by default then we just exclude
some of them. The "complete" tag goes the opposite direction, we only
show ones that are either external, "complete" or mainporcelain.

I think we would go with whitelist than blacklist though to avoid
helper commands showing up by default...

Also in 02/12 I moved to fixed column format. Strictly speaking I
do not need that (but it makes the code slightly more complex). If you
are really against fixed column format, speak up now.

Nguyễn Thái Ngọc Duy (12):
  generate-cmds.sh: factor out synopsis extract code
  generate-cmds.sh: export all commands to command-list.h
  help: use command-list.h for common command list
  Remove common-cmds.h
  git.c: convert --list-*builtins to --list-cmds=*
  git: accept multiple --list-cmds options
  completion: implement and use --list-cmds=all
  git: support --list-cmds=
  help: add "-a --verbose" to list all commands with synopsis
  help: use command-list.txt for the source of guides
  command-list.txt: add new category "complete"
  completion: let git provide the completable command list

 .gitignore |   2 +-
 Documentation/git-help.txt |   4 +-
 Documentation/gitattributes.txt|   2 +-
 Documentation/gitmodules.txt   |   2 +-
 Documentation/gitrevisions.txt |   2 +-
 Makefile   |  16 +--
 builtin/help.c |  39 +-
 command-list.txt   |  55 
 contrib/completion/git-completion.bash | 121 +++-
 generate-cmdlist.sh| 128 +++--
 git.c  |  19 ++-
 help.c | 186 +
 help.h |   4 +
 t/t0012-help.sh|  11 +-
 t/t9902-completion.sh  |   5 +-
 15 files changed, 344 insertions(+), 252 deletions(-)

-- 
2.17.0.519.gb89679a4aa

Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[Elijah Newren]

Hi Junio,

On Wed, Apr 25, 2018 at 1:37 AM, Junio C Hamano  wrote:

> * en/unpack-trees-split-index-fix (2018-04-24) 1 commit
>  - unpack_trees: fix breakage when o->src_index != o->dst_index
>
>  The split-index feature had a long-standing and dormant bug in
>  certain use of the in-core merge machinery, which has been fixed.
>
>  Will merge to 'next'.

This topic...

> * en/rename-directory-detection-reboot (2018-04-25) 36 commits

>
>  Reboot of an attempt to detect wholesale directory renames and use
>  it while merging.
>
>  Expecting a reroll.
>  cf. 

...is the fix to the issue mentioned in this email you reference.  The
various revisions of the en/unpack-trees-split-index-fix patches were
even posted as a response to the parent of the email you reference
(which I thought made sense since the parent was the report of the
issue; sorry if that confused things).

Would you still like a re-roll?

If you do, the _only_ thing I'll change is a minor typo in the commit
message of 948d8a6f0895 ("t6046: testcases checking whether updates
can be skipped in a merge", 2018-04-19), doing a simple s/year/years/.
There aren't any remaining outstanding issues with this series I know
of, and I've tested everything I can think of to test.

[PATCH v2] perf/aggregate: use Getopt::Long for option parsing

[Christian Couder]

When passing an option '--foo' that it does not recognize, the
aggregate.perl script should die with an helpful error message
like:

Unknown option: foo
./aggregate.perl [options] [--] [...] [--] \
[...] >

  Options:
--codespeed  * Format output for Codespeed
--reponame  * Send given reponame to codespeed
--sort-by   * Sort output (only "regression" \
criteria is supported)

rather than:

  fatal: Needed a single revision
  rev-parse --verify --foo: command returned error: 128

To implement that let's use Getopt::Long for option parsing
instead of the current manual and sloppy parsing. This should
save some code and make option parsing simpler, tighter and
safer.

This will avoid something like 'foo--sort-by=regression' to
be handled as if '--sort-by=regression' had been used, for
example.

As Getopt::Long eats '--' at the end of options, this changes
a bit the way '--' is handled as we can now have '--' both
after the options and before the scripts.

Signed-off-by: Christian Couder 
---
Actually it looks like we should not use:

  Getopt::Long::Configure qw/ pass_through /;

as GetOptions() would not error out when an unknown option
like --foo is used.

 t/perf/aggregate.perl | 62 ++-
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 48637ef64b..bc865160e7 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -4,6 +4,7 @@ use lib '../../perl/build/lib';
 use strict;
 use warnings;
 use JSON;
+use Getopt::Long;
 use Git;
 
 sub get_times {
@@ -36,46 +37,34 @@ sub format_times {
return $out;
 }
 
+sub usage {
+   print <
+
+  Options:
+--codespeed  * Format output for Codespeed
+--reponame  * Send given reponame to codespeed
+--sort-by   * Sort output (only "regression" criteria is 
supported)
+--subsection* Use results from given subsection
+
+EOT
+   exit(1);
+}
+
 my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
 $codespeed, $sortby, $subsection, $reponame);
+
+Getopt::Long::Configure qw/ require_order /;
+
+my $rc = GetOptions("codespeed" => \$codespeed,
+   "reponame=s"=> \$reponame,
+   "sort-by=s" => \$sortby,
+   "subsection=s"  => \$subsection);
+usage() unless $rc;
+
 while (scalar @ARGV) {
my $arg = $ARGV[0];
my $dir;
-   if ($arg eq "--codespeed") {
-   $codespeed = 1;
-   shift @ARGV;
-   next;
-   }
-   if ($arg =~ /--sort-by(?:=(.*))?/) {
-   shift @ARGV;
-   if (defined $1) {
-   $sortby = $1;
-   } else {
-   $sortby = shift @ARGV;
-   if (! defined $sortby) {
-   die "'--sort-by' requires an argument";
-   }
-   }
-   next;
-   }
-   if ($arg eq "--subsection") {
-   shift @ARGV;
-   $subsection = $ARGV[0];
-   shift @ARGV;
-   if (! $subsection) {
-   die "empty subsection";
-   }
-   next;
-   }
-   if ($arg eq "--reponame") {
-   shift @ARGV;
-   $reponame = $ARGV[0];
-   shift @ARGV;
-   if (! $reponame) {
-   die "empty reponame";
-   }
-   next;
-   }
last if -f $arg or $arg eq "--";
if (! -d $arg) {
my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
@@ -225,7 +214,8 @@ sub print_sorted_results {
my ($sortby) = @_;
 
if ($sortby ne "regression") {
-   die "only 'regression' is supported as '--sort-by' argument";
+   print "Only 'regression' is supported as '--sort-by' 
argument\n";
+   usage();
}
 
my @evolutions;
-- 
2.17.0.257.g28b659db43

Re: [PATCH v2 1/2] merge: Add merge.renames config setting

[Elijah Newren]

On Tue, Apr 24, 2018 at 1:31 PM, Ben Peart  wrote:
> On 4/24/2018 2:59 PM, Elijah Newren wrote:
>> On Tue, Apr 24, 2018 at 10:11 AM, Ben Peart 
>> wrote:
>>>
>>> diff --git a/builtin/merge.c b/builtin/merge.c
>>> index 8746c5e3e8..3be52cd316 100644
>>> --- a/builtin/merge.c
>>> +++ b/builtin/merge.c
>>> @@ -424,6 +424,7 @@ static void finish(struct commit *head_commit,
>>>  opts.output_format |=
>>>  DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
>>>  opts.detect_rename = DIFF_DETECT_RENAME;
>>> +   git_config_get_bool("merge.renames",
>>> _rename);
>>>  diff_setup_done();
>>>  diff_tree_oid(head, new_head, "", );
>>>  diffcore_std();
>>
>>
>> Shouldn't this also be turned off if either (a) merge.renames is unset
>> and diff.renames is false, or (b) the user specifies -Xno-renames?
>>
>
> This makes me think that I should probably remove the line that overrides
> the detect_rename setting with the merge config setting.  As I look at the
> code, none of the other merge options are reflected in the diffstat;
> instead, all the settings are pretty much hard coded.  Perhaps I shouldn't
> rock that boat.

Actually, stat_graph_width respects the diff.statGraphWidth config
option, even though it's slightly hidden due to the magic value of -1,
and being handled from diff.c.

However, trying to get this suggestion of mine hooked up, particularly
with -Xno-renames and -Xfind-renames (the latter because it might need
to override a merge.renames or diff.renames config setting), might be
slightly tricky because the -X options are only passed down to a
single merge strategy but this code is outside of the merge
strategies.  So making it a separate patch, or even a separate patch
series may make sense.  I'm still interested in this change if you
aren't, but I'm fine with it not being part of your series if you
don't want to tackle it.

Re: [PATCH v2 0/2] add additional config settings for merge

[Ben Peart]

On 4/24/2018 8:13 PM, Junio C Hamano wrote:

Ben Peart  writes:


  diff.renameLimit::
The number of files to consider when performing the copy/rename
-   detection; equivalent to the 'git diff' option `-l`.
+   detection; equivalent to the 'git diff' option `-l`. This setting
+   has no effect if rename detection is turned off.


You mean "turned off via diff.renames"?

This is not meant as a suggestion to rewrite this paragraph
further---but if the answer is "no", then that might be an
indication that the sentence is inviting a misunderstanding.



Yes, this is referring to turned off via the config setting 
"diff.renames" but it could also be turned off by passing "--no-renames" 
on the command line.


To be clear, this documentation change isn't trying to document any 
changes to the code or behavior - it is just an attempt to clarify what 
the existing behavior is.  If it isn't helping, I can remove it.



  diff.renames::
Whether and how Git detects renames.  If set to "false",
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 5a9ab969db..38492bcb98 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -39,7 +39,8 @@ include::fmt-merge-msg-config.txt[]
  merge.renameLimit::
The number of files to consider when performing rename detection
during a merge; if not specified, defaults to the value of
-   diff.renameLimit.
+   diff.renameLimit. This setting has no effect if rename detection
+   is turned off.


Ditto.  If your design is to make the merge machinery completely
ignore diff.renames and only pay attention to merge.renames [*1*],
then it probably is a good idea to be more specific here, by saying
"... is turned off via ...", though.


  merge.renames::
Whether and how Git detects renames.  If set to "false",


[Footnote]

*1* ...which I do not think is such a good idea, by the way.  I'd
personally expect merge.renames to allow overriding and falling back
to diff.renames, just like the {merge,diff}.renameLimit pair does.



It looks like I'm in the minority on whether the merge settings should 
inherit from the corresponding diff settings so I will submit a new 
patch series that does it the same way as the {merge,diff}.renameLimit 
pair works.


I'll leave it as an exercise for someone else [1] to change any other 
merge settings that should behave that way.


[1] 
https://public-inbox.org/git/20180420133632.17580-1-benpe...@microsoft.com/T/#m52a3dbd0945360bfb873fd3b553472558ef3b796

Re: [RFC PATCH] checkout: Force matching mtime between files

[Marc Branchaud]

On 2018-04-25 04:48 AM, Junio C Hamano wrote:

"Robin H. Johnson"  writes:


In the thread from 6 years ago, you asked about tar's behavior for
mtimes. 'tar xf' restores mtimes from the tar archive, so relative
ordering after restore would be the same, and would only rebuild if the
original source happened to be dirty.

This behavior is already non-deterministic in Git, and would be improved
by the patch.


But Git is not an archiver (tar), but is a source code control
system, so I do not think we should spend any extra cycles to
"improve" its behaviour wrt the relative ordering, at least for the
default case.  Only those who rely on having build artifact *and*
source should pay the runtime (and preferrably also the
maintainance) cost.


Anyone who uses "make" or some other mtime-based tool is affected by 
this.  I agree that it's not "Everyone" but it sure is a lot of people.


Are we all that sure that the performance hit is that drastic?  After 
all, we've just done write_entry().  Calling utime() at that point 
should just hit the filesystem cache.



The best approach to do so is to have those people do the "touch"
thing in their own post-checkout hook.  People who use Git as the
source control system won't have to pay runtime cost of doing the
touch thing, and we do not have to maintain such a hook script.
Only those who use the "feature" would.


The post-checkout hook approach is not exactly straightforward.

Naively, it's simply

for F in `git diff --name-only $1 $2`; do touch "$F"; done

But consider:

* Symlinks can cause the wrong file to be touched.  (Granted, Michał's 
proposed patch also doesn't deal with symlinks.)  Let's assume that a 
hook can be crafted will all possible sophistication.  There are still 
some fundamental problems:


* In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are 
identical so the above loop does nothing.  Offhand I'm not even sure how 
a hook might get the right files in this case.


* The hook has to be set up in every repo and submodule (at least until 
something like Ævar's experiments come to fruition).


* A fresh clone can't run the hook.  This is especially important when 
dealing with submodules.  (In one case where we were bit by this, make 
though that half of a fresh submodule clone's files were stale, and 
decided to re-autoconf the entire thing.)



I just don't think the hook approach can completely solve the problem.

I appreciate Ævar's concern that there are more than just two mtime 
requests floating around.  But I think git's users are best served by a 
built-in approach, with a config setting to control the desired mtime 
handling (defaulting to the current behaviour).  People who want a 
different mtime solution will at least have a clear place in the code to 
propose a patch.


M.

Re: GSoC students and mentors in 2018

[Pratik Karki]

On Tue, Apr 24, 2018 at 2:46 AM, Stefan Beller  wrote:
> Hi Git community,
>
> This year we'll participate once again in Google Summer or Code!
> We'll have 3 students and 3 mentors, which is more than in recent years.
>
> Paul-Sebastian Ungureanu mentored by DScho, wants to convert git-stash
> into a builtin.
>
> Alban Gruin and Pratik Karki want to convert parts of git-rebase into
> a builtin. Both are mentored by Christian and myself.
>
> The slots were just announced today, please join me in welcoming them
> to the Git mailing list! (Although you may remember them from the
> micro projects[1,2,3])
>
> [1] 
> https://public-inbox.org/git/20180319155929.7000-1-ungureanupaulsebast...@gmail.com/
> [2] https://public-inbox.org/git/2018030907.17607-1-alban.gr...@gmail.com/
> [3] 
> https://public-inbox.org/git/20180327173137.5970-1-predatoram...@gmail.com/
>
> Thanks,
> Stefan

Hi Git community, I will be working to convert parts of git-rebase into builtin.
I hope my contribution will improve Git.

Best of luck to Alban and Paul. Hope we get our patches merged this summer.

Cheers,
Pratik

Re: java diffs show no method context

[Alban Gruin]

Le 25/04/2018 à 14:53, Ulrich Windl a écrit :
> Hi!
> 
> This is for git 2.13.6, and it may be an FAQ or frequent feature request. 
> Anyway:
> I'm new to Java, and writing my first project using Git, I found that "git 
> diff" only reports the class in the diff context, but not the method (as seen 
> for C, for example).
> I'd wish to have the method where the diff is located.

Hi,

to achieve this behaviour, you have to create a file named
".gitattributes" at the root of your project, containing this line:

*.java diff=java

.gitattributes allows you to configure other things, as described in the
documentation[1].

I hope it helps.

[1] https://www.git-scm.com/docs/gitattributes

Cheers,
Alban

Re: GSoC students and mentors in 2018

[Alban Gruin]

Le 23/04/2018 à 23:01, Stefan Beller a écrit :
> Hi Git community,
> 
> This year we'll participate once again in Google Summer or Code!
> We'll have 3 students and 3 mentors, which is more than in recent years.
> 
> Paul-Sebastian Ungureanu mentored by DScho, wants to convert git-stash
> into a builtin.
> 
> Alban Gruin and Pratik Karki want to convert parts of git-rebase into
> a builtin. Both are mentored by Christian and myself.
> 
> The slots were just announced today, please join me in welcoming them
> to the Git mailing list! (Although you may remember them from the
> micro projects[1,2,3])
> 
> [1] 
> https://public-inbox.org/git/20180319155929.7000-1-ungureanupaulsebast...@gmail.com/
> [2] https://public-inbox.org/git/2018030907.17607-1-alban.gr...@gmail.com/
> [3] 
> https://public-inbox.org/git/20180327173137.5970-1-predatoram...@gmail.com/
> 
> Thanks,
> Stefan
> 

Hi,

it’s an honor for me too! I am excited to work with the git commity.

Cheers,
Alban

Re: [PATCH v1 2/2] merge: Add merge.aggressive config setting

[Ben Peart]

On 4/24/2018 7:57 PM, Junio C Hamano wrote:

Ben Peart  writes:


That said, it makes sense to me to do

this when rename detection is turned off.  In fact, I think you'd
automatically want to set aggressive to true whenever rename detection
is turned off (whether by your merge.renames option or the
-Xno-renames flag).
...


While combining them would work for our specific use scenario (since
we turn both on already along with turning off merge.stat), I really
hesitate to tie these two different flags and code paths together with
a single config setting.


The cases that non-agressive variant leaves unmerged are not
auto-resolved only because marking them as merged will rob the
chance from the rename detection logic to notice which ones are
"new" paths that could be matched with "deleted" ones to turn into
renames.  If rename deteciton is not done, there is no reason to
leave it non aggressive, as "#1 = missing, #2 = something and #3 =
missing" entry (just one example that is not auto-resolved by
non-agressive, but the principle is the same) left unmerged in the
index will get resolved to keep the current entry by the post
processing logic anyway.

In fact, checking git-merge-resolve would tell us that we already
use "aggresive" variant there unconditionally.

So, I think Elijah is correct---there is no reason not to enable
this setting when the other one to refuse rename detection is in
effect.



Thank you. I understand this description and it make sense to me.  I'm 
unfamiliar with the merge code so have to rely on you who are the 
experts for a sanity check.


I will remove the separate merge.aggressive config setting and instead, 
set the aggressive flag when the renames is turned off (whether by the 
new merge.renames setting or by the -Xno-renames flag) as Elijah suggests.

Re: [PATCH v3 4/6] git.c: implement --list-cmds=porcelain

[Duy Nguyen]

On Wed, Apr 25, 2018 at 3:46 PM, SZEDER Gábor  wrote:
> On Tue, Apr 24, 2018 at 6:17 PM, Duy Nguyen  wrote:
>> On Tue, Apr 24, 2018 at 6:12 PM, Duy Nguyen  wrote:
>>> git-completion.bash will be updated to ask git "give me the commands
>>> in the mainporcelain, completable or external category". This also
>>> addresses another thing that bugs me: I wanted an option to let me
>>> complete all commands instead of just porcelain. This approach kinda
>>> generalizes that and it would be easy to let the user choose what
>>> category they want to complete.
>>
>> To complete this: there could also be yet another special category
>> "custom", where --list-cmds=custom simply returns a list of commands
>> specified in config file. With this the user can pick the "custom"
>> category to have total control of what command shows up at "git "
>> if they are not satisfied with the predefined categories.
>
> Note that config settings can be repository-specific, which might
> cause surprises if the user sets a custom command list in one
> repository's config file, but not (or a different one!) in others.
> Then the list of completed commands will depend on where the user
> happened to be when first hitting 'git '.

I think that is expected when the word "config file" is mentioned.
It's no different than aliases, which could also be repo specific and
affects completion.

> Unless you forgo
> caching the list of commands and run 'git --list-cmds=...' every time
> the user hits 'git ', but that will add the overhead of fork()ing
> a subshell and a git command.

This is a good point. I'd rather forgo caching iff the "custom"
strategy is used (technically we could still cache if we know the
source if $HOME/.gitconfig or higher but it's not worth the effort).
Just make it clear to the user that if they go with "custom" strategy
then they may hit some performance hit.

> Once upon a time I toyed with the idea of introducing environment
> variables like $GIT_COMPLETION_EXCLUDE_COMMANDS and
> $GIT_COMPLETION_INCLUDE_COMMANDS, to exclude and include commands from
> 'git '.  I wanted to exclude 'git cherry', because I have never
> ever used it but it's always in the way when I want to cherry-pick.
> And I wanted to include 'git for-each-ref' back when I was running it
> frequently while working on speeding up refs completion, but it
> wouldn't have brought that much benefits, because I have a 'git
> for-each-commit' script, too.
> Never really pursued it, though, just patched the long list in
> __git_list_porcelain_commands() in my git clone :)

I'm tempted to support "delta custom list" (e.g. "+cherry-pick" in the
config variable means "add that command on the existing list", or
"-blame" means exclude it) but that's probably too much work.
-- 
Duy

Re: [PATCH v2 1/1] completion: load completion file for external subcommand

[SZEDER Gábor]

On Mon, Apr 23, 2018 at 5:12 PM, SZEDER Gábor  wrote:
> On Thu, Apr 19, 2018 at 9:07 PM, Florian Gamböck  wrote:
>> On 2018-04-18 21:51, SZEDER Gábor wrote:
>>> I believe the main bash-completion repository can be found at:
>>>
>>>  https://github.com/scop/bash-completion.git
>>>
>>> This repository still contains the branch 'dynamic-loading'; for the
>>> record it points to 3b029892f6f9db3b7210a7f66d636be3e5ec5fa2.
>>>
>>> Two commits on that branch are worth mentioning:
>>>
>>>   20c05b43 (Load completions in separate files dynamically, get rid of
>>> have()., 2011-10-12)
>>>   5baebf81 (Add _xfunc for loading and calling functions on demand,
>>> use it in apt-get, cvsps, rsync, and sshfs., 2011-10-13)

 I think the easiest method is to use a function that is defined by
 bash-completion v2.0+, namely __load_completion.
>>>
>>> This is wrong, __load_completion() was introduced in cad3abfc
>>> (__load_completion: New function, use in _completion_loader and _xfunc,
>>> 2015-07-15), and the first release tag containg it is '2.2' from 2016-03-03.

>>> Would it be possible to use _xfunc() instead to plug that hole?  It seems
>>> the be tricky, because that function not only sources but also _calls_ the
>>> completion function.
>>
>> But isn't this exactly what we want?
>
> No, that's definitely not what we want.

In my previous emails I overlooked the _completion_loader() helper
function.

It seems that this function does almost exactly what we want.  It was
introduced along with dynamic completion loading back in 20c05b43, so
it's available for us even in older LTS/Enterprise releases.  Since
cad3abfc it's a wrapper around __load_completion() and thus benefits
from all the improvements, notably searching for completion scripts in
a user-specified directory ($BASH_COMPLETION_USER_DIR) or in the
user's home directory ($XDG_DATA_HOME or ~/.local/...) as well.  It
loads the matching completion script, but does not call the completion
function unconditionally.

The "almost" refers to he case when _completion_loader() can't find a
completion script with a matching name to load, and then registers the
_minimal() completion function for the given command to do basic path
completion as fallback.  I don't think this matters in practice,
because in this case the given command is a git command in its dashed
form, e.g. 'git-diff-index', and those have been deprecated for a long
time.

I think all you need to do is run a
s/__load_completion/_completion_loader/ on your patch and update the
commit message with relevant bits from the above discussion.

Re: [PATCH v4 00/10] Compute and consume generation numbers

[Derrick Stolee]

As promised, here is the diff from v3.

Thanks,
-Stolee

-- >8 --

diff --git a/builtin/merge.c b/builtin/merge.c
index 7e1da6c6ea..b819756946 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1148,6 +1148,7 @@ int cmd_merge(int argc, const char **argv, const 
char *prefix)
    branch = branch_to_free = resolve_refdup("HEAD", 0, _oid, 
NULL);

    if (branch)
    skip_prefix(branch, "refs/heads/", );
+
    init_diff_ui_defaults();
    git_config(git_merge_config, NULL);

@@ -1156,7 +1157,6 @@ int cmd_merge(int argc, const char **argv, const 
char *prefix)

    else
    head_commit = lookup_commit_or_die(_oid, "HEAD");

-
    if (branch_mergeoptions)
    parse_branch_merge_options(branch_mergeoptions);
    argc = parse_options(argc, argv, prefix, builtin_merge_options,
diff --git a/commit-graph.c b/commit-graph.c
index 21e853c21a..aebd242def 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -257,7 +257,7 @@ static int fill_commit_in_graph(struct commit *item, 
struct commit_graph *g, uin

    uint32_t *parent_data_ptr;
    uint64_t date_low, date_high;
    struct commit_list **pptr;
-   const unsigned char *commit_data = g->chunk_commit_data + 
GRAPH_DATA_WIDTH * pos;
+   const unsigned char *commit_data = g->chunk_commit_data + 
(g->hash_len + 16) * pos;


    item->object.parsed = 1;
    item->graph_pos = pos;
@@ -304,7 +304,7 @@ static int find_commit_in_graph(struct commit *item, 
struct commit_graph *g, uin

    *pos = item->graph_pos;
    return 1;
    } else {
-   return bsearch_graph(commit_graph, &(item->object.oid), 
pos);

+   return bsearch_graph(g, &(item->object.oid), pos);
    }
 }

@@ -312,10 +312,10 @@ int parse_commit_in_graph(struct commit *item)
 {
    uint32_t pos;

-   if (item->object.parsed)
-   return 0;
    if (!core_commit_graph)
    return 0;
+   if (item->object.parsed)
+   return 1;
    prepare_commit_graph();
    if (commit_graph && find_commit_in_graph(item, commit_graph, ))
    return fill_commit_in_graph(item, commit_graph, pos);
@@ -454,9 +454,8 @@ static void write_graph_chunk_data(struct hashfile 
*f, int hash_len,

    else
    packedDate[0] = 0;

-   if ((*list)->generation != GENERATION_NUMBER_INFINITY) {
+   if ((*list)->generation != GENERATION_NUMBER_INFINITY)
    packedDate[0] |= htonl((*list)->generation << 2);
-   }

    packedDate[1] = htonl((*list)->date);
    hashwrite(f, packedDate, 8);
diff --git a/commit.c b/commit.c
index 9ef6f699bd..e2e16ea1a7 100644
--- a/commit.c
+++ b/commit.c
@@ -653,7 +653,7 @@ int compare_commits_by_gen_then_commit_date(const 
void *a_, const void *b_, void

    else if (a->generation > b->generation)
    return -1;

-   /* use date as a heuristic when generataions are equal */
+   /* use date as a heuristic when generations are equal */
    if (a->date < b->date)
    return 1;
    else if (a->date > b->date)
@@ -1078,7 +1078,7 @@ int in_merge_bases_many(struct commit *commit, int 
nr_reference, struct commit *

    }

    if (commit->generation > min_generation)
-   return 0;
+   return ret;

    bases = paint_down_to_common(commit, nr_reference, reference, 
commit->generation);

    if (commit->object.flags & PARENT2)
diff --git a/ref-filter.c b/ref-filter.c
index e2fea6d635..fb35067fc9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,7 @@
 #include "trailer.h"
 #include "wt-status.h"
 #include "commit-slab.h"
+#include "commit-graph.h"

 static struct ref_msg {
    const char *gone;
@@ -1582,7 +1583,7 @@ static int in_commit_list(const struct commit_list 
*want, struct commit *c)

 }

 /*
- * Test whether the candidate or one of its parents is contained in the 
list.

+ * Test whether the candidate is contained in the list.
  * Do not recurse to find out, though, but return -1 if inconclusive.
  */
 static enum contains_result contains_test(struct commit *candidate,
@@ -1629,7 +1630,7 @@ static enum contains_result 
contains_tag_algo(struct commit *candidate,


    for (p = want; p; p = p->next) {
    struct commit *c = p->item;
-   parse_commit_or_die(c);
+   load_commit_graph_info(c);
    if (c->generation < cutoff)
    cutoff = c->generation;
    }

Re: [PATCH v3 4/6] git.c: implement --list-cmds=porcelain

[Duy Nguyen]

On Wed, Apr 25, 2018 at 3:06 PM, SZEDER Gábor  wrote:
>> -__git_list_all_commands ()
>
>> -__git_list_porcelain_commands ()
>
> Users can have their own completion scriptlets for their own git
> commands, and these should be able to rely on helper functions in
> git-completion.bash to do things like refs completion and what not.
> Therefore, we tend not to remove or alter those helper functions in a
> backwards incompatible way, because we don't want to break those
> completion scriptlets.

What kind of "API" guarantee do we provide here? I don't mind adding
the wrappers, but for me it's hard for new contributors (like me) to
see which one should be stable and which one is internal.
-- 
Duy

[PATCH v4 09/10] merge: check config before loading commits

[Derrick Stolee]

Now that we use generation numbers from the commit-graph, we must
ensure that all commits that exist in the commit-graph are loaded
from that file instead of from the object database. Since the
commit-graph file is only checked if core.commitGraph is true, we
must check the default config before we load any commits.

In the merge builtin, the config was checked after loading the HEAD
commit. This was due to the use of the global 'branch' when checking
merge-specific config settings.

Move the config load to be between the initialization of 'branch' and
the commit lookup.

Without this change, a fast-forward merge would hit a BUG("bad
generation skip") statement in commit.c during paint_down_to_common().
This is because the HEAD commit would be loaded with "infinite"
generation but then reached by commits with "finite" generation
numbers.

Add a test to t5318-commit-graph.sh that exercises this code path to
prevent a regression.

Signed-off-by: Derrick Stolee 
---
 builtin/merge.c | 7 ---
 t/t5318-commit-graph.sh | 9 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5e5e4497e3..b819756946 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1148,14 +1148,15 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
branch = branch_to_free = resolve_refdup("HEAD", 0, _oid, NULL);
if (branch)
skip_prefix(branch, "refs/heads/", );
+
+   init_diff_ui_defaults();
+   git_config(git_merge_config, NULL);
+
if (!branch || is_null_oid(_oid))
head_commit = NULL;
else
head_commit = lookup_commit_or_die(_oid, "HEAD");
 
-   init_diff_ui_defaults();
-   git_config(git_merge_config, NULL);
-
if (branch_mergeoptions)
parse_branch_merge_options(branch_mergeoptions);
argc = parse_options(argc, argv, prefix, builtin_merge_options,
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index a380419b65..77d85aefe7 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -221,4 +221,13 @@ test_expect_success 'write graph in bare repo' '
 graph_git_behavior 'bare repo with graph, commit 8 vs merge 1' bare commits/8 
merge/1
 graph_git_behavior 'bare repo with graph, commit 8 vs merge 2' bare commits/8 
merge/2
 
+test_expect_success 'perform fast-forward merge in full repo' '
+   cd "$TRASH_DIRECTORY/full" &&
+   git checkout -b merge-5-to-8 commits/5 &&
+   git merge commits/8 &&
+   git show-ref -s merge-5-to-8 >output &&
+   git show-ref -s commits/8 >expect &&
+   test_cmp expect output
+'
+
 test_done
-- 
2.17.0.39.g685157f7fb

[PATCH v4 10/10] commit-graph.txt: update design document

[Derrick Stolee]

We now calculate generation numbers in the commit-graph file and use
them in paint_down_to_common().

Expand the section on generation numbers to discuss how the three
special generation numbers GENERATION_NUMBER_INFINITY, _ZERO, and
_MAX interact with other generation numbers.

Signed-off-by: Derrick Stolee 
---
 Documentation/technical/commit-graph.txt | 30 +++-
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/commit-graph.txt 
b/Documentation/technical/commit-graph.txt
index 0550c6d0dc..d9f2713efa 100644
--- a/Documentation/technical/commit-graph.txt
+++ b/Documentation/technical/commit-graph.txt
@@ -77,6 +77,29 @@ in the commit graph. We can treat these commits as having 
"infinite"
 generation number and walk until reaching commits with known generation
 number.
 
+We use the macro GENERATION_NUMBER_INFINITY = 0x to mark commits not
+in the commit-graph file. If a commit-graph file was written by a version
+of Git that did not compute generation numbers, then those commits will
+have generation number represented by the macro GENERATION_NUMBER_ZERO = 0.
+
+Since the commit-graph file is closed under reachability, we can guarantee
+the following weaker condition on all commits:
+
+If A and B are commits with generation numbers N amd M, respectively,
+and N < M, then A cannot reach B.
+
+Note how the strict inequality differs from the inequality when we have
+fully-computed generation numbers. Using strict inequality may result in
+walking a few extra commits, but the simplicity in dealing with commits
+with generation number *_INFINITY or *_ZERO is valuable.
+
+We use the macro GENERATION_NUMBER_MAX = 0x3FFF to for commits whose
+generation numbers are computed to be at least this value. We limit at
+this value since it is the largest value that can be stored in the
+commit-graph file using the 30 bits available to generation numbers. This
+presents another case where a commit can have generation number equal to
+that of a parent.
+
 Design Details
 --
 
@@ -98,17 +121,12 @@ Future Work
 - The 'commit-graph' subcommand does not have a "verify" mode that is
   necessary for integration with fsck.
 
-- The file format includes room for precomputed generation numbers. These
-  are not currently computed, so all generation numbers will be marked as
-  0 (or "uncomputed"). A later patch will include this calculation.
-
 - After computing and storing generation numbers, we must make graph
   walks aware of generation numbers to gain the performance benefits they
   enable. This will mostly be accomplished by swapping a commit-date-ordered
   priority queue with one ordered by generation number. The following
-  operations are important candidates:
+  operation is an important candidate:
 
-- paint_down_to_common()
 - 'log --topo-order'
 
 - Currently, parse_commit_gently() requires filling in the root tree
-- 
2.17.0.39.g685157f7fb

[PATCH v4 04/10] commit: use generations in paint_down_to_common()

[Derrick Stolee]

Define compare_commits_by_gen_then_commit_date(), which uses generation
numbers as a primary comparison and commit date to break ties (or as a
comparison when both commits do not have computed generation numbers).

Since the commit-graph file is closed under reachability, we know that
all commits in the file have generation at most GENERATION_NUMBER_MAX
which is less than GENERATION_NUMBER_INFINITY.

This change does not affect the number of commits that are walked during
the execution of paint_down_to_common(), only the order that those
commits are inspected. In the case that commit dates violate topological
order (i.e. a parent is "newer" than a child), the previous code could
walk a commit twice: if a commit is reached with the PARENT1 bit, but
later is re-visited with the PARENT2 bit, then that PARENT2 bit must be
propagated to its parents. Using generation numbers avoids this extra
effort, even if it is somewhat rare.

Signed-off-by: Derrick Stolee 
---
 commit.c | 20 +++-
 commit.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 711f674c18..4d00b0a1d6 100644
--- a/commit.c
+++ b/commit.c
@@ -640,6 +640,24 @@ static int compare_commits_by_author_date(const void *a_, 
const void *b_,
return 0;
 }
 
+int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
void *unused)
+{
+   const struct commit *a = a_, *b = b_;
+
+   /* newer commits first */
+   if (a->generation < b->generation)
+   return 1;
+   else if (a->generation > b->generation)
+   return -1;
+
+   /* use date as a heuristic when generations are equal */
+   if (a->date < b->date)
+   return 1;
+   else if (a->date > b->date)
+   return -1;
+   return 0;
+}
+
 int compare_commits_by_commit_date(const void *a_, const void *b_, void 
*unused)
 {
const struct commit *a = a_, *b = b_;
@@ -789,7 +807,7 @@ static int queue_has_nonstale(struct prio_queue *queue)
 /* all input commits in one and twos[] must have been parsed! */
 static struct commit_list *paint_down_to_common(struct commit *one, int n, 
struct commit **twos)
 {
-   struct prio_queue queue = { compare_commits_by_commit_date };
+   struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
struct commit_list *result = NULL;
int i;
 
diff --git a/commit.h b/commit.h
index aac3b8c56f..64436ff44e 100644
--- a/commit.h
+++ b/commit.h
@@ -341,6 +341,7 @@ extern int remove_signature(struct strbuf *buf);
 extern int check_commit_signature(const struct commit *commit, struct 
signature_check *sigc);
 
 int compare_commits_by_commit_date(const void *a_, const void *b_, void 
*unused);
+int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
void *unused);
 
 LAST_ARG_MUST_BE_NULL
 extern int run_commit_hook(int editor_is_used, const char *index_file, const 
char *name, ...);
-- 
2.17.0.39.g685157f7fb

[PATCH v4 07/10] commit: use generation numbers for in_merge_bases()

[Derrick Stolee]

The containment algorithm for 'git branch --contains' is different
from that for 'git tag --contains' in that it uses is_descendant_of()
instead of contains_tag_algo(). The expensive portion of the branch
algorithm is computing merge bases.

When a commit-graph file exists with generation numbers computed,
we can avoid this merge-base calculation when the target commit has
a larger generation number than the initial commits.

Performance tests were run on a copy of the Linux repository where
HEAD is contained in v4.13 but no earlier tag. Also, all tags were
copied to branches and 'git branch --contains' was tested:

Before: 60.0s
After:   0.4s
Rel %: -99.3%

Reported-by: Jeff King 
Signed-off-by: Derrick Stolee 
---
 commit.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 39a3749abd..7bb007f56a 100644
--- a/commit.c
+++ b/commit.c
@@ -1056,12 +1056,19 @@ int in_merge_bases_many(struct commit *commit, int 
nr_reference, struct commit *
 {
struct commit_list *bases;
int ret = 0, i;
+   uint32_t min_generation = GENERATION_NUMBER_INFINITY;
 
if (parse_commit(commit))
return ret;
-   for (i = 0; i < nr_reference; i++)
+   for (i = 0; i < nr_reference; i++) {
if (parse_commit(reference[i]))
return ret;
+   if (min_generation > reference[i]->generation)
+   min_generation = reference[i]->generation;
+   }
+
+   if (commit->generation > min_generation)
+   return ret;
 
bases = paint_down_to_common(commit, nr_reference, reference);
if (commit->object.flags & PARENT2)
-- 
2.17.0.39.g685157f7fb

[PATCH v4 05/10] commit-graph: always load commit-graph information

[Derrick Stolee]

Most code paths load commits using lookup_commit() and then
parse_commit(). In some cases, including some branch lookups, the commit
is parsed using parse_object_buffer() which side-steps parse_commit() in
favor of parse_commit_buffer().

With generation numbers in the commit-graph, we need to ensure that any
commit that exists in the commit-graph file has its generation number
loaded.

Create new load_commit_graph_info() method to fill in the information
for a commit that exists only in the commit-graph file. Call it from
parse_commit_buffer() after loading the other commit information from
the given buffer. Only fill this information when specified by the
'check_graph' parameter.

Signed-off-by: Derrick Stolee 
---
 commit-graph.c | 45 ++---
 commit-graph.h |  8 
 commit.c   |  7 +--
 commit.h   |  2 +-
 object.c   |  2 +-
 sha1_file.c|  2 +-
 6 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 047fa9fca5..aebd242def 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -245,6 +245,12 @@ static struct commit_list **insert_parent_or_die(struct 
commit_graph *g,
return _list_insert(c, pptr)->next;
 }
 
+static void fill_commit_graph_info(struct commit *item, struct commit_graph 
*g, uint32_t pos)
+{
+   const unsigned char *commit_data = g->chunk_commit_data + 
GRAPH_DATA_WIDTH * pos;
+   item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
+}
+
 static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, 
uint32_t pos)
 {
uint32_t edge_value;
@@ -292,31 +298,40 @@ static int fill_commit_in_graph(struct commit *item, 
struct commit_graph *g, uin
return 1;
 }
 
+static int find_commit_in_graph(struct commit *item, struct commit_graph *g, 
uint32_t *pos)
+{
+   if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
+   *pos = item->graph_pos;
+   return 1;
+   } else {
+   return bsearch_graph(g, &(item->object.oid), pos);
+   }
+}
+
 int parse_commit_in_graph(struct commit *item)
 {
+   uint32_t pos;
+
if (!core_commit_graph)
return 0;
if (item->object.parsed)
return 1;
-
prepare_commit_graph();
-   if (commit_graph) {
-   uint32_t pos;
-   int found;
-   if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
-   pos = item->graph_pos;
-   found = 1;
-   } else {
-   found = bsearch_graph(commit_graph, 
&(item->object.oid), );
-   }
-
-   if (found)
-   return fill_commit_in_graph(item, commit_graph, pos);
-   }
-
+   if (commit_graph && find_commit_in_graph(item, commit_graph, ))
+   return fill_commit_in_graph(item, commit_graph, pos);
return 0;
 }
 
+void load_commit_graph_info(struct commit *item)
+{
+   uint32_t pos;
+   if (!core_commit_graph)
+   return;
+   prepare_commit_graph();
+   if (commit_graph && find_commit_in_graph(item, commit_graph, ))
+   fill_commit_graph_info(item, commit_graph, pos);
+}
+
 static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit 
*c)
 {
struct object_id oid;
diff --git a/commit-graph.h b/commit-graph.h
index 260a468e73..96cccb10f3 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -17,6 +17,14 @@ char *get_commit_graph_filename(const char *obj_dir);
  */
 int parse_commit_in_graph(struct commit *item);
 
+/*
+ * It is possible that we loaded commit contents from the commit buffer,
+ * but we also want to ensure the commit-graph content is correctly
+ * checked and filled. Fill the graph_pos and generation members of
+ * the given commit.
+ */
+void load_commit_graph_info(struct commit *item);
+
 struct tree *get_commit_tree_in_graph(const struct commit *c);
 
 struct commit_graph {
diff --git a/commit.c b/commit.c
index 4d00b0a1d6..39a3749abd 100644
--- a/commit.c
+++ b/commit.c
@@ -331,7 +331,7 @@ const void *detach_commit_buffer(struct commit *commit, 
unsigned long *sizep)
return ret;
 }
 
-int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long 
size)
+int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long 
size, int check_graph)
 {
const char *tail = buffer;
const char *bufptr = buffer;
@@ -386,6 +386,9 @@ int parse_commit_buffer(struct commit *item, const void 
*buffer, unsigned long s
}
item->date = parse_commit_date(bufptr, tail);
 
+   if (check_graph)
+   load_commit_graph_info(item);
+
return 0;
 }
 
@@ -412,7 +415,7 @@ int parse_commit_gently(struct commit *item, int 
quiet_on_missing)
return error("Object %s not a commit",
 oid_to_hex(>object.oid));
}
-  

[PATCH v4 08/10] commit: add short-circuit to paint_down_to_common()

[Derrick Stolee]

When running 'git branch --contains', the in_merge_bases_many()
method calls paint_down_to_common() to discover if a specific
commit is reachable from a set of branches. Commits with lower
generation number are not needed to correctly answer the
containment query of in_merge_bases_many().

Add a new parameter, min_generation, to paint_down_to_common() that
prevents walking commits with generation number strictly less than
min_generation. If 0 is given, then there is no functional change.

For in_merge_bases_many(), we can pass commit->generation as the
cutoff, and this saves time during 'git branch --contains' queries
that would otherwise walk "around" the commit we are inspecting.

For a copy of the Linux repository, where HEAD is checked out at
v4.13~100, we get the following performance improvement for
'git branch --contains' over the previous commit:

Before: 0.21s
After:  0.13s
Rel %: -38%

Signed-off-by: Derrick Stolee 
---
 commit.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index 7bb007f56a..e2e16ea1a7 100644
--- a/commit.c
+++ b/commit.c
@@ -808,11 +808,14 @@ static int queue_has_nonstale(struct prio_queue *queue)
 }
 
 /* all input commits in one and twos[] must have been parsed! */
-static struct commit_list *paint_down_to_common(struct commit *one, int n, 
struct commit **twos)
+static struct commit_list *paint_down_to_common(struct commit *one, int n,
+   struct commit **twos,
+   int min_generation)
 {
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
struct commit_list *result = NULL;
int i;
+   uint32_t last_gen = GENERATION_NUMBER_INFINITY;
 
one->object.flags |= PARENT1;
if (!n) {
@@ -831,6 +834,13 @@ static struct commit_list *paint_down_to_common(struct 
commit *one, int n, struc
struct commit_list *parents;
int flags;
 
+   if (commit->generation > last_gen)
+   BUG("bad generation skip");
+   last_gen = commit->generation;
+
+   if (commit->generation < min_generation)
+   break;
+
flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
if (flags == (PARENT1 | PARENT2)) {
if (!(commit->object.flags & RESULT)) {
@@ -879,7 +889,7 @@ static struct commit_list *merge_bases_many(struct commit 
*one, int n, struct co
return NULL;
}
 
-   list = paint_down_to_common(one, n, twos);
+   list = paint_down_to_common(one, n, twos, 0);
 
while (list) {
struct commit *commit = pop_commit();
@@ -946,7 +956,7 @@ static int remove_redundant(struct commit **array, int cnt)
filled_index[filled] = j;
work[filled++] = array[j];
}
-   common = paint_down_to_common(array[i], filled, work);
+   common = paint_down_to_common(array[i], filled, work, 0);
if (array[i]->object.flags & PARENT2)
redundant[i] = 1;
for (j = 0; j < filled; j++)
@@ -1070,7 +1080,7 @@ int in_merge_bases_many(struct commit *commit, int 
nr_reference, struct commit *
if (commit->generation > min_generation)
return ret;
 
-   bases = paint_down_to_common(commit, nr_reference, reference);
+   bases = paint_down_to_common(commit, nr_reference, reference, 
commit->generation);
if (commit->object.flags & PARENT2)
ret = 1;
clear_commit_marks(commit, all_flags);
-- 
2.17.0.39.g685157f7fb

[PATCH v4 06/10] ref-filter: use generation number for --contains

[Derrick Stolee]

A commit A can reach a commit B only if the generation number of A
is strictly larger than the generation number of B. This condition
allows significantly short-circuiting commit-graph walks.

Use generation number for '--contains' type queries.

On a copy of the Linux repository where HEAD is containd in v4.13
but no earlier tag, the command 'git tag --contains HEAD' had the
following peformance improvement:

Before: 0.81s
After:  0.04s
Rel %:  -95%

Helped-by: Jeff King 
Signed-off-by: Derrick Stolee 
---
 ref-filter.c | 24 
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index aff24d93be..fb35067fc9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,7 @@
 #include "trailer.h"
 #include "wt-status.h"
 #include "commit-slab.h"
+#include "commit-graph.h"
 
 static struct ref_msg {
const char *gone;
@@ -1587,7 +1588,8 @@ static int in_commit_list(const struct commit_list *want, 
struct commit *c)
  */
 static enum contains_result contains_test(struct commit *candidate,
  const struct commit_list *want,
- struct contains_cache *cache)
+ struct contains_cache *cache,
+ uint32_t cutoff)
 {
enum contains_result *cached = contains_cache_at(cache, candidate);
 
@@ -1603,6 +1605,10 @@ static enum contains_result contains_test(struct commit 
*candidate,
 
/* Otherwise, we don't know; prepare to recurse */
parse_commit_or_die(candidate);
+
+   if (candidate->generation < cutoff)
+   return CONTAINS_NO;
+
return CONTAINS_UNKNOWN;
 }
 
@@ -1618,8 +1624,18 @@ static enum contains_result contains_tag_algo(struct 
commit *candidate,
  struct contains_cache *cache)
 {
struct contains_stack contains_stack = { 0, 0, NULL };
-   enum contains_result result = contains_test(candidate, want, cache);
+   enum contains_result result;
+   uint32_t cutoff = GENERATION_NUMBER_INFINITY;
+   const struct commit_list *p;
+
+   for (p = want; p; p = p->next) {
+   struct commit *c = p->item;
+   load_commit_graph_info(c);
+   if (c->generation < cutoff)
+   cutoff = c->generation;
+   }
 
+   result = contains_test(candidate, want, cache, cutoff);
if (result != CONTAINS_UNKNOWN)
return result;
 
@@ -1637,7 +1653,7 @@ static enum contains_result contains_tag_algo(struct 
commit *candidate,
 * If we just popped the stack, parents->item has been marked,
 * therefore contains_test will return a meaningful yes/no.
 */
-   else switch (contains_test(parents->item, want, cache)) {
+   else switch (contains_test(parents->item, want, cache, cutoff)) 
{
case CONTAINS_YES:
*contains_cache_at(cache, commit) = CONTAINS_YES;
contains_stack.nr--;
@@ -1651,7 +1667,7 @@ static enum contains_result contains_tag_algo(struct 
commit *candidate,
}
}
free(contains_stack.contains_stack);
-   return contains_test(candidate, want, cache);
+   return contains_test(candidate, want, cache, cutoff);
 }
 
 static int commit_contains(struct ref_filter *filter, struct commit *commit,
-- 
2.17.0.39.g685157f7fb

[PATCH v4 02/10] commit: add generation number to struct commmit

[Derrick Stolee]

The generation number of a commit is defined recursively as follows:

* If a commit A has no parents, then the generation number of A is one.
* If a commit A has parents, then the generation number of A is one
  more than the maximum generation number among the parents of A.

Add a uint32_t generation field to struct commit so we can pass this
information to revision walks. We use three special values to signal
the generation number is invalid:

GENERATION_NUMBER_INFINITY 0x
GENERATION_NUMBER_MAX 0x3FFF
GENERATION_NUMBER_ZERO 0

The first (_INFINITY) means the generation number has not been loaded or
computed. The second (_MAX) means the generation number is too large to
store in the commit-graph file. The third (_ZERO) means the generation
number was loaded from a commit graph file that was written by a version
of git that did not support generation numbers.

Signed-off-by: Derrick Stolee 
---
 alloc.c| 1 +
 commit-graph.c | 2 ++
 commit.h   | 4 
 3 files changed, 7 insertions(+)

diff --git a/alloc.c b/alloc.c
index cf4f8b61e1..e8ab14f4a1 100644
--- a/alloc.c
+++ b/alloc.c
@@ -94,6 +94,7 @@ void *alloc_commit_node(void)
c->object.type = OBJ_COMMIT;
c->index = alloc_commit_index();
c->graph_pos = COMMIT_NOT_FROM_GRAPH;
+   c->generation = GENERATION_NUMBER_INFINITY;
return c;
 }
 
diff --git a/commit-graph.c b/commit-graph.c
index 70fa1b25fd..9ad21c3ffb 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -262,6 +262,8 @@ static int fill_commit_in_graph(struct commit *item, struct 
commit_graph *g, uin
date_low = get_be32(commit_data + g->hash_len + 12);
item->date = (timestamp_t)((date_high << 32) | date_low);
 
+   item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
+
pptr = >parents;
 
edge_value = get_be32(commit_data + g->hash_len);
diff --git a/commit.h b/commit.h
index 23a3f364ed..aac3b8c56f 100644
--- a/commit.h
+++ b/commit.h
@@ -10,6 +10,9 @@
 #include "pretty.h"
 
 #define COMMIT_NOT_FROM_GRAPH 0x
+#define GENERATION_NUMBER_INFINITY 0x
+#define GENERATION_NUMBER_MAX 0x3FFF
+#define GENERATION_NUMBER_ZERO 0
 
 struct commit_list {
struct commit *item;
@@ -30,6 +33,7 @@ struct commit {
 */
struct tree *maybe_tree;
uint32_t graph_pos;
+   uint32_t generation;
 };
 
 extern int save_commit_buffer;
-- 
2.17.0.39.g685157f7fb

[PATCH v4 00/10] Compute and consume generation numbers

[Derrick Stolee]

Thanks for the feedback on the previous version. I think this series is
stabilizing nicely. I'll reply to this message with an inter-diff as it
is not too large to share but would clutter this cover letter.

Thanks,
-Stolee

-- >8 --

This is the one of several "small" patches that follow the serialized
Git commit graph patch (ds/commit-graph) and lazy-loading trees
(ds/lazy-load-trees).

As described in Documentation/technical/commit-graph.txt, the generation
number of a commit is one more than the maximum generation number among
its parents (trivially, a commit with no parents has generation number
one). This section is expanded to describe the interaction with special
generation numbers GENERATION_NUMBER_INFINITY (commits not in the commit-graph
file) and *_ZERO (commits in a commit-graph file written before generation
numbers were implemented).

This series makes the computation of generation numbers part of the
commit-graph write process.

Finally, generation numbers are used to order commits in the priority
queue in paint_down_to_common(). This allows a short-circuit mechanism
to improve performance of `git branch --contains`.

Further, use generation numbers for 'git tag --contains), providing a
significant speedup (at least 95% for some cases).

A more substantial refactoring of revision.c is required before making
'git log --graph' use generation numbers effectively.

This patch series is built on ds/lazy-load-trees.

Derrick Stolee (10):
  ref-filter: fix outdated comment on in_commit_list
  commit: add generation number to struct commmit
  commit-graph: compute generation numbers
  commit: use generations in paint_down_to_common()
  commit-graph: always load commit-graph information
  ref-filter: use generation number for --contains
  commit: use generation numbers for in_merge_bases()
  commit: add short-circuit to paint_down_to_common()
  merge: check config before loading commits
  commit-graph.txt: update design document

 Documentation/technical/commit-graph.txt | 30 ++--
 alloc.c  |  1 +
 builtin/merge.c  |  7 +-
 commit-graph.c   | 92 
 commit-graph.h   |  8 +++
 commit.c | 54 +++---
 commit.h |  7 +-
 object.c |  2 +-
 ref-filter.c | 26 +--
 sha1_file.c  |  2 +-
 t/t5318-commit-graph.sh  |  9 +++
 11 files changed, 198 insertions(+), 40 deletions(-)


base-commit: 7b8a21dba1bce44d64bd86427d3d92437adc4707
-- 
2.17.0.39.g685157f7fb

[PATCH v4 03/10] commit-graph: compute generation numbers

[Derrick Stolee]

While preparing commits to be written into a commit-graph file, compute
the generation numbers using a depth-first strategy.

The only commits that are walked in this depth-first search are those
without a precomputed generation number. Thus, computation time will be
relative to the number of new commits to the commit-graph file.

If a computed generation number would exceed GENERATION_NUMBER_MAX, then
use GENERATION_NUMBER_MAX instead.

Signed-off-by: Derrick Stolee 
---
 commit-graph.c | 45 +
 1 file changed, 45 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 9ad21c3ffb..047fa9fca5 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -439,6 +439,9 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
else
packedDate[0] = 0;
 
+   if ((*list)->generation != GENERATION_NUMBER_INFINITY)
+   packedDate[0] |= htonl((*list)->generation << 2);
+
packedDate[1] = htonl((*list)->date);
hashwrite(f, packedDate, 8);
 
@@ -571,6 +574,46 @@ static void close_reachable(struct packed_oid_list *oids)
}
 }
 
+static void compute_generation_numbers(struct commit** commits,
+  int nr_commits)
+{
+   int i;
+   struct commit_list *list = NULL;
+
+   for (i = 0; i < nr_commits; i++) {
+   if (commits[i]->generation != GENERATION_NUMBER_INFINITY &&
+   commits[i]->generation != GENERATION_NUMBER_ZERO)
+   continue;
+
+   commit_list_insert(commits[i], );
+   while (list) {
+   struct commit *current = list->item;
+   struct commit_list *parent;
+   int all_parents_computed = 1;
+   uint32_t max_generation = 0;
+
+   for (parent = current->parents; parent; parent = 
parent->next) {
+   if (parent->item->generation == 
GENERATION_NUMBER_INFINITY ||
+   parent->item->generation == 
GENERATION_NUMBER_ZERO) {
+   all_parents_computed = 0;
+   commit_list_insert(parent->item, );
+   break;
+   } else if (parent->item->generation > 
max_generation) {
+   max_generation = 
parent->item->generation;
+   }
+   }
+
+   if (all_parents_computed) {
+   current->generation = max_generation + 1;
+   pop_commit();
+   }
+
+   if (current->generation > GENERATION_NUMBER_MAX)
+   current->generation = GENERATION_NUMBER_MAX;
+   }
+   }
+}
+
 void write_commit_graph(const char *obj_dir,
const char **pack_indexes,
int nr_packs,
@@ -694,6 +737,8 @@ void write_commit_graph(const char *obj_dir,
if (commits.nr >= GRAPH_PARENT_MISSING)
die(_("too many commits to write graph"));
 
+   compute_generation_numbers(commits.list, commits.nr);
+
graph_name = get_commit_graph_filename(obj_dir);
fd = hold_lock_file_for_update(, graph_name, 0);
 
-- 
2.17.0.39.g685157f7fb

[PATCH v4 01/10] ref-filter: fix outdated comment on in_commit_list

[Derrick Stolee]

The in_commit_list() method does not check the parents of
the candidate for containment in the list. Fix the comment
that incorrectly states that it does.

Reported-by: Jakub Narebski 
Signed-off-by: Derrick Stolee 
---
 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index cffd8bf3ce..aff24d93be 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1582,7 +1582,7 @@ static int in_commit_list(const struct commit_list *want, 
struct commit *c)
 }
 
 /*
- * Test whether the candidate or one of its parents is contained in the list.
+ * Test whether the candidate is contained in the list.
  * Do not recurse to find out, though, but return -1 if inconclusive.
  */
 static enum contains_result contains_test(struct commit *candidate,
-- 
2.17.0.39.g685157f7fb

Re: [PATCH v3 5/9] ref-filter: use generation number for --contains

[Derrick Stolee]

On 4/24/2018 2:56 PM, Jakub Narebski wrote:

Derrick Stolee  writes:

One way to fix this is to call 'prepare_commit_graph()' directly and
then test that 'commit_graph' is non-null before performing any
parses. I'm not thrilled with how that couples the commit-graph
implementation to this feature, but that may be necessary to avoid
regressions in the non-commit-graph case.

Another possible solution (not sure if better or worse) would be to
change the signature of contains_tag_algo() function to take parameter
or flag that would decide whether to parse "wants".


If I reorder commits so "commit-graph:always load commit-graph 
information" is before this one, then we can call 
load_commit_graph_info() which just fills the generation and graph_pos 
information. This will keep the coupling very light, instead of needing 
to call prepare_commit_graph() or checking the config setting.


Thanks,
-Stolee

Re: [PATCH v3 4/6] git.c: implement --list-cmds=porcelain

[SZEDER Gábor]

On Tue, Apr 24, 2018 at 6:17 PM, Duy Nguyen  wrote:
> On Tue, Apr 24, 2018 at 6:12 PM, Duy Nguyen  wrote:
>> git-completion.bash will be updated to ask git "give me the commands
>> in the mainporcelain, completable or external category". This also
>> addresses another thing that bugs me: I wanted an option to let me
>> complete all commands instead of just porcelain. This approach kinda
>> generalizes that and it would be easy to let the user choose what
>> category they want to complete.
>
> To complete this: there could also be yet another special category
> "custom", where --list-cmds=custom simply returns a list of commands
> specified in config file. With this the user can pick the "custom"
> category to have total control of what command shows up at "git "
> if they are not satisfied with the predefined categories.

Note that config settings can be repository-specific, which might
cause surprises if the user sets a custom command list in one
repository's config file, but not (or a different one!) in others.
Then the list of completed commands will depend on where the user
happened to be when first hitting 'git '.  Unless you forgo
caching the list of commands and run 'git --list-cmds=...' every time
the user hits 'git ', but that will add the overhead of fork()ing
a subshell and a git command.

Once upon a time I toyed with the idea of introducing environment
variables like $GIT_COMPLETION_EXCLUDE_COMMANDS and
$GIT_COMPLETION_INCLUDE_COMMANDS, to exclude and include commands from
'git '.  I wanted to exclude 'git cherry', because I have never
ever used it but it's always in the way when I want to cherry-pick.
And I wanted to include 'git for-each-ref' back when I was running it
frequently while working on speeding up refs completion, but it
wouldn't have brought that much benefits, because I have a 'git
for-each-commit' script, too.
Never really pursued it, though, just patched the long list in
__git_list_porcelain_commands() in my git clone :)

Re: [PATCH v3 4/6] git.c: implement --list-cmds=porcelain

[SZEDER Gábor]

On Sat, Apr 21, 2018 at 6:54 PM, Nguyễn Thái Ngọc Duy  wrote:
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index a5f13ade20..7d17ca23f6 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -835,18 +835,23 @@ __git_complete_strategy ()

> +__git_list_commands ()

Please add a comment before this function to mention its mandatory
argument and that is should correspond to a category in
'command-list.txt'


> -__git_list_all_commands ()

> -__git_list_porcelain_commands ()

Users can have their own completion scriptlets for their own git
commands, and these should be able to rely on helper functions in
git-completion.bash to do things like refs completion and what not.
Therefore, we tend not to remove or alter those helper functions in a
backwards incompatible way, because we don't want to break those
completion scriptlets.

Your patch removes these two functions.  At first I let it slide,
because first I thought that anyone who needs the list of all git
commands is better off calling __git_compute_all_commands() and then
using $__git_all_commands (same goes for porcelains), because it
involves less fork()ed subshells and external processes.  Then I
thought why would anyone possibly need the list of git commands in a
custom completion scriptlet in the first place...  what for?!

Well, it turns out that there is at least one completion script out
there that does rely on __git_list_all_commands() [1].

Please keep these two functions as a thin wrapper around
__git_list_commands().

[1] And in a rather curious way at that:

https://github.com/github/hub/blob/master/etc/hub.bash_completion.sh#L12

java diffs show no method context

[Ulrich Windl]

Hi!

This is for git 2.13.6, and it may be an FAQ or frequent feature request. 
Anyway:
I'm new to Java, and writing my first project using Git, I found that "git 
diff" only reports the class in the diff context, but not the method (as seen 
for C, for example).
I'd wish to have the method where the diff is located. Here is an example chunk:


@@ -100,6 +119,8 @@ public class PdfParser {
PdfObject obj, value;
PdfObjectName name;
for ( int pos = startIndex; pos < endIndex; pos += 2 ) {
+   if ( (obj = objects.get(pos)).type != PdfObject.Type.Name )
+   exception("Name expected");
name = (PdfObjectName) obj;
if ( pos + 1 == endIndex )
exception("missing value");

The corrsponding definition of the method is like this:

/**
 * Populate Dictionary with parsed objects
 * @param dict Dictionary to fill
 * @param startIndex Position of first item to add
 * @param endIndex Position of first item not to add
 */
private void populateDictionary(PdfObjectDictionary dict, int startIndex, in
t endIndex)
{
...

Regards,
Ulrich

Re: [PATCH v1] perf/aggregate: tighten option parsing

[Christian Couder]

On Sat, Apr 21, 2018 at 5:50 AM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>>> Not necessarily worth a re-roll.
>>
>> Not that it matters in this case, but just as a bit of Perl rx pedantry,
>> yes his is tighter & more correct. You didn't consider how "." interacts
>> with newlines:
>>
>> $ perl -wE 'my @rx = (qr/^--./, qr/^--.+$/, qr/^--./m, qr/^--.+$/m, 
>> qr/^--./s, qr/^--.+$/s); for (@rx) { my $s = "--foo\n--bar"; say $_, "\t", 
>> ($s =~ $_ ? 1 : 0) }'
>> (?^u:^--.)  1
>> (?^u:^--.+$)0
>> (?^um:^--.) 1
>> (?^um:^--.+$)   1
>> (?^us:^--.) 1
>> (?^us:^--.+$)   1
>>
>> I don't think it matters here, not like someone will pass \n in options
>> to aggregate.perl...
>
> Hmph, do we want the command not to barf when "--foo\n--bar" is
> given from the command line and we cannot find such an option?
>
> I thought that the location the match under discussion is used does
> want to see a hit with any option looking string that begins with
> double dashes.  I would have expected "tigher and hence incorrect",
> in other words.
>
> Somewhat puzzled...

I guess it might be better at this point to just "use Getopt::Long;"
(along with "Getopt::Long::Configure qw/ pass_through /;") as git
send-email does. It might avoid mistakes and subtle discussions like
the above.

Thanks for the reviews,
Christian.

Re: [PATCH v3 0/4] rebase -i: avoid stale "# This is a combinationof" in commit messages

[Johannes Schindelin]

Hi Phillip,

On Mon, 23 Apr 2018, Phillip Wood wrote:

> On 23/04/18 19:11, Stefan Beller wrote:
> > 
> > On Sat, Apr 21, 2018 at 12:34 AM, Johannes Schindelin
> >  wrote:
> > > Eric Sunshine pointed out that I had such a commit message in
> > > https://public-inbox.org/git/CAPig+cRrS0_nYJJY=o6cbov630snqhpv5qgrqdd8mw-syzn...@mail.gmail.com/
> > > and I went on a hunt to figure out how the heck this happened.
> > >
> > > Turns out that if there is a fixup/squash chain where the *last* command
> > > fails with merge conflicts, and we either --skip ahead or resolve the
> > > conflict to a clean tree and then --continue, our code does not do a
> > > final cleanup.
> > >
> > > Contrary to my initial gut feeling, this bug was not introduced by my
> > > rewrite in C of the core parts of rebase -i, but it looks to me as if
> > > that bug was with us for a very long time (at least the --skip part).
> > >
> > > The developer (read: user of rebase -i) in me says that we would want to
> > > fast-track this, but the author of rebase -i in me says that we should
> > > be cautious and cook this in `next` for a while.
> > 
> > I looked through the patches again and think this series is good to go.
> 
> I've just realized I commented on an outdated version as the new version was
> posted there rather than as a reply to v1. I've just looked through it and I'm
> not sure it addresses the unnecessary editing of the commit message of the
> previous commit if a single squash command is skipped as outlined in
> https://public-inbox.org/git/b6512eae-e214-9699-4d69-77117a0da...@talktalk.net/

I have not forgotten about this! I simply did not find the time yet, is
all...

The patch series still has not been merged to `next`, but I plan on
working on your suggested changes as an add-on commit anyway. I am not
quite sure yet how I want to handle the "avoid running commit for the
first fixup/squash in the series" problem, but I think we will have to add
*yet another* file that is written (in the "we already have comments in
the commit message" conditional block in error_failed_squash())...

Ciao,
Dscho

js/rebase-recreate-merge, was Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[Johannes Schindelin]

Hi Junio,

On Wed, 25 Apr 2018, Junio C Hamano wrote:

> * js/rebase-recreate-merge (2018-04-24) 16 commits
>  - rebase -i --rebase-merges: add a section to the man page
>  - rebase -i: introduce --rebase-merges=[no-]rebase-cousins
>  - pull: accept --rebase=merges to recreate the branch topology
>  - rebase --rebase-merges: avoid "empty merges"
>  - sequencer: handle post-rewrite for merge commands
>  - sequencer: make refs generated by the `label` command worktree-local
>  - rebase --rebase-merges: add test for --keep-empty
>  - rebase: introduce the --rebase-merges option
>  - rebase-helper --make-script: introduce a flag to rebase merges
>  - sequencer: fast-forward `merge` commands, if possible
>  - sequencer: introduce the `merge` command
>  - git-rebase--interactive: clarify arguments
>  - sequencer: offer helpful advice when a command was rescheduled
>  - sequencer: refactor how original todo list lines are accessed
>  - sequencer: make rearrange_squash() a bit more obvious
>  - sequencer: avoid using errno clobbered by rollback_lock_file()
> 
>  "git rebase" learned "--rebase-merges" to transplant the whole
>  topology of commit graph elsewhere.
> 
>  Was on hold.  What's the donness of this thing?
>  cf. 
> 

There was enough in limbo that it required a v9, and in hindsight that was
a good thing, too, as I prefered to resolve the merge conflicts with
Phillip's patch series myself rather than have you do it.

But I hope v9 is good enough for `master`, even if I agree that it would
be safer to go through `next`.

This patch series, along with the upcoming `sequencer-and-root-commits`
one, is a crucial building block for the two GSoC projects to convert
rebase and rebase -i into builtins.

Ciao,
Dscho

`iconv` should have the encoding `ISO646-SE2`

[Abinsium]

I installed from `Git-2.16.2-64-bit.exe` from git-scm.com. `iconv` is 
included in this package. I think `iconv` should have the encoding 
`ISO646-SE2`. Ubuntu 16.04 has this encoding. I use it to read old 
Swedish text files, which there are a lot of e.g.:
`curl -s https://www.abc.se/programbanken/abc/abc80/asmkod/basicii.txt | 
dos2unix | iconv -f ISO646-SE2 -t UTF8 | less`
`ISO646-SE2` is used by e.g. the retro-computers Luxor 
[ABC80](https://en.wikipedia.org/wiki/ABC_80) (1978) and 
[ABC800](https://en.wikipedia.org/wiki/ABC_800)-series (1981). At my 
university we only have Git Bash and not Ubuntu for WSL in Windows 10 
computers.


(Not clear where I should report this, but one should be able to report 
issues with the configuration of the other programs than `git` in the 
package somewhere. If there is a better place, please let me know.)

Originally reported at https://github.com/git/git-scm.com/issues/1199

[PATCH v9 14/17] rebase --rebase-merges: avoid "empty merges"

[Johannes Schindelin]

The `git merge` command does not allow merging commits that are already
reachable from HEAD: `git merge HEAD^`, for example, will report that we
are already up to date and not change a thing.

In an interactive rebase, such a merge could occur previously, e.g. when
competing (or slightly modified) versions of a patch series were applied
upstream, and the user had to `git rebase --skip` all of the local
commits, and the topic branch becomes "empty" as a consequence.

Let's teach the todo command `merge` to behave the same as `git merge`.

Seeing as it requires some low-level trickery to create such merges with
Git's commands in the first place, we do not even have to bother to
introduce an option to force `merge` to create such merge commits.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c  | 7 +++
 t/t3430-rebase-merges.sh | 8 
 2 files changed, 15 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 558efc1af6e..afa155c2829 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2810,6 +2810,13 @@ static int do_merge(struct commit *commit, const char 
*arg, int arg_len,
write_message("no-ff", 5, git_path_merge_mode(), 0);
 
bases = get_merge_bases(head_commit, merge_commit);
+   if (bases && !oidcmp(_commit->object.oid,
+>item->object.oid)) {
+   ret = 0;
+   /* skip merging an ancestor of HEAD */
+   goto leave_merge;
+   }
+
for (j = bases; j; j = j->next)
commit_list_insert(j->item, );
free_commit_list(bases);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index e9c5dc1cd95..1628c8dcc20 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -215,4 +215,12 @@ test_expect_success 'post-rewrite hook and fixups work for 
merges' '
test_cmp expect actual
 '
 
+test_expect_success 'refuse to merge ancestors of HEAD' '
+   echo "merge HEAD^" >script-from-scratch &&
+   test_config -C wt sequence.editor \""$PWD"/replace-editor.sh\" &&
+   before="$(git rev-parse HEAD)" &&
+   git rebase -i HEAD &&
+   test_cmp_rev HEAD $before
+'
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v9 17/17] rebase -i --rebase-merges: add a section to the man page

[Johannes Schindelin]

The --rebase-merges mode is probably not half as intuitive to use as
its inventor hopes, so let's document it some.

Signed-off-by: Johannes Schindelin 
---
 Documentation/git-rebase.txt | 135 +++
 1 file changed, 135 insertions(+)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index fe681d69281..bd5ecff980e 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -403,6 +403,8 @@ reordered, inserted and dropped at will.
 It is currently only possible to recreate the merge commits using the
 `recursive` merge strategy; Different merge strategies can be used only via
 explicit `exec git merge -s  [...]` commands.
++
+See also REBASING MERGES below.
 
 -p::
 --preserve-merges::
@@ -801,6 +803,139 @@ The ripple effect of a "hard case" recovery is especially 
bad:
 'everyone' downstream from 'topic' will now have to perform a "hard
 case" recovery too!
 
+REBASING MERGES
+-
+
+The interactive rebase command was originally designed to handle
+individual patch series. As such, it makes sense to exclude merge
+commits from the todo list, as the developer may have merged the
+then-current `master` while working on the branch, only to rebase
+all the commits onto `master` eventually (skipping the merge
+commits).
+
+However, there are legitimate reasons why a developer may want to
+recreate merge commits: to keep the branch structure (or "commit
+topology") when working on multiple, inter-related branches.
+
+In the following example, the developer works on a topic branch that
+refactors the way buttons are defined, and on another topic branch
+that uses that refactoring to implement a "Report a bug" button. The
+output of `git log --graph --format=%s -5` may look like this:
+
+
+*   Merge branch 'report-a-bug'
+|\
+| * Add the feedback button
+* | Merge branch 'refactor-button'
+|\ \
+| |/
+| * Use the Button class for all buttons
+| * Extract a generic Button class from the DownloadButton one
+
+
+The developer might want to rebase those commits to a newer `master`
+while keeping the branch topology, for example when the first topic
+branch is expected to be integrated into `master` much earlier than the
+second one, say, to resolve merge conflicts with changes to the
+DownloadButton class that made it into `master`.
+
+This rebase can be performed using the `--rebase-merges` option.
+It will generate a todo list looking like this:
+
+
+label onto
+
+# Branch: refactor-button
+reset onto
+pick 123456 Extract a generic Button class from the DownloadButton one
+pick 654321 Use the Button class for all buttons
+label refactor-button
+
+# Branch: report-a-bug
+reset refactor-button # Use the Button class for all buttons
+pick abcdef Add the feedback button
+label report-a-bug
+
+reset onto
+merge -C a1b2c3 refactor-button # Merge 'refactor-button'
+merge -C 6f5e4d report-a-bug # Merge 'report-a-bug'
+
+
+In contrast to a regular interactive rebase, there are `label`, `reset`
+and `merge` commands in addition to `pick` ones.
+
+The `label` command associates a label with the current HEAD when that
+command is executed. These labels are created as worktree-local refs
+(`refs/rewritten/`) that will be deleted when the rebase
+finishes. That way, rebase operations in multiple worktrees linked to
+the same repository do not interfere with one another. If the `label`
+command fails, it is rescheduled immediately, with a helpful message how
+to proceed.
+
+The `reset` command resets the HEAD, index and worktree to the specified
+revision. It is isimilar to an `exec git reset --hard `, but
+refuses to overwrite untracked files. If the `reset` command fails, it is
+rescheduled immediately, with a helpful message how to edit the todo list
+(this typically happens when a `reset` command was inserted into the todo
+list manually and contains a typo).
+
+The `merge` command will merge the specified revision into whatever is
+HEAD at that time. With `-C `, the commit message of
+the specified merge commit will be used. When the `-C` is changed to
+a lower-case `-c`, the message will be opened in an editor after a
+successful merge so that the user can edit the message.
+
+If a `merge` command fails for any reason other than merge conflicts (i.e.
+when the merge operation did not even start), it is rescheduled immediately.
+
+At this time, the `merge` command will *always* use the `recursive`
+merge strategy, with no way to choose a different one. To work around
+this, an `exec` command can be used to call `git merge` explicitly,
+using the fact that the labels are worktree-local refs (the ref
+`refs/rewritten/onto` would correspond to the label `onto`, for example).
+
+Note: the first command (`label onto`) labels the revision onto which
+the commits are rebased; The name `onto` is just a convention, as a nod
+to the `--onto` option.
+
+It is also possible to 

[PATCH v9 16/17] rebase -i: introduce --rebase-merges=[no-]rebase-cousins

[Johannes Schindelin]

When running `git rebase --rebase-merges` non-interactively with an
ancestor of HEAD as  (or leaving the todo list unmodified),
we would ideally recreate the exact same commits as before the rebase.

However, if there are commits in the commit range .. that do not
have  as direct ancestor (i.e. if `git log ..` would
show commits that are omitted by `git log --ancestry-path ..`),
this is currently not the case: we would turn them into commits that have
 as direct ancestor.

Let's illustrate that with a diagram:

C
  /   \
A - B - E - F
  \   /
D

Currently, after running `git rebase -i --rebase-merges B`, the new branch
structure would be (pay particular attention to the commit `D`):

   --- C' --
  / \
A - B -- E' - F'
  \/
D'

This is not really preserving the branch topology from before! The
reason is that the commit `D` does not have `B` as ancestor, and
therefore it gets rebased onto `B`.

This is unintuitive behavior. Even worse, when recreating branch
structure, most use cases would appear to want cousins *not* to be
rebased onto the new base commit. For example, Git for Windows (the
heaviest user of the Git garden shears, which served as the blueprint
for --rebase-merges) frequently merges branches from `next` early, and
these branches certainly do *not* want to be rebased. In the example
above, the desired outcome would look like this:

   --- C' --
  / \
A - B -- E' - F'
  \/
   -- D' --

Let's introduce the term "cousins" for such commits ("D" in the
example), and let's not rebase them by default. For hypothetical
use cases where cousins *do* need to be rebased, `git rebase
--rebase=merges=rebase-cousins` needs to be used.

Signed-off-by: Johannes Schindelin 
---
 Documentation/git-rebase.txt | 15 +++
 builtin/rebase--helper.c |  9 -
 git-rebase--interactive.sh   |  1 +
 git-rebase.sh| 12 +++-
 sequencer.c  |  4 
 sequencer.h  |  6 ++
 t/t3430-rebase-merges.sh | 18 ++
 7 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 7f1756f1eba..fe681d69281 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -380,7 +380,7 @@ rebase.instructionFormat.  A customized instruction format 
will automatically
 have the long commit hash prepended to the format.
 
 -r::
---rebase-merges::
+--rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
By default, a rebase will simply drop merge commits from the todo
list, and put the rebased commits into a single, linear branch.
With `--rebase-merges`, the rebase will instead try to preserve
@@ -389,9 +389,16 @@ have the long commit hash prepended to the format.
manual amendments in these merge commits will have to be
resolved/re-applied manually.
 +
-This mode is similar in spirit to `--preserve-merges`, but in contrast to
-that option works well in interactive rebases: commits can be reordered,
-inserted and dropped at will.
+By default, or when `no-rebase-cousins` was specified, commits which do not
+have `` as direct ancestor will keep their original branch point,
+i.e. commits that would be excluded by gitlink:git-log[1]'s
+`--ancestry-path` option will keep their original ancestry by default. If
+the `rebase-cousins` mode is turned on, such commits are instead rebased
+onto `` (or ``, if specified).
++
+The `--rebase-merges` mode is similar in spirit to `--preserve-merges`, but
+in contrast to that option works well in interactive rebases: commits can be
+reordered, inserted and dropped at will.
 +
 It is currently only possible to recreate the merge commits using the
 `recursive` merge strategy; Different merge strategies can be used only via
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 781782e7272..f7c2a5fdc81 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
-   int abbreviate_commands = 0;
+   int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
@@ -25,6 +25,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "allow-empty-message", _empty_message,
N_("allow commits with empty messages")),
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
commits")),
+   OPT_BOOL(0, "rebase-cousins", _cousins,
+N_("keep original branch points of cousins")),

[PATCH v9 15/17] pull: accept --rebase=merges to recreate the branch topology

[Johannes Schindelin]

Similar to the `preserve` mode simply passing the `--preserve-merges`
option to the `rebase` command, the `merges` mode simply passes the
`--rebase-merges` option.

This will allow users to conveniently rebase non-trivial commit
topologies when pulling new commits, without flattening them.

Signed-off-by: Johannes Schindelin 
---
 Documentation/config.txt   |  8 
 Documentation/git-pull.txt |  6 +-
 builtin/pull.c | 14 ++
 builtin/remote.c   | 18 ++
 contrib/completion/git-completion.bash |  2 +-
 5 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb37..d6bcb5dcb67 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1058,6 +1058,10 @@ branch..rebase::
"git pull" is run. See "pull.rebase" for doing this in a non
branch-specific manner.
 +
+When `merges`, pass the `--rebase-merges` option to 'git rebase'
+so that the local merge commits are included in the rebase (see
+linkgit:git-rebase[1] for details).
++
 When preserve, also pass `--preserve-merges` along to 'git rebase'
 so that locally committed merge commits will not be flattened
 by running 'git pull'.
@@ -2617,6 +2621,10 @@ pull.rebase::
pull" is run. See "branch..rebase" for setting this on a
per-branch basis.
 +
+When `merges`, pass the `--rebase-merges` option to 'git rebase'
+so that the local merge commits are included in the rebase (see
+linkgit:git-rebase[1] for details).
++
 When preserve, also pass `--preserve-merges` along to 'git rebase'
 so that locally committed merge commits will not be flattened
 by running 'git pull'.
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index ce05b7a5b13..4e0ad6fd8e0 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -101,13 +101,17 @@ Options related to merging
 include::merge-options.txt[]
 
 -r::
---rebase[=false|true|preserve|interactive]::
+--rebase[=false|true|merges|preserve|interactive]::
When true, rebase the current branch on top of the upstream
branch after fetching. If there is a remote-tracking branch
corresponding to the upstream branch and the upstream branch
was rebased since last fetched, the rebase uses that information
to avoid rebasing non-local changes.
 +
+When set to `merges`, rebase using `git rebase --rebase-merges` so that
+the local merge commits are included in the rebase (see
+linkgit:git-rebase[1] for details).
++
 When set to preserve, rebase with the `--preserve-merges` option passed
 to `git rebase` so that locally created merge commits will not be flattened.
 +
diff --git a/builtin/pull.c b/builtin/pull.c
index 71aac5005e0..c719a4f9d73 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -27,14 +27,16 @@ enum rebase_type {
REBASE_FALSE = 0,
REBASE_TRUE,
REBASE_PRESERVE,
+   REBASE_MERGES,
REBASE_INTERACTIVE
 };
 
 /**
  * Parses the value of --rebase. If value is a false value, returns
  * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
- * "preserve", returns REBASE_PRESERVE. If value is a invalid value, dies with
- * a fatal error if fatal is true, otherwise returns REBASE_INVALID.
+ * "merges", returns REBASE_MERGES. If value is "preserve", returns
+ * REBASE_PRESERVE. If value is a invalid value, dies with a fatal error if
+ * fatal is true, otherwise returns REBASE_INVALID.
  */
 static enum rebase_type parse_config_rebase(const char *key, const char *value,
int fatal)
@@ -47,6 +49,8 @@ static enum rebase_type parse_config_rebase(const char *key, 
const char *value,
return REBASE_TRUE;
else if (!strcmp(value, "preserve"))
return REBASE_PRESERVE;
+   else if (!strcmp(value, "merges"))
+   return REBASE_MERGES;
else if (!strcmp(value, "interactive"))
return REBASE_INTERACTIVE;
 
@@ -130,7 +134,7 @@ static struct option pull_options[] = {
/* Options passed to git-merge or git-rebase */
OPT_GROUP(N_("Options related to merging")),
{ OPTION_CALLBACK, 'r', "rebase", _rebase,
- "false|true|preserve|interactive",
+ "false|true|merges|preserve|interactive",
  N_("incorporate changes by rebasing rather than merging"),
  PARSE_OPT_OPTARG, parse_opt_rebase },
OPT_PASSTHRU('n', NULL, _diffstat, NULL,
@@ -800,7 +804,9 @@ static int run_rebase(const struct object_id *curr_head,
argv_push_verbosity();
 
/* Options passed to git-rebase */
-   if (opt_rebase == REBASE_PRESERVE)
+   if (opt_rebase == REBASE_MERGES)
+   argv_array_push(, "--rebase-merges");
+   else if (opt_rebase == REBASE_PRESERVE)
argv_array_push(, "--preserve-merges");
else if (opt_rebase 

[PATCH v9 13/17] sequencer: handle post-rewrite for merge commands

[Johannes Schindelin]

In the previous patches, we implemented the basic functionality of the
`git rebase -i --rebase-merges` command, in particular the `merge`
command to create merge commits in the sequencer.

The interactive rebase is a lot more these days, though, than a simple
cherry-pick in a loop. For example, it calls the post-rewrite hook (if
any) after rebasing with a mapping of the old->new commits.

This patch implements the post-rewrite handling for the `merge` command
we just introduced. The other commands that were added recently (`label`
and `reset`) do not create new commits, therefore post-rewrite hooks do
not need to handle them.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c  |  5 -
 t/t3430-rebase-merges.sh | 25 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index e9297122633..558efc1af6e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3074,7 +3074,10 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
item->arg, item->arg_len,
item->flags, opts)) < 0)
reschedule = 1;
-   else if (res > 0)
+   else if (item->commit)
+   record_in_rewritten(>commit->object.oid,
+   peek_command(todo_list, 1));
+   if (res > 0)
/* failed with merge conflicts */
return error_with_patch(item->commit,
item->arg,
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 96853784ec0..e9c5dc1cd95 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -190,4 +190,29 @@ test_expect_success 'refs/rewritten/* is worktree-local' '
test_cmp_rev HEAD "$(cat wt/b)"
 '
 
+test_expect_success 'post-rewrite hook and fixups work for merges' '
+   git checkout -b post-rewrite &&
+   test_commit same1 &&
+   git reset --hard HEAD^ &&
+   test_commit same2 &&
+   git merge -m "to fix up" same1 &&
+   echo same old same old >same2.t &&
+   test_tick &&
+   git commit --fixup HEAD same2.t &&
+   fixup="$(git rev-parse HEAD)" &&
+
+   mkdir -p .git/hooks &&
+   test_when_finished "rm .git/hooks/post-rewrite" &&
+   echo "cat >actual" | write_script .git/hooks/post-rewrite &&
+
+   test_tick &&
+   git rebase -i --autosquash -r HEAD^^^ &&
+   printf "%s %s\n%s %s\n%s %s\n%s %s\n" >expect $(git rev-parse \
+   $fixup^^2 HEAD^2 \
+   $fixup^^ HEAD^ \
+   $fixup^ HEAD \
+   $fixup HEAD) &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v9 12/17] sequencer: make refs generated by the `label` command worktree-local

[Johannes Schindelin]

This allows for rebases to be run in parallel in separate worktrees
(think: interrupted in the middle of one rebase, being asked to perform
a different rebase, adding a separate worktree just for that job).

Signed-off-by: Johannes Schindelin 
---
 refs.c   |  3 ++-
 t/t3430-rebase-merges.sh | 14 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 8b7a77fe5ee..f61ec58d1df 100644
--- a/refs.c
+++ b/refs.c
@@ -600,7 +600,8 @@ int dwim_log(const char *str, int len, struct object_id 
*oid, char **log)
 static int is_per_worktree_ref(const char *refname)
 {
return !strcmp(refname, "HEAD") ||
-   starts_with(refname, "refs/bisect/");
+   starts_with(refname, "refs/bisect/") ||
+   starts_with(refname, "refs/rewritten/");
 }
 
 static int is_pseudoref_syntax(const char *refname)
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 5f0febb9970..96853784ec0 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -176,4 +176,18 @@ test_expect_success 'with a branch tip that was 
cherry-picked already' '
EOF
 '
 
+test_expect_success 'refs/rewritten/* is worktree-local' '
+   git worktree add wt &&
+   cat >wt/script-from-scratch <<-\EOF &&
+   label xyz
+   exec GIT_DIR=../.git git rev-parse --verify refs/rewritten/xyz >a || :
+   exec git rev-parse --verify refs/rewritten/xyz >b
+   EOF
+
+   test_config -C wt sequence.editor \""$PWD"/replace-editor.sh\" &&
+   git -C wt rebase -i HEAD &&
+   test_must_be_empty wt/a &&
+   test_cmp_rev HEAD "$(cat wt/b)"
+'
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445