Re: [PATCH v5 01/12] sha1-file: rename algorithm to "sha1"

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 04 2018, brian m. carlson wrote:

> The transition plan anticipates us using a syntax such as "^{sha1}" for
> disambiguation.  Since this is a syntax some people will be typing a
> lot, it makes sense to provide a short, easy-to-type syntax.  Omitting
> the dash doesn't create any ambiguity; however, it does make the syntax
> shorter and easier to type, especially for touch typists.  In addition,
> the transition plan already uses "sha1" in this context.

The comment for git_hash_algo's "name" member in hash.h says:

/*
 * The name of the algorithm, as appears in the config file and in
 * messages.
 */
const char *name;

Whereas this commit message just refers to a doesn't-yet-exist ^{$algo}
syntax. The hash-function-transition.txt doc also uses forms like sha1
or sha256 in config, not sha-1 or sha-256.

I don't have a point I'm leading up to here, other than a question of
whether we should be doing something closer to this:

diff --git a/hash.h b/hash.h
index 7c8238bc2e..8ae51ac410 100644
--- a/hash.h
+++ b/hash.h
@@ -67,10 +67,17 @@ typedef void (*git_hash_final_fn)(unsigned char *hash, 
git_hash_ctx *ctx);

 struct git_hash_algo {
/*
-* The name of the algorithm, as appears in the config file and in
-* messages.
+* The short name of the algorithm (e.g. "sha1") for use in
+* config files (see hash-function-transition.txt) and the
+* ^{$name} peel syntax.
 */
-   const char *name;
+   const char *short_name;
+
+   /*
+* The long name of the algorithm (e.g. "SHA-1") for use in
+* messages to users.
+*/
+   const char *long_name;

/* A four-byte version identifier, used in pack indices. */
uint32_t format_id;
diff --git a/sha1-file.c b/sha1-file.c
index dd0b6aa873..5ad0526155 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -86,6 +86,7 @@ static void git_hash_unknown_final(unsigned char *hash, 
git_hash_ctx *ctx)

 const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
{
+   NULL,
NULL,
0x,
0,
@@ -97,7 +98,8 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
NULL,
},
{
-   "sha-1",
+   "sha1",
+   "SHA-1",
/* "sha1", big-endian */
0x73686131,
GIT_SHA1_RAWSZ,

Re: [PATCH v2 0/5] %(trailers) improvements in pretty format

[Anders Waldenborg]

Eric Sunshine writes:
> If "key" is for including particular trailers, intuition might lead
> people to think that "nokey" is for excluding certain trailers.
> Perhaps a different name for "nokey", such as "valueonly" or
> "stripkey", would be better.

Good point. I guess "valueonly" would be preferred as it says what it
shows, not what it hides.

Re: [PATCH 01/13] apply: mark include/exclude options as NONEG

[Junio C Hamano]

Jeff King  writes:

> The options callback for "git apply --no-include" is not ready to handle
> the "unset" parameter, and as a result will segfault when it adds a NULL
> argument to the include list (likewise for "--no-exclude").
>
> In theory this might be used to clear the list, but since both
> "--include" and "--exclude" add to the same list, it's not immediately
> obvious what the semantics should be. Let's punt on that for now and
> just disallow the broken options.

Thanks.  I agree with the conclusion to leave it to later outside
this series to define what --no-(include|exclude) should do.

I suspect something along the lines of

Each element on the single list is marked as either include or
exclude, and "--no-include" would remove the accumulated
"include" entries in the list without touching any "exclude"
elements.

would be sufficiently clear and useful, perhaps.

>
> Signed-off-by: Jeff King 
> ---
>  apply.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 073d5f0451..d1ca6addeb 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4939,10 +4939,10 @@ int apply_parse_options(int argc, const char **argv,
>   struct option builtin_apply_options[] = {
>   { OPTION_CALLBACK, 0, "exclude", state, N_("path"),
>   N_("don't apply changes matching the given path"),
> - 0, apply_option_parse_exclude },
> + PARSE_OPT_NONEG, apply_option_parse_exclude },
>   { OPTION_CALLBACK, 0, "include", state, N_("path"),
>   N_("apply changes matching the given path"),
> - 0, apply_option_parse_include },
> + PARSE_OPT_NONEG, apply_option_parse_include },
>   { OPTION_CALLBACK, 'p', NULL, state, N_("num"),
>   N_("remove  leading slashes from traditional diff 
> paths"),
>   0, apply_option_parse_p },

Re: [PATCH 02/13] am: handle --no-patch-format option

[Junio C Hamano]

Jeff King  writes:

> Running "git am --no-patch-format" will currently segfault, since it
> tries to parse a NULL argument. Instead, let's have it cancel any
> previous --patch-format option.

Makes perfect sense.

>
> Signed-off-by: Jeff King 
> ---
>  builtin/am.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 3ee9a9d2a9..dcb880b699 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2165,7 +2165,9 @@ static int parse_opt_patchformat(const struct option 
> *opt, const char *arg, int
>  {
>   int *opt_value = opt->value;
>  
> - if (!strcmp(arg, "mbox"))
> + if (unset)
> + *opt_value = PATCH_FORMAT_UNKNOWN;
> + else if (!strcmp(arg, "mbox"))
>   *opt_value = PATCH_FORMAT_MBOX;
>   else if (!strcmp(arg, "stgit"))
>   *opt_value = PATCH_FORMAT_STGIT;

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt]

Am 05.11.18 um 00:26 schrieb Junio C Hamano:

OK, thanks.  It seems that the relative silence after this message
is a sign that the resulting patch after squashing is what everybody
is happey with?


I'm not 100% happy. I'll resend a squashed patch, but it has to wait as 
I have to catch a train now.


Appologies for the sub-optimal submission process.

-- Hannes

Re: "git checkout" safety feature

[Jeff King]

On Mon, Nov 05, 2018 at 07:24:42AM +0100, Matthias Urlichs wrote:

> Hi,
> > "git checkout  " is a feature to overwrite local
> > changes.  It is what you use when you make a mess editing the files
> > and want to go back to a known state.  Why should that feature be
> > destroyed?
> 
> Not destroyed, but optionally made finger-fumble-save – like "alias rm
> rm -i".

There are a couple of destructive commands left in Git (e.g., this one,
and "git reset --hard" is another). I didn't dig up archive references,
but the topic of safety valves has come up many times over the years.
The discussion usually ends with the notion that instead of warning
that the operation is destructive (because that gets annoying when its
purpose is to be destructive), we should make it possible to undo a
mistake.

So in this case, that would mean saving the working tree file to a blob
before we obliterate it.

See similar discussion in:

  
https://public-inbox.org/git/cacsjy8c5qolvg4pzy_pthqoygh9ohdevhxsuywqhqypn3ob...@mail.gmail.com/

for example.

-Peff

[PATCH 13/13] assert NOARG/NONEG behavior of parse-options callbacks

[Jeff King]

When we define a parse-options callback, the flags we put in the option
struct must match what the callback expects. For example, a callback
which does not handle the "unset" parameter should only be used with
PARSE_OPT_NONEG. But since the callback and the option struct are not
defined next to each other, it's easy to get this wrong (as earlier
patches in this series show).

Fortunately, the compiler can help us here: compiling with
-Wunused-parameters can show us which callbacks ignore their "unset"
parameters (and likewise, ones that ignore "arg" expect to be triggered
with PARSE_OPT_NOARG).

But after we've inspected a callback and determined that all of its
callers use the right flags, what do we do next? We'd like to silence
the compiler warning, but do so in a way that will catch any wrong calls
in the future.

We can do that by actually checking those variables and asserting that
they match our expectations. Because this is such a common pattern,
we'll introduce some helper macros. The resulting messages aren't
as descriptive as we could make them, but the file/line information from
BUG() is enough to identify the problem (and anyway, the point is that
these should never be seen).

Each of the annotated callbacks in this patch triggers
-Wunused-parameters, and was manually inspected to make sure all callers
use the correct options (so none of these BUGs should be triggerable).

Signed-off-by: Jeff King 
---
 apply.c   | 18 ++
 builtin/blame.c   |  4 
 builtin/cat-file.c|  2 ++
 builtin/checkout-index.c  |  2 ++
 builtin/clean.c   |  1 +
 builtin/commit.c  |  3 +++
 builtin/fetch.c   |  2 ++
 builtin/grep.c| 14 +-
 builtin/init-db.c |  1 +
 builtin/interpret-trailers.c  |  2 ++
 builtin/log.c | 10 ++
 builtin/ls-files.c|  7 +++
 builtin/merge-file.c  |  2 ++
 builtin/merge.c   |  1 +
 builtin/notes.c   |  7 +++
 builtin/pack-objects.c|  3 +++
 builtin/read-tree.c   |  3 +++
 builtin/rebase.c  |  6 ++
 builtin/show-branch.c |  1 +
 builtin/show-ref.c|  1 +
 builtin/tag.c |  2 ++
 builtin/update-index.c| 21 +++--
 parse-options-cb.c|  7 +++
 parse-options.h   | 14 ++
 ref-filter.c  |  2 ++
 t/helper/test-parse-options.c |  1 +
 26 files changed, 134 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index c4c9f849b1..3746310cef 100644
--- a/apply.c
+++ b/apply.c
@@ -4772,6 +4772,9 @@ static int apply_option_parse_exclude(const struct option 
*opt,
  const char *arg, int unset)
 {
struct apply_state *state = opt->value;
+
+   BUG_ON_OPT_NEG(unset);
+
add_name_limit(state, arg, 1);
return 0;
 }
@@ -4780,6 +4783,9 @@ static int apply_option_parse_include(const struct option 
*opt,
  const char *arg, int unset)
 {
struct apply_state *state = opt->value;
+
+   BUG_ON_OPT_NEG(unset);
+
add_name_limit(state, arg, 0);
state->has_include = 1;
return 0;
@@ -4790,6 +4796,9 @@ static int apply_option_parse_p(const struct option *opt,
int unset)
 {
struct apply_state *state = opt->value;
+
+   BUG_ON_OPT_NEG(unset);
+
state->p_value = atoi(arg);
state->p_value_known = 1;
return 0;
@@ -4799,6 +4808,9 @@ static int apply_option_parse_space_change(const struct 
option *opt,
   const char *arg, int unset)
 {
struct apply_state *state = opt->value;
+
+   BUG_ON_OPT_ARG(arg);
+
if (unset)
state->ws_ignore_action = ignore_ws_none;
else
@@ -4810,6 +4822,9 @@ static int apply_option_parse_whitespace(const struct 
option *opt,
 const char *arg, int unset)
 {
struct apply_state *state = opt->value;
+
+   BUG_ON_OPT_NEG(unset);
+
state->whitespace_option = arg;
if (parse_whitespace_option(state, arg))
return -1;
@@ -4820,6 +4835,9 @@ static int apply_option_parse_directory(const struct 
option *opt,
const char *arg, int unset)
 {
struct apply_state *state = opt->value;
+
+   BUG_ON_OPT_NEG(unset);
+
strbuf_reset(>root);
strbuf_addstr(>root, arg);
strbuf_complete(>root, '/');
diff --git a/builtin/blame.c b/builtin/blame.c
index a443af9ee9..06a7163ffe 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -732,6 +732,8 @@ static int blame_copy_callback(const struct option *option, 
const char *arg, int
 {
int *opt = option->value;
 
+   BUG_ON_OPT_NEG(unset);
+
/*
 * -C enables copy from 

[PATCH 12/13] parse-options: drop OPT_DATE()

[Jeff King]

There are no users of OPT_DATE except for test-parse-options; its
only caller went away in 27ec394a97 (prune: introduce OPT_EXPIRY_DATE()
and use it, 2013-04-25).

It also has a bug: it does not specify PARSE_OPT_NONEG, but its callback
does not respect the "unset" flag, and will feed NULL to approxidate()
and segfault. Probably this should be marked with NONEG, or the callback
should set the timestamp to some sentinel value (e.g,. "0", or
"(time_t)-1").

But since there are no callers, deleting it means we don't even have to
think about what the right behavior should be.

Signed-off-by: Jeff King 
---
 parse-options-cb.c|  7 ---
 parse-options.h   |  4 
 t/helper/test-parse-options.c |  1 -
 t/t0040-parse-options.sh  | 22 --
 4 files changed, 34 deletions(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index e8236534ac..6a61166b26 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -28,13 +28,6 @@ int parse_opt_abbrev_cb(const struct option *opt, const char 
*arg, int unset)
return 0;
 }
 
-int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
-int unset)
-{
-   *(timestamp_t *)(opt->value) = approxidate(arg);
-   return 0;
-}
-
 int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
 int unset)
 {
diff --git a/parse-options.h b/parse-options.h
index dd14911a29..c3f2d2eceb 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -150,9 +150,6 @@ struct option {
  (h), 0, _opt_string_list }
 #define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \
  (h), PARSE_OPT_NOARG, _opt_tertiary 
}
-#define OPT_DATE(s, l, v, h) \
-   { OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,\
- parse_opt_approxidate_cb }
 #define OPT_EXPIRY_DATE(s, l, v, h) \
{ OPTION_CALLBACK, (s), (l), (v), N_("expiry-date"),(h), 0, \
  parse_opt_expiry_date_cb }
@@ -232,7 +229,6 @@ extern struct option *parse_options_concat(struct option 
*a, struct option *b);
 
 /*- some often used options -*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
-extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
 extern int parse_opt_expiry_date_cb(const struct option *, const char *, int);
 extern int parse_opt_color_flag_cb(const struct option *, const char *, int);
 extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 9cb8a0ea0f..f0623bb42b 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -119,7 +119,6 @@ int cmd__parse_options(int argc, const char **argv)
OPT_INTEGER('j', NULL, , "get a integer, too"),
OPT_MAGNITUDE('m', "magnitude", , "get a magnitude"),
OPT_SET_INT(0, "set23", , "set integer to 23", 23),
-   OPT_DATE('t', NULL, , "get timestamp of "),
OPT_CALLBACK('L', "length", , "str",
"get length of ", length_callback),
OPT_FILENAME('F', "file", , "set file to "),
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 17d0c18feb..f5b10861c4 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -23,7 +23,6 @@ usage: test-tool parse-options 
 -j get a integer, too
 -m, --magnitudeget a magnitude
 --set23   set integer to 23
--t  get timestamp of 
 -L, --length get length of 
 -F, --file  set file to 
 
@@ -245,27 +244,6 @@ test_expect_success 'keep some options as arguments' '
test-tool parse-options --expect="arg 00: --quux" --quux
 '
 
-cat >expect <<\EOF
-boolean: 0
-integer: 0
-magnitude: 0
-timestamp: 1
-string: (not set)
-abbrev: 7
-verbose: -1
-quiet: 1
-dry run: no
-file: (not set)
-arg 00: foo
-EOF
-
-test_expect_success 'OPT_DATE() works' '
-   test-tool parse-options -t "1970-01-01 00:00:01 +" \
-   foo -q >output 2>output.err &&
-   test_must_be_empty output.err &&
-   test_cmp expect output
-'
-
 cat >expect <<\EOF
 Callback: "four", 0
 boolean: 5
-- 
2.19.1.1505.g9cd28186cf

[PATCH 11/13] apply: return -1 from option callback instead of calling exit(1)

[Jeff King]

The option callback for "apply --whitespace" exits with status "1" on
error. It makes more sense for it to just return an error to
parse-options. That code will exit, too, but it will use status "129"
that is customary for option errors.

The exit() dates back to aaf6c447aa (builtin/apply: make
parse_whitespace_option() return -1 instead of die()ing, 2016-08-08).
That commit gives no reason why we'd prefer the current exit status (it
looks like it was just bumping the "die" up a level in the callstack,
but did not go as far as it could have).

Signed-off-by: Jeff King 
---
 apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index d1ca6addeb..c4c9f849b1 100644
--- a/apply.c
+++ b/apply.c
@@ -4812,7 +4812,7 @@ static int apply_option_parse_whitespace(const struct 
option *opt,
struct apply_state *state = opt->value;
state->whitespace_option = arg;
if (parse_whitespace_option(state, arg))
-   exit(1);
+   return -1;
return 0;
 }
 
-- 
2.19.1.1505.g9cd28186cf

[PATCH 10/13] cat-file: report an error on multiple --batch options

[Jeff King]

The options callback for --batch and --batch-check detects when the two
mutually incompatible options are used. But it simply returns an error
code to parse-options, meaning the program will quit without any kind of
message to the user.

Instead, let's use error() to print something and return -1. Note that
this flips the error return from 1 to -1, but negative values are more
idiomatic here (and parse-options treats them the same).

Signed-off-by: Jeff King 
---
 builtin/cat-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 4a5289079c..0f6b692df6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -596,7 +596,7 @@ static int batch_option_callback(const struct option *opt,
struct batch_options *bo = opt->value;
 
if (bo->enabled) {
-   return 1;
+   return error(_("only one batch option may be specified"));
}
 
bo->enabled = 1;
-- 
2.19.1.1505.g9cd28186cf

[PATCH 09/13] tag: mark "--message" option with NONEG

[Jeff King]

We do not allow "--no-message" to work now, as the option callback
returns "-1" when it sees a NULL arg. However, that will cause
parse-options to exit(129) without printing anything further, leaving
the user confused about what happened.

Instead, let's explicitly mark it as PARSE_OPT_NONEG, which will give a
useful error message (and print the usual -h output).

In theory this could be used to override an earlier "-m", but it's not
clear how it would interact with other message options (e.g., would it
also clear data read for "-F"?). Since it's already disabled and nobody
is asking for it, let's punt on that and just improve the error message.

Signed-off-by: Jeff King 
---
 builtin/tag.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index f623632186..6a396a5090 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -390,8 +390,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_GROUP(N_("Tag creation options")),
OPT_BOOL('a', "annotate", ,
N_("annotated tag, needs a message")),
-   OPT_CALLBACK('m', "message", , N_("message"),
-N_("tag message"), parse_msg_arg),
+   { OPTION_CALLBACK, 'm', "message", , N_("message"),
+ N_("tag message"), PARSE_OPT_NONEG, parse_msg_arg },
OPT_FILENAME('F', "file", , N_("read message from 
file")),
OPT_BOOL('e', "edit", _flag, N_("force edit of tag 
message")),
OPT_BOOL('s', "sign", , N_("annotated and GPG-signed 
tag")),
-- 
2.19.1.1505.g9cd28186cf

[PATCH 08/13] show-branch: mark --reflog option as NONEG

[Jeff King]

Running "git show-branch --no-reflog" will behave as if "--reflog" was
given with no options, which makes no sense.

In theory this option might be used to cancel an earlier "--reflog"
option, but the semantics are not clear. Let's punt on it and just
disallow the broken option.

Signed-off-by: Jeff King 
---
On most of these I tried to decide whether there was a sensible
behavior, and actually do the fix. But I have to admit I have no idea
how "show-branch -g" is supposed to work, which is the main reason I
punted here. If somebody cares enough to write a patch, they can
supersede this (but we should do something, because the current behavior
is nonsense).

 builtin/show-branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 65f4a4c83c..b1b540f7f6 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -674,7 +674,7 @@ int cmd_show_branch(int ac, const char **av, const char 
*prefix)
{ OPTION_CALLBACK, 'g', "reflog", _base, 
N_("[,]"),
N_("show  most recent ref-log entries starting 
at "
   "base"),
-   PARSE_OPT_OPTARG,
+   PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
parse_reflog_param },
OPT_END()
};
-- 
2.19.1.1505.g9cd28186cf

[PATCH 07/13] format-patch: mark "--no-numbered" option with NONEG

[Jeff King]

We have separate parse-options entries for "numbered" and "no-numbered",
which means that we accept "--no-no-numbered". It does not behave
sensibly, though (it ignores the "unset" flag and acts like
"--no-numbered").

We could fix that, but obviously this is silly and unintentional. Let's
just disallow it.

Signed-off-by: Jeff King 
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..41188e723c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1508,7 +1508,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_NOARG, numbered_callback },
{ OPTION_CALLBACK, 'N', "no-numbered", , NULL,
N_("use [PATCH] even with multiple patches"),
-   PARSE_OPT_NOARG, no_numbered_callback },
+   PARSE_OPT_NOARG | PARSE_OPT_NONEG, 
no_numbered_callback },
OPT_BOOL('s', "signoff", _signoff, N_("add Signed-off-by:")),
OPT_BOOL(0, "stdout", _stdout,
N_("print patches to standard out")),
-- 
2.19.1.1505.g9cd28186cf

[PATCH 06/13] status: mark --find-renames option with NONEG

[Jeff King]

If you run "git status --no-find-renames", it will behave the same as
"--find-renames", because we ignore the "unset" parameter (we see a NULL
"arg", but since the score argument is optional, we just think that the
user did not provide a score).

We already have a separate "--no-renames" to disable renames, so there's
not much point in supporting "--no-find-renames". Let's just flag it as
an error.

Signed-off-by: Jeff King 
---
 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 074bd9a551..4d7754ca43 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1335,7 +1335,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
OPT_BOOL(0, "no-renames", _renames, N_("do not detect 
renames")),
{ OPTION_CALLBACK, 'M', "find-renames", _score_arg,
  N_("n"), N_("detect renames, optionally set similarity 
index"),
- PARSE_OPT_OPTARG, opt_parse_rename_score },
+ PARSE_OPT_OPTARG | PARSE_OPT_NONEG, opt_parse_rename_score },
OPT_END(),
};
 
-- 
2.19.1.1505.g9cd28186cf

[PATCH 05/13] cat-file: mark batch options with NONEG

[Jeff King]

Running "cat-file --no-batch" will behave as if "--batch" was given,
since the option callback does not handle the "unset" flag (likewise for
"--no-batch-check").

In theory this might be used to cancel an earlier --batch, but it's not
immediately obvious how that would interact with --batch-check. Let's
just disallow the negated form of both options.

Signed-off-by: Jeff King 
---
 builtin/cat-file.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 8d97c84725..4a5289079c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -631,10 +631,12 @@ int cmd_cat_file(int argc, const char **argv, const char 
*prefix)
OPT_BOOL(0, "buffer", _output, N_("buffer --batch 
output")),
{ OPTION_CALLBACK, 0, "batch", , "format",
N_("show info and content of objects fed from the 
standard input"),
-   PARSE_OPT_OPTARG, batch_option_callback },
+   PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
+   batch_option_callback },
{ OPTION_CALLBACK, 0, "batch-check", , "format",
N_("show info about objects fed from the standard 
input"),
-   PARSE_OPT_OPTARG, batch_option_callback },
+   PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
+   batch_option_callback },
OPT_BOOL(0, "follow-symlinks", _symlinks,
 N_("follow in-tree symlinks (used with --batch or 
--batch-check)")),
OPT_BOOL(0, "batch-all-objects", _objects,
-- 
2.19.1.1505.g9cd28186cf

[PATCH 04/13] pack-objects: mark index-version option as NONEG

[Jeff King]

Running "git pack-objects --no-index-version" will segfault, since the
callback is not prepared to handle the "unset" flag.

In theory this might be used to counteract an earlier "--index-version",
or override a pack.indexversion config setting. But the semantics aren't
immediately obvious, and it's unlikely anybody wants this. Let's just
disable the broken option for now.

Signed-off-by: Jeff King 
---
 builtin/pack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index e50c6cd1ff..4eac2f997b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3252,7 +3252,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
 N_("similar to --all-progress when progress meter is 
shown")),
{ OPTION_CALLBACK, 0, "index-version", NULL, 
N_("[,]"),
  N_("write the pack index file in the specified idx format 
version"),
- 0, option_parse_index_version },
+ PARSE_OPT_NONEG, option_parse_index_version },
OPT_MAGNITUDE(0, "max-pack-size", _size_limit,
  N_("maximum size of each output pack file")),
OPT_BOOL(0, "local", ,
-- 
2.19.1.1505.g9cd28186cf

[PATCH 03/13] ls-files: mark exclude options as NONEG

[Jeff King]

Running "git ls-files --no-exclude" will currently segfault, as its
option callback does not handle the "unset" parameter.

In theory this could be used to clear the exclude list, but it is not
clear how that would interact with the other exclude options, nor is the
current code capable of clearing the list. Let's just disable the broken
option.

Note that --no-exclude-from will similarly segfault, but
--no-exclude-standard will not. It just silently does the wrong thing
(pretending as if --exclude-standard was specified).

Signed-off-by: Jeff King 
---
 builtin/ls-files.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 7f9919a362..2787453eb1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -548,15 +548,16 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
N_("show resolve-undo information")),
{ OPTION_CALLBACK, 'x', "exclude", _list, N_("pattern"),
N_("skip files matching pattern"),
-   0, option_parse_exclude },
+   PARSE_OPT_NONEG, option_parse_exclude },
{ OPTION_CALLBACK, 'X', "exclude-from", , N_("file"),
N_("exclude patterns are read from "),
-   0, option_parse_exclude_from },
+   PARSE_OPT_NONEG, option_parse_exclude_from },
OPT_STRING(0, "exclude-per-directory", _per_dir, 
N_("file"),
N_("read additional per-directory exclude patterns in 
")),
{ OPTION_CALLBACK, 0, "exclude-standard", , NULL,
N_("add the standard git exclusions"),
-   PARSE_OPT_NOARG, option_parse_exclude_standard },
+   PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+   option_parse_exclude_standard },
OPT_SET_INT_F(0, "full-name", _len,
  N_("make the output relative to the project top 
directory"),
  0, PARSE_OPT_NONEG),
-- 
2.19.1.1505.g9cd28186cf

[PATCH 01/13] apply: mark include/exclude options as NONEG

[Jeff King]

The options callback for "git apply --no-include" is not ready to handle
the "unset" parameter, and as a result will segfault when it adds a NULL
argument to the include list (likewise for "--no-exclude").

In theory this might be used to clear the list, but since both
"--include" and "--exclude" add to the same list, it's not immediately
obvious what the semantics should be. Let's punt on that for now and
just disallow the broken options.

Signed-off-by: Jeff King 
---
 apply.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 073d5f0451..d1ca6addeb 100644
--- a/apply.c
+++ b/apply.c
@@ -4939,10 +4939,10 @@ int apply_parse_options(int argc, const char **argv,
struct option builtin_apply_options[] = {
{ OPTION_CALLBACK, 0, "exclude", state, N_("path"),
N_("don't apply changes matching the given path"),
-   0, apply_option_parse_exclude },
+   PARSE_OPT_NONEG, apply_option_parse_exclude },
{ OPTION_CALLBACK, 0, "include", state, N_("path"),
N_("apply changes matching the given path"),
-   0, apply_option_parse_include },
+   PARSE_OPT_NONEG, apply_option_parse_include },
{ OPTION_CALLBACK, 'p', NULL, state, N_("num"),
N_("remove  leading slashes from traditional diff 
paths"),
0, apply_option_parse_p },
-- 
2.19.1.1505.g9cd28186cf

[PATCH 02/13] am: handle --no-patch-format option

[Jeff King]

Running "git am --no-patch-format" will currently segfault, since it
tries to parse a NULL argument. Instead, let's have it cancel any
previous --patch-format option.

Signed-off-by: Jeff King 
---
 builtin/am.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3ee9a9d2a9..dcb880b699 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2165,7 +2165,9 @@ static int parse_opt_patchformat(const struct option 
*opt, const char *arg, int
 {
int *opt_value = opt->value;
 
-   if (!strcmp(arg, "mbox"))
+   if (unset)
+   *opt_value = PATCH_FORMAT_UNKNOWN;
+   else if (!strcmp(arg, "mbox"))
*opt_value = PATCH_FORMAT_MBOX;
else if (!strcmp(arg, "stgit"))
*opt_value = PATCH_FORMAT_STGIT;
-- 
2.19.1.1505.g9cd28186cf

[PATCH 0/13] parseopt fixes from -Wunused-parameters

[Jeff King]

Continuing my exploration of what -Wunused-parameters can show us, here
are some bug-fixes related to parse-options callbacks.

This is the last of the actual bug-fixes I've found. After this, I have
about 60 patches worth of cleanups (i.e., dropping the unused
parameters), and then I have a series to annotate parameters that must
be unused (e.g., for functions that must conform to callback
interfaces). After we can start compiling with -Wunused-parameters,
assuming we don't find the annotations too cumbersome.

But this series fixes real bugs. These first four fix segfaults:

  [01/13]: apply: mark include/exclude options as NONEG
  [02/13]: am: handle --no-patch-format option
  [03/13]: ls-files: mark exclude options as NONEG
  [04/13]: pack-objects: mark index-version option as NONEG

And these four fix cases where we just quietly do the wrong thing:

  [05/13]: cat-file: mark batch options with NONEG
  [06/13]: status: mark --find-renames option with NONEG
  [07/13]: format-patch: mark "--no-numbered" option with NONEG
  [08/13]: show-branch: mark --reflog option as NONEG

These ones are just message improvements:

  [09/13]: tag: mark "--message" option with NONEG
  [10/13]: cat-file: report an error on multiple --batch options
  [11/13]: apply: return -1 from option callback instead of calling exit(1)

This one is a segfault, but it has no callers. ;)

  [12/13]: parse-options: drop OPT_DATE()

And then this last one is mostly about annotating the callbacks. It
doesn't strictly need to happen here, but the alternative is that I'd
eventually have to deal with it in the later series I mentioned.

  [13/13]: assert NOARG/NONEG behavior of parse-options callbacks

 apply.c   | 24 +---
 builtin/am.c  |  4 +++-
 builtin/blame.c   |  4 
 builtin/cat-file.c| 10 +++---
 builtin/checkout-index.c  |  2 ++
 builtin/clean.c   |  1 +
 builtin/commit.c  |  5 -
 builtin/fetch.c   |  2 ++
 builtin/grep.c| 14 +-
 builtin/init-db.c |  1 +
 builtin/interpret-trailers.c  |  2 ++
 builtin/log.c | 12 +++-
 builtin/ls-files.c| 14 +++---
 builtin/merge-file.c  |  2 ++
 builtin/merge.c   |  1 +
 builtin/notes.c   |  7 +++
 builtin/pack-objects.c|  5 -
 builtin/read-tree.c   |  3 +++
 builtin/rebase.c  |  6 ++
 builtin/show-branch.c |  3 ++-
 builtin/show-ref.c|  1 +
 builtin/tag.c |  6 --
 builtin/update-index.c| 21 +++--
 parse-options-cb.c| 14 +++---
 parse-options.h   | 18 ++
 ref-filter.c  |  2 ++
 t/helper/test-parse-options.c |  2 +-
 t/t0040-parse-options.sh  | 22 --
 28 files changed, 155 insertions(+), 53 deletions(-)

-Peff

Re: "git checkout" safety feature

[Matthias Urlichs]

Hi,
> "git checkout  " is a feature to overwrite local
> changes.  It is what you use when you make a mess editing the files
> and want to go back to a known state.  Why should that feature be
> destroyed?

Not destroyed, but optionally made finger-fumble-save – like "alias rm
rm -i".

-- 
-- Matthias Urlichs




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] diff: differentiate error handling in parse_color_moved_ws

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>>  
>> -static int parse_color_moved_ws(const char *arg)
>> +static unsigned parse_color_moved_ws(const char *arg)
>>  {
>>  int ret = 0;
>>  struct string_list l = STRING_LIST_INIT_DUP;
>> @@ -312,15 +312,19 @@ static int parse_color_moved_ws(const char *arg)
>>  ret |= XDF_IGNORE_WHITESPACE;
>>  else if (!strcmp(sb.buf, "allow-indentation-change"))
>>  ret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;
>> -else
>> +else {
>> +ret |= COLOR_MOVED_WS_ERROR;
>>  error(_("ignoring unknown color-moved-ws mode '%s'"), 
>> sb.buf);
>> +}
>> ...  
>>  } else if (skip_prefix(arg, "--color-moved-ws=", )) {
>> -options->color_moved_ws_handling = parse_color_moved_ws(arg);
>> +unsigned cm = parse_color_moved_ws(arg);
>> +if (cm & COLOR_MOVED_WS_ERROR)
>> +die("bad --color-moved-ws argument: %s", arg);
>> +options->color_moved_ws_handling = cm;
>
> Excellent.
>
> Will queue.  Perhaps a test or two can follow to ensure a bad value
> from config does not kill while a command line does?

Wait.  This does not fix

git -c diff.colormovedws=nonsense diff

that dies with an error message---it should ignore the config and at
moat issue a warning.

The command line handling of

git diff --color-moved-ws=nonsense

does correctly die, but it first says "error: ignoring" before
saying "fatal: bad argument", which is suboptimal.

So, not so excellent (yet) X-<.

Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Junio C Hamano]

Anders Waldenborg  writes:

> + if (opts->separator && first_printed)
> + strbuf_addbuf(out, opts->separator);
>   if (opts->no_key)
> - strbuf_addf(out, "%s\n", val.buf);
> + strbuf_addf(out, "%s", val.buf);

Avoid addf with "%s" alone as a formatter; instead say

strbuf_addstr(out, val.buf);

cf. contrib/coccinelle/strbuf.cocci

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Anders Waldenborg  writes:

> + else if (skip_prefix(arg, "key=", )) {
> + const char *end = arg + strcspn(arg, 
> ",)");
> +
> + if (opts.filter_key)
> + free(opts.filter_key);

Call the free() unconditionally, cf. contrib/coccinelle/free.cocci.

Re: git-rebase is ignoring working-tree-encoding

[Adrián Gimeno Balaguer]

El dom., 4 nov. 2018 a las 18:07, Torsten Bögershausen
() escribió:
>
> Thanks for the report.
> I have tried to follow the problem from your verbal descriptions
> (and the PR) but I need to admit that I don't fully understand the
> problem (yet).

I have created a PR in the Git's repository. You can read an updated
description there:

https://github.com/git/git/pull/550

> Could you try to create some instructions how to reproduce it?
> A numer of shell instructions would be great,
> in best case some kind of "test case", like the tests in
> the t/ directory in Git.
>
> It would be nice to be able to re-produce it.
> And if there is a bug, to get it fixed.

This is covered in the mentioned PR above. Thanks for feedback.

-- 
Adrián

Re: [PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key

[Michał Górny]

On Mon, 2018-11-05 at 10:08 +0900, Junio C Hamano wrote:
> Michał Górny  writes:
> 
> > > It's my understanding that GnuPG will use the most recent subkey
> > > suitable for a particular purpose, and I think the test relies on that
> > > behavior.  However, I'm not sure that's documented.  Do we want to rely
> > > on that behavior or be more explicit?  (This is a question, not an
> > > opinion.)
> > 
> > To be honest, I don't recall which suitable subkey is used.  However, it
> > definitely will prefer a subkey with signing capabilities over
> > the primary key if one is present, and this is well-known and expected
> > behavior.
> > 
> > In fact, if you have a key with two signing subkeys A and B and it
> > considers A better, then even if you explicitly pass keyid of B, it will
> > use A.  To force another subkey you have to append '!' to keyid.
> > 
> > Therefore, I think this is a behavior we can rely on.
> 
> I didn't check how the signing key configuration is done in the test
> sript (which is outside the patch context), but do you mean that we
> create these signed objects by specifying which key to use with a
> keyid with "!"  appended?  If so I agree that would make sense,
> because we would then know which subkey should be used for signing
> and checking with %GF/%GP would be a good way to do so.
> 

No, we don't have duplicate subkeys to be required to use that.  Some of
the tests use explicit '-S' to force using the other key; other
seem to use a default key (I can't find a place where the default would
be set, so I suppose it's GnuPG default).

-- 
Best regards,
Michał Górny


signature.asc
Description: This is a digitally signed message part

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Junio C Hamano]

Eric Sunshine  writes:

> On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano  wrote:
>> OK, thanks.  It seems that the relative silence after this message
>> is a sign that the resulting patch after squashing is what everybody
>> is happey with?
>>
>> -- >8 --
>> From: Steve Hoelzer 
>>
>> Signed-off-by: Johannes Sixt 
>> Acked-by: Steve Hoelzer 
>
> It's not clear from this who the author is.

Right.  The latter should be s-o-b and the order swapped, and
probably say "Steve wrote the original version, Johannes extended
it" in the text to match.

THanks.

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Eric Sunshine]

On Sun, Nov 4, 2018 at 10:48 PM Junio C Hamano  wrote:
> Eric Sunshine  writes:
> > Does the user have to include the colon when specifying  of
> > 'key='?
> > Does 'key=', do a full or partial match on trailers?
> > What happens if 'key=...' is specified multiple times?
> > Thinking further on the last two points, should  be a regular 
> > expression?
>
> Another thing that needs to be clarified in the document would be
> case sensitivity.  People sometimes spell "Signed-Off-By:" by
> mistake (or is it by malice?).

The documentation does say parenthetically "(matching is done
case-insensitively)", so I think that's already covered. Or did you
have something else in mind?

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Junio C Hamano]

Eric Sunshine  writes:

> Does the user have to include the colon when specifying  of
> 'key='? I can see from peeking at the implementation that the
> colon must not be used, but this should be documented. Should the code
> tolerate a trailing colon? (Genuine question; it's easy to do and
> would be more user-friendly.)
>
> Does 'key=', do a full or partial match on trailers? And, if
> partial, is the match anchored at the start or can it match anywhere
> in the trailer key? I see from the implementation that it does a full
> match, but this behavior should be documented.
>
> What happens if 'key=...' is specified multiple times? Are the
> multiple keys conjunctive? Disjunctive? Last-wins? I can see from the
> implementation that it is last-wins, but this behavior should be
> documented. (I wonder how painful it will be for people who want to
> match multiple keys. This doesn't have to be answered yet, as the
> behavior can always be loosened later to allow multiple-key matching
> since the current syntax does not disallow such expansion.)
>
> Thinking further on the last two points, should  be a regular expression?

Another thing that needs to be clarified in the document would be
case sensitivity.  People sometimes spell "Signed-Off-By:" by
mistake (or is it by malice?).

I do suspect that the parser should just make a list of sought-after
keys, not doing "last-one-wins", as that won't be very difficult to
do and makes what happens when given multiple keys trivially obvious.

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Eric Sunshine]

On Sun, Nov 4, 2018 at 6:26 PM Junio C Hamano  wrote:
> OK, thanks.  It seems that the relative silence after this message
> is a sign that the resulting patch after squashing is what everybody
> is happey with?
>
> -- >8 --
> From: Steve Hoelzer 
>
> Signed-off-by: Johannes Sixt 
> Acked-by: Steve Hoelzer 

It's not clear from this who the author is.

Re: [PATCH v5 00/12] Base SHA-256 implementation

[Junio C Hamano]

"brian m. carlson"  writes:

> This series provides a functional SHA-256 implementation and wires it
> up, along with some housekeeping patches to make it suitable for
> testing.
>
> Changes from v4:
> * Downcase hex constants for consistency.
> * Remove needless parentheses in return statement.
> * Remove braces for single statement loops.
> * Switch to +=.
> * Add references to rationale for SHA-256.
> * Remove inclusion of "git-compat-util.h" in header.

You ended up not doing the last one, judging from the interdiff
below.  I think it is OK to leave the header in.

Thanks, will replace.

Re: Design of multiple hash support

[Junio C Hamano]

"brian m. carlson"  writes:

> I'm currently working on getting Git to support multiple hash algorithms
> in the same binary (SHA-1 and SHA-256).  In order to have a fully
> functional binary, we'll need to have some way of indicating to certain
> commands (such as init and show-index) that they should assume a certain
> hash algorithm.
>
> There are basically two approaches I can take.  The first is to provide
> each command that needs to learn about this with its own --hash
> argument.  So we'd have:
>
>   git init --hash=sha256
>   git show-index --hash=sha256 
> The other alternative is that we provide a global option to git, which
> is parsed by all programs, like so:
>
>   git --hash=sha256 init
>   git --hash=sha256 show-index 

Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Junio C Hamano]

Anders Waldenborg  writes:

> @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* 
> in UTF-8 */
>   arg++;
>  
>   opts.only_trailers = 1;
> + } else if (skip_prefix(arg, "separator=", 
> )) {
> + size_t seplen = strcspn(arg, ",)");
> + strbuf_reset();
> + char *fmt = xstrndup(arg, seplen);
> + strbuf_expand(, fmt, 
> format_fundamental, NULL);

This somehow feels akin to using end-user supplied param to printf(3)
as its format argument e.g.

int main(int ac, char *av) {
printf(av[1]);
return 0;
}

which is not a good idea.  Is there a mechanism with which we can
ensure that the separator= specification will never come from
potentially malicious sources (e.g. not used to show things on webpage
allowing random folks who access he site to supply custom format)?

Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function

[Junio C Hamano]

Anders Waldenborg  writes:

> No functional change intended
>
> Signed-off-by: Anders Waldenborg 
> ---
>  pretty.c | 37 ++---
>  1 file changed, 26 insertions(+), 11 deletions(-)

I do not think "fundamental" is the best name for this, but I agree
that it would be useful to split the helpers into one that is
"constant across commits" and the other one that is "per commit".

> diff --git a/pretty.c b/pretty.c
> index f87ba4f18..9fdddce9d 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, 
> const char *candidate,
>   return 0;
>  }
>  
> +static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */
> +  const char *placeholder,
> +  void *context)
> +{
> + int ch;
> +
> + switch (placeholder[0]) {
> + case 'n':   /* newline */
> + strbuf_addch(sb, '\n');
> + return 1;
> + case 'x':
> + /* %x00 == NUL, %x0a == LF, etc. */
> + ch = hex2chr(placeholder + 1);
> + if (ch < 0)
> + return 0;
> + strbuf_addch(sb, ch);
> + return 3;
> + }
> + return 0;
> +}
> +
>  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>   const char *placeholder,
>   void *context)
> @@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* 
> in UTF-8 */
>   const char *msg = c->message;
>   struct commit_list *p;
>   const char *arg;
> - int ch;
> + size_t res;
>  
>   /* these are independent of the commit */
> + res = format_fundamental(sb, placeholder, NULL);
> + if (res)
> + return res;
> +
>   switch (placeholder[0]) {
>   case 'C':
>   if (starts_with(placeholder + 1, "(auto)")) {
> @@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* 
> in UTF-8 */
>*/
>   return ret;
>   }
> - case 'n':   /* newline */
> - strbuf_addch(sb, '\n');
> - return 1;
> - case 'x':
> - /* %x00 == NUL, %x0a == LF, etc. */
> - ch = hex2chr(placeholder + 1);
> - if (ch < 0)
> - return 0;
> - strbuf_addch(sb, ch);
> - return 3;
>   case 'w':
>   if (placeholder[1] == '(') {
>   unsigned long width = 0, indent1 = 0, indent2 = 0;

Re: [PATCH v4 2/5] am: improve author-script error reporting

[Junio C Hamano]

Eric Sunshine  writes:

> Junio labeled the "-2" trick "cute", and while it is optimal in that
> it only traverses the key/value list once, it also increases cognitive
> load since the reader has to spend a good deal more brain cycles
> understanding what is going on than would be the case with simpler
> (and less noisily repetitive) code.

... and update three copies of very similar looking code that have
to stay similar.  If this were parsing unbounded and customizable
set of variables, I think the approach you suggest to use a helper
that iterates over the whole array for each key makes sense, but for
now I think what was queued would be OK.

> An alternative would be to make the code trivially easy to understand,
> though a bit more costly, but that expense should be negligible since
> this file should always be tiny, containing very few key/value pairs,
> and it's not timing critical code anyhow. For instance:
>
> static char *find(struct string_list *kv, const char *key)
> {
> const char *val = NULL;
> int i;
> for (i = 0; i < kv.nr; i++) {
> if (!strcmp(kv.items[i].string, key)) {
> if (val) {
> error(_("duplicate %s"), key);
> return NULL;
> }
> val = kv.items[i].util;
> }
> }
> if (!val)
> error(_("missing %s"), key);
> return val;
> }
>
> static int read_author_script(struct am_state *state)
> {
> ...
> char *name, *email, *date;
> ...
> name = find(, "GIT_AUTHOR_NAME");
> email = find(, "GIT_AUTHOR_EMAIL");
> date = find(, "GIT_AUTHOR_DATE");
> if (name && email && date) {
> state->author_name = name;
> state->author_email = email;
> state->author_date = date;
> retval = 0;
> }
> string_list_clear, !!retval);
> ...

Re: [PATCH v2] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> tree_entry_interesting() is used for matching pathspec on a tree. The
> interesting thing about this function is that, because the tree
> entries are known to be sorted, this function can return more than
> just "yes, matched" and "no, not matched". It can also say "yes, this
> entry is matched and so is the remaining entries in the tree".
>
> This is where I made a mistake when matching exclude pathspec. For
> exclude pathspec, we do matching twice, one with positive patterns and
> one with negative ones, then a rule table is applied to determine the
> final "include or exclude" result. Note that "matched" does not
> necessarily mean include. For negative patterns, "matched" means
> exclude.
>
> This particular rule is too eager to include everything. Rule 8 says
> that "if all entries are positively matched" and the current entry is
> not negatively matched (i.e. not excluded), then all entries are
> positively matched and therefore included. But this is not true. If
> the _current_ entry is not negatively matched, it does not mean the
> next one will not be and we cannot conclude right away that all
> remaining entries are positively matched and can be included.
>
> Rules 8 and 18 are now updated to be less eager. We conclude that the
> current entry is positively matched and included. But we say nothing
> about remaining entries. tree_entry_interesting() will be called again
> for those entries where we will determine entries individually.

Thanks.  Will queue.

> Reported-by: Christophe Bliard 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  v2 fixes the too broad "git add ." in the test
>
>  t/t6132-pathspec-exclude.sh | 17 +
>  tree-walk.c | 11 ---
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh
> index eb829fce97..2462b19ddd 100755
> --- a/t/t6132-pathspec-exclude.sh
> +++ b/t/t6132-pathspec-exclude.sh
> @@ -194,4 +194,21 @@ test_expect_success 'multiple exclusions' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 't_e_i() exclude case #8' '
> + git init case8 &&
> + (
> + cd case8 &&
> + echo file >file1 &&
> + echo file >file2 &&
> + git add file1 file2 &&
> + git commit -m twofiles &&
> + git grep -l file HEAD :^file2 >actual &&
> + echo HEAD:file1 >expected &&
> + test_cmp expected actual &&
> + git grep -l file HEAD :^file1 >actual &&
> + echo HEAD:file2 >expected &&
> + test_cmp expected actual
> + )
> +'
> +
>  test_done
> diff --git a/tree-walk.c b/tree-walk.c
> index 77b37f36fa..79bafbd1a2 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -1107,7 +1107,7 @@ enum interesting tree_entry_interesting(const struct 
> name_entry *entry,
>*   5  |  file |1 |1 |   0
>*   6  |  file |1 |2 |   0
>*   7  |  file |2 |   -1 |   2
> -  *   8  |  file |2 |0 |   2
> +  *   8  |  file |2 |0 |   1
>*   9  |  file |2 |1 |   0
>*  10  |  file |2 |2 |  -1
>* -+---+--+--+---
> @@ -1118,7 +1118,7 @@ enum interesting tree_entry_interesting(const struct 
> name_entry *entry,
>*  15  |  dir  |1 |1 |   1 (*)
>*  16  |  dir  |1 |2 |   0
>*  17  |  dir  |2 |   -1 |   2
> -  *  18  |  dir  |2 |0 |   2
> +  *  18  |  dir  |2 |0 |   1
>*  19  |  dir  |2 |1 |   1 (*)
>*  20  |  dir  |2 |2 |  -1
>*
> @@ -1134,7 +1134,12 @@ enum interesting tree_entry_interesting(const struct 
> name_entry *entry,
>  
>   negative = do_match(entry, base, base_offset, ps, 1);
>  
> - /* #3, #4, #7, #8, #13, #14, #17, #18 */
> + /* #8, #18 */
> + if (positive == all_entries_interesting &&
> + negative == entry_not_interesting)
> + return entry_interesting;
> +
> + /* #3, #4, #7, #13, #14, #17 */
>   if (negative <= entry_not_interesting)
>   return positive;

Re: [PATCH] sequencer.c: remove a stray semicolon

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  On top of ag/rebase-i-in-c

Thanks.

Re: [PATCH] git-worktree.txt: correct linkgit command name

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Noticed-by: SZEDER Gábor 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  On top of nd/per-worktree-ref-iteration

Thanks.

>
>  Documentation/git-worktree.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 9117e4fb50..69d55f1350 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -230,7 +230,7 @@ GIT_COMMON_DIR/worktrees/foo/HEAD and
>  GIT_COMMON_DIR/worktrees/bar/refs/bisect/bad.
>  
>  To access refs, it's best not to look inside GIT_DIR directly. Instead
> -use commands such as linkgit:git-revparse[1] or linkgit:git-update-ref[1]
> +use commands such as linkgit:git-rev-parse[1] or linkgit:git-update-ref[1]
>  which will handle refs correctly.
>  
>  DETAILS

Re: [PATCH v2] build: link with curl-defined linker flags

[Junio C Hamano]

James Knight  writes:

> Changes v1 -> v2:
>  - Improved support for detecting curl linker flags when not using a
> configure-based build (mentioned by Junio C Hamano).
>  - Adding a description on how to explicitly use the CURL_LDFLAGS
> define when not using configure (suggested by Junio C Hamano).
>
> The original patch made a (bad) assumption that builds would always
> invoke ./configure before attempting to build Git. To support a
> make-invoked build, CURL_LDFLAGS can also be populated using the defined
> curl-config utility. This change also comes with the assumption that
> since both ./configure and Makefile are using curl-config to determine
> aspects of the build, it should be also fine to use the same utility to
> provide the linker flags to compile against (please indicate so if this
> is another bad assumption). With this change, the explicit configuration
> of CURL_LDFLAGS inside config.mak.uname for Minix and NONSTOP_KERNEL
> have been dropped.

Will replace; thanks.


>  Makefile | 30 +++---
>  config.mak.uname |  3 ---
>  configure.ac | 17 +++--
>  3 files changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 
> b08d5ea258c69a78745dfa73fe698c11d021858a..36da17dc1f9b1a70c9142604afe989f1eb8ee87f
>  100644
> --- a/Makefile
> +++ b/Makefile
> @@ -59,6 +59,13 @@ all::
>  # Define CURL_CONFIG to curl's configuration program that prints information
>  # about the library (e.g., its version number).  The default is 
> 'curl-config'.
>  #
> +# Define CURL_LDFLAGS to specify flags that you need to link when using 
> libcurl,
> +# if you do not want to rely on the libraries provided by CURL_CONFIG.  The
> +# default value is a result of `curl-config --libs`.  An example value for
> +# CURL_LDFLAGS is as follows:
> +#
> +# CURL_LDFLAGS=-lcurl
> +#
>  # Define NO_EXPAT if you do not have expat installed.  git-http-push is
>  # not built, and you cannot push using http:// and https:// transports 
> (dumb).
>  #
> @@ -183,10 +190,6 @@ all::
>  #
>  # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto 
> (Darwin).
>  #
> -# Define NEEDS_SSL_WITH_CURL if you need -lssl with -lcurl (Minix).
> -#
> -# Define NEEDS_IDN_WITH_CURL if you need -lidn when using -lcurl (Minix).
> -#
>  # Define NEEDS_LIBICONV if linking with libc is not enough (Darwin).
>  #
>  # Define NEEDS_LIBINTL_BEFORE_LIBICONV if you need libintl before libiconv.
> @@ -1305,20 +1308,17 @@ else
>   ifdef CURLDIR
>   # Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
>   BASIC_CFLAGS += -I$(CURLDIR)/include
> - CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
> $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl
> + CURL_LIBCURL = -L$(CURLDIR)/$(lib) 
> $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
>   else
> - CURL_LIBCURL = -lcurl
> - endif
> - ifdef NEEDS_SSL_WITH_CURL
> - CURL_LIBCURL += -lssl
> - ifdef NEEDS_CRYPTO_WITH_SSL
> - CURL_LIBCURL += -lcrypto
> - endif
> - endif
> - ifdef NEEDS_IDN_WITH_CURL
> - CURL_LIBCURL += -lidn
> + CURL_LIBCURL =
>   endif
>  
> +ifdef CURL_LDFLAGS
> + CURL_LIBCURL += $(CURL_LDFLAGS)
> +else
> + CURL_LIBCURL += $(shell $(CURL_CONFIG) --libs)
> +endif
> +
>   REMOTE_CURL_PRIMARY = git-remote-http$X
>   REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X 
> git-remote-ftps$X
>   REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
> diff --git a/config.mak.uname b/config.mak.uname
> index 
> 8acdeb71fdab3b3e8e3c536110eb6de13f14e8ff..19e6633040b1db4a148d7b33c4e9d374fe7f87ba
>  100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -431,8 +431,6 @@ ifeq ($(uname_S),Minix)
>   NO_NSEC = YesPlease
>   NEEDS_LIBGEN =
>   NEEDS_CRYPTO_WITH_SSL = YesPlease
> - NEEDS_IDN_WITH_CURL = YesPlease
> - NEEDS_SSL_WITH_CURL = YesPlease
>   NEEDS_RESOLV =
>   NO_HSTRERROR = YesPlease
>   NO_MMAP = YesPlease
> @@ -458,7 +456,6 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>   # Missdetected, hence commented out, see below.
>   #NO_CURL = YesPlease
>   # Added manually, see above.
> - NEEDS_SSL_WITH_CURL = YesPlease
>   HAVE_LIBCHARSET_H = YesPlease
>   HAVE_STRINGS_H = YesPlease
>   NEEDS_LIBICONV = YesPlease
> diff --git a/configure.ac b/configure.ac
> index 
> e11b7976ab1c93d8ccec2e499d0093db42551059..44e8c036b6ec417e95ca4e5c2861785900d8634c
>  100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -600,17 +600,14 @@ AC_CHECK_PROG([CURL_CONFIG], [curl-config],
>  
>  if test $CURL_CONFIG != no; then
>  GIT_CONF_SUBST([CURL_CONFIG])
> -if test -z "${NO_OPENSSL}"; then
> -  AC_MSG_CHECKING([if Curl supports SSL])
> -  if test $(curl-config --features|grep SSL) = SSL; then
> - NEEDS_SSL_WITH_CURL=YesPlease
> - AC_MSG_RESULT([yes])
> -  else
> - 

Re: "git checkout" safety feature

[Junio C Hamano]

Matthias Urlichs  writes:

> A recent discussion on LWN https://lwn.net/Articles/770642/ noted that
> "git checkout  " does not warn if one if the files has
> been modified locally, nor is there an option to do so.
>
> IMHO that should be fixed, preferably by somebody who knows git's
> internals well enough to do so in half an hour ;-)

"git checkout  " is a feature to overwrite local
changes.  It is what you use when you make a mess editing the files
and want to go back to a known state.  Why should that feature be
destroyed?

Re: [PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key

[Junio C Hamano]

Michał Górny  writes:

>> It's my understanding that GnuPG will use the most recent subkey
>> suitable for a particular purpose, and I think the test relies on that
>> behavior.  However, I'm not sure that's documented.  Do we want to rely
>> on that behavior or be more explicit?  (This is a question, not an
>> opinion.)
>
> To be honest, I don't recall which suitable subkey is used.  However, it
> definitely will prefer a subkey with signing capabilities over
> the primary key if one is present, and this is well-known and expected
> behavior.
>
> In fact, if you have a key with two signing subkeys A and B and it
> considers A better, then even if you explicitly pass keyid of B, it will
> use A.  To force another subkey you have to append '!' to keyid.
>
> Therefore, I think this is a behavior we can rely on.

I didn't check how the signing key configuration is done in the test
sript (which is outside the patch context), but do you mean that we
create these signed objects by specifying which key to use with a
keyid with "!"  appended?  If so I agree that would make sense,
because we would then know which subkey should be used for signing
and checking with %GF/%GP would be a good way to do so.

Thanks.

Design of multiple hash support

[brian m. carlson]

I'm currently working on getting Git to support multiple hash algorithms
in the same binary (SHA-1 and SHA-256).  In order to have a fully
functional binary, we'll need to have some way of indicating to certain
commands (such as init and show-index) that they should assume a certain
hash algorithm.

There are basically two approaches I can take.  The first is to provide
each command that needs to learn about this with its own --hash
argument.  So we'd have:

  git init --hash=sha256
  git show-index --hash=sha256 https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> A reverted commit will have a new trailer
>
> Revert: 

Please don't, unless you are keeping the current "the effect of
commit X relative to its parent Y was reverted" writtein in prose,
which is meant to be followed up with a manually written "because
..." and adding this as an extra footer that is meant solely for
machine consumption.  Of course reversion of a merge needs to say
relative to which parent of the merge it is undoing.

> Similarly a cherry-picked commit with -x will have
>
> Cherry-Pick: 

Unlike the "revert" change above, this probably is a good change, as
a"(cherry-pickt-from: X)" does not try to convey anything more or
anything less than such a standard looking trailer and it is in
different shape only by historical accident.  But people's scripts
may need to have a long transition period for this change to happen.


>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  I think standardizing how we record commit ids in the commit message
>  is a good idea. Though to be honest this started because of my irk of
>  an English string "cherry picked from..." that cannot be translated.
>  It might as well be a computer language that happens to look like
>  English.
>
>  Documentation/git-cherry-pick.txt |  5 ++---
>  sequencer.c   | 20 ++--
>  t/t3510-cherry-pick-sequence.sh   | 12 ++--
>  t/t3511-cherry-pick-x.sh  | 26 +-
>  4 files changed, 27 insertions(+), 36 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt 
> b/Documentation/git-cherry-pick.txt
> index d35d771fc8..8ffbaed1a0 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -58,9 +58,8 @@ OPTIONS
>   message prior to committing.
>  
>  -x::
> - When recording the commit, append a line that says
> - "(cherry picked from commit ...)" to the original commit
> - message in order to indicate which commit this change was
> + When recording the commit, append "Cherry-Pick:" trailer line
> + recording the commit name which commit this change was
>   cherry-picked from.  This is done only for cherry
>   picks without conflicts.  Do not use this option if
>   you are cherry-picking from your private branch because
> diff --git a/sequencer.c b/sequencer.c
> index 9e1ab3a2a7..f7318f2862 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -36,7 +36,6 @@
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
>  const char sign_off_header[] = "Signed-off-by: ";
> -static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>  
>  GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>  
> @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   base_label = msg.label;
>   next = parent;
>   next_label = msg.parent_label;
> - strbuf_addstr(, "Revert \"");
> - strbuf_addstr(, msg.subject);
> - strbuf_addstr(, "\"\n\nThis reverts commit ");
> - strbuf_addstr(, oid_to_hex(>object.oid));
> -
> - if (commit->parents && commit->parents->next) {
> - strbuf_addstr(, ", reversing\nchanges made to ");
> - strbuf_addstr(, oid_to_hex(>object.oid));
> - }
> - strbuf_addstr(, ".\n");
> + strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
> +
> + strbuf_addf(, "Revert: %s\n",
> + oid_to_hex(>object.oid));
>   } else {
>   const char *p;
>  
> @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   strbuf_complete_line();
>   if (!has_conforming_footer(, NULL, 0))
>   strbuf_addch(, '\n');
> - strbuf_addstr(, cherry_picked_prefix);
> - strbuf_addstr(, oid_to_hex(>object.oid));
> - strbuf_addstr(, ")\n");
> + strbuf_addf(, "Cherry-Pick: %s\n",
> + oid_to_hex(>object.oid));
>   }
>   if (!is_fixup(command))
>   author = get_author(msg.message);
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index c84eeefdc9..89b6e7fc1e 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '
>   git cat-file commit HEAD~1 >picked_msg &&
>   git cat-file commit HEAD~2 >unrelatedpick_msg &&
>   git cat-file commit HEAD~3 >initial_msg &&
> - ! grep "cherry picked from" initial_msg &&
> - grep "cherry picked from" unrelatedpick_msg &&
> - grep "cherry picked from" picked_msg &&
> - grep "cherry picked from" anotherpick_msg
> + ! grep "Cherry-Pick:" initial_msg &&
> +   

Re: [RFC PATCH] checkout: add synonym of -b

[Junio C Hamano]

Mikkel Kjeldsen  writes:

> Add --new-branch as a long-form synonym of -b. I occasionally encounter
> some confusion in new users having interpreted "checkout -b" to mean
> "checkout branch", or internalized it as "the way to create a new
> branch" rather than merely a convenience for "branch && checkout". I
> think an explicit long-form can help alleviate that.
>
> Signed-off-by: Mikkel Kjeldsen 
> ---
>
> Notes:
> This makes the synopsis and description lines look a little clumsy (and
> I think incorrect...?) so if this proposal is accepted perhaps those
> parts are better left out. It is meant more for training and
> documentation than regular usage, anyway.

Sounds like even you (who wrote this patch) expect the long form
option to be impractical for regular usage and everybody would end
up using the -b form?

I am borderline "Meh" on this change, slightly on the negative side,
primarily because we'd need to worry about what to do with "-B" if
we did this to "-b", and I do not think it is worth even spending
brain cycles to worry about it (e.g. should it be
--force-new-branch?  should --new-branch used together with --force
a better-looking alternative?  if we were to add --force, how should
it interact with other existing options the command support? etc.)

But let's hear what others think.

Thanks.

Re: [PATCH v3] commit: add a commit.allowEmpty config variable

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> I.e. you seemingly have no interest in using "git commit" to produce
> empty commits, but are just trying to cherry-pick something and it's
> failing because it (presumably, or am I missing something) cherry picks
> an existing commit content ends up not changing anything.
>
> I.e. you'd like to make the logic 37f7a85793 ("Teach commit about
> CHERRY_PICK_HEAD", 2011-02-19) added a message for the default.
>
> So let's talk about that use case, and for those of us less familiar
> with this explain why it is that this needs to still be optional at
> all. I.e. aren't we just exposing an implementation detail here where
> cherry-pick uses the commit machinery? Should we maybe just always pass
> --allow-empty on cherry-pick, if not why not?
>
> I can think of some reasons, but the above is a hint that both this
> patch + the current documentation which talks about "foreign SCM
> scripts" have drifted very far from what this is actually being used
> for, so let's update that.

The command line "--allowAnything" in general is meant to be an
escape hatch for unusual situations, and if a workflow requires
constant use of that escape hatch, there is something wrong either
in the workflow or in the tool used in the workflow, and it is what
we should first see if we can fix, I would think, before making it
easy to constantly use the escape hatch.

I didn't look at the external reference you looked at but it sounds
like your review comment is taking the topic in the right direction.

Thanks for digging for the backstory.  

Re: [RFC v1] Add virtual file system settings and hook proc

[Junio C Hamano]

Ben Peart  writes:

>>> +   if (*dtype == DT_UNKNOWN)
>>> +   *dtype = get_dtype(NULL, istate, pathname, pathlen);
>>
>> We try to defer paying cost to determine unknown *dtype as late as
>> possible by having this call in last_exclude_matching_from_list(),
>> and not here.  If we are doing this, we probably should update the
>> callpaths that call last_exclude_matching_from_list() to make the
>> caller responsible for doing get_dtype() and drop the lazy finding
>> of dtype from the callee.  Alternatively, the new "is excluded from
>> vfs" helper can learn to do the lazy get_dtype() just like the
>> existing last_exclude_matching_from_list() does.  I suspect the
>> latter may be simpler.
>
> In is_excluded_from_virtualfilesystem() dtype can't be lazy because it
> is always needed (which is why I test and die if it isn't known).  

You make a call to that function even when virtual-file-system hook
is not in use, i.e. instead of the caller saying

if (is_vfs_in_use()) {
*dtype = get_dtype(...);
if (is_excluded_from_vfs(...) > 0)
return 1;
}

your caller makes an unconditional call to is_excluded_from_vfs().
Isn't that the only reason why you break the laziness of determining
dtype?

You can keep the caller simple by making an unconditional call, but
maintain the laziness by updating the callig convention to pass
dtype (not *dtype) to the function, e.g..

if (is_excluded_from_vfs(pathname, pathlen, dtype) > 0)
return 1;

and then at the beginning of the helper

if (is_vfs_in_use())
return -1; /* undetermined */
*dtype = get_dtype(...);
... whatever logic it has now ...

no?

> Your comments are all feedback on the code - how it was implemented,
> style, etc.  Any thoughts on whether this is something we could/should
> merge into master (after any necessary cleanup)?  Would anyone else
> find this interesting/helpful?

I am pretty much neutral.  Not strongly opposed to it, but not all
that interested until seeing its integration with the "userland" to
see how the whole thing works ;-)

[PATCH v5 09/12] commit-graph: convert to using the_hash_algo

[brian m. carlson]

Instead of using hard-coded constants for object sizes, use
the_hash_algo to look them up.  In addition, use a function call to look
up the object ID version and produce the correct value.  For now, we use
version 1, which means to use the default algorithm used in the rest of
the repository.

Signed-off-by: brian m. carlson 
---
 commit-graph.c | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 40c855f185..6763d19288 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -23,16 +23,11 @@
 #define GRAPH_CHUNKID_DATA 0x43444154 /* "CDAT" */
 #define GRAPH_CHUNKID_LARGEEDGES 0x45444745 /* "EDGE" */
 
-#define GRAPH_DATA_WIDTH 36
+#define GRAPH_DATA_WIDTH (the_hash_algo->rawsz + 16)
 
 #define GRAPH_VERSION_1 0x1
 #define GRAPH_VERSION GRAPH_VERSION_1
 
-#define GRAPH_OID_VERSION_SHA1 1
-#define GRAPH_OID_LEN_SHA1 GIT_SHA1_RAWSZ
-#define GRAPH_OID_VERSION GRAPH_OID_VERSION_SHA1
-#define GRAPH_OID_LEN GRAPH_OID_LEN_SHA1
-
 #define GRAPH_OCTOPUS_EDGES_NEEDED 0x8000
 #define GRAPH_PARENT_MISSING 0x7fff
 #define GRAPH_EDGE_LAST_MASK 0x7fff
@@ -44,13 +39,18 @@
 #define GRAPH_FANOUT_SIZE (4 * 256)
 #define GRAPH_CHUNKLOOKUP_WIDTH 12
 #define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
-   + GRAPH_FANOUT_SIZE + GRAPH_OID_LEN)
+   + GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)
 
 char *get_commit_graph_filename(const char *obj_dir)
 {
return xstrfmt("%s/info/commit-graph", obj_dir);
 }
 
+static uint8_t oid_version(void)
+{
+   return 1;
+}
+
 static struct commit_graph *alloc_commit_graph(void)
 {
struct commit_graph *g = xcalloc(1, sizeof(*g));
@@ -125,15 +125,15 @@ struct commit_graph *load_commit_graph_one(const char 
*graph_file)
}
 
hash_version = *(unsigned char*)(data + 5);
-   if (hash_version != GRAPH_OID_VERSION) {
+   if (hash_version != oid_version()) {
error(_("hash version %X does not match version %X"),
- hash_version, GRAPH_OID_VERSION);
+ hash_version, oid_version());
goto cleanup_fail;
}
 
graph = alloc_commit_graph();
 
-   graph->hash_len = GRAPH_OID_LEN;
+   graph->hash_len = the_hash_algo->rawsz;
graph->num_chunks = *(unsigned char*)(data + 6);
graph->graph_fd = fd;
graph->data = graph_map;
@@ -149,7 +149,7 @@ struct commit_graph *load_commit_graph_one(const char 
*graph_file)
 
chunk_lookup += GRAPH_CHUNKLOOKUP_WIDTH;
 
-   if (chunk_offset > graph_size - GIT_MAX_RAWSZ) {
+   if (chunk_offset > graph_size - the_hash_algo->rawsz) {
error(_("improper chunk offset %08x%08x"), 
(uint32_t)(chunk_offset >> 32),
  (uint32_t)chunk_offset);
goto cleanup_fail;
@@ -764,6 +764,7 @@ void write_commit_graph(const char *obj_dir,
int num_extra_edges;
struct commit_list *parent;
struct progress *progress = NULL;
+   const unsigned hashsz = the_hash_algo->rawsz;
 
if (!commit_graph_compatible(the_repository))
return;
@@ -909,7 +910,7 @@ void write_commit_graph(const char *obj_dir,
hashwrite_be32(f, GRAPH_SIGNATURE);
 
hashwrite_u8(f, GRAPH_VERSION);
-   hashwrite_u8(f, GRAPH_OID_VERSION);
+   hashwrite_u8(f, oid_version());
hashwrite_u8(f, num_chunks);
hashwrite_u8(f, 0); /* unused padding byte */
 
@@ -924,8 +925,8 @@ void write_commit_graph(const char *obj_dir,
 
chunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;
chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
-   chunk_offsets[2] = chunk_offsets[1] + GRAPH_OID_LEN * commits.nr;
-   chunk_offsets[3] = chunk_offsets[2] + (GRAPH_OID_LEN + 16) * commits.nr;
+   chunk_offsets[2] = chunk_offsets[1] + hashsz * commits.nr;
+   chunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * commits.nr;
chunk_offsets[4] = chunk_offsets[3] + 4 * num_extra_edges;
 
for (i = 0; i <= num_chunks; i++) {
@@ -938,8 +939,8 @@ void write_commit_graph(const char *obj_dir,
}
 
write_graph_chunk_fanout(f, commits.list, commits.nr);
-   write_graph_chunk_oids(f, GRAPH_OID_LEN, commits.list, commits.nr);
-   write_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);
+   write_graph_chunk_oids(f, hashsz, commits.list, commits.nr);
+   write_graph_chunk_data(f, hashsz, commits.list, commits.nr);
write_graph_chunk_large_edges(f, commits.list, commits.nr);
 
close_commit_graph(the_repository);

[PATCH v5 12/12] hash: add an SHA-256 implementation using OpenSSL

[brian m. carlson]

We already have OpenSSL routines available for SHA-1, so add routines
for SHA-256 as well.

On a Core i7-6600U, this SHA-256 implementation compares favorably to
the SHA1DC SHA-1 implementation:

SHA-1: 157 MiB/s (64 byte chunks); 337 MiB/s (16 KiB chunks)
SHA-256: 165 MiB/s (64 byte chunks); 408 MiB/s (16 KiB chunks)

Signed-off-by: brian m. carlson 
---
 Makefile | 7 +++
 hash.h   | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/Makefile b/Makefile
index 5a07e03100..36fd3a149b 100644
--- a/Makefile
+++ b/Makefile
@@ -183,6 +183,8 @@ all::
 #
 # Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
 #
+# Define OPENSSL_SHA256 to use the SHA-256 routines in OpenSSL.
+#
 # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl (Darwin).
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
@@ -1638,6 +1640,10 @@ endif
 endif
 endif
 
+ifdef OPENSSL_SHA256
+   EXTLIBS += $(LIB_4_CRYPTO)
+   BASIC_CFLAGS += -DSHA256_OPENSSL
+else
 ifdef GCRYPT_SHA256
BASIC_CFLAGS += -DSHA256_GCRYPT
EXTLIBS += -lgcrypt
@@ -1645,6 +1651,7 @@ else
LIB_OBJS += sha256/block/sha256.o
BASIC_CFLAGS += -DSHA256_BLK
 endif
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
diff --git a/hash.h b/hash.h
index 2ef098052d..adde708cf2 100644
--- a/hash.h
+++ b/hash.h
@@ -17,6 +17,8 @@
 
 #if defined(SHA256_GCRYPT)
 #include "sha256/gcrypt.h"
+#elif defined(SHA256_OPENSSL)
+#include 
 #else
 #include "sha256/block/sha256.h"
 #endif

[PATCH v5 11/12] sha256: add an SHA-256 implementation using libgcrypt

[brian m. carlson]

Generally, one gets better performance out of cryptographic routines
written in assembly than C, and this is also true for SHA-256.  In
addition, most Linux distributions cannot distribute Git linked against
OpenSSL for licensing reasons.

Most systems with GnuPG will also have libgcrypt, since it is a
dependency of GnuPG.  libgcrypt is also faster than the SHA1DC
implementation for messages of a few KiB and larger.

For comparison, on a Core i7-6600U, this implementation processes 16 KiB
chunks at 355 MiB/s while SHA1DC processes equivalent chunks at 337
MiB/s.

In addition, libgcrypt is licensed under the LGPL 2.1, which is
compatible with the GPL.  Add an implementation of SHA-256 that uses
libgcrypt.

Signed-off-by: brian m. carlson 
---
 Makefile| 13 +++--
 hash.h  |  4 
 sha256/gcrypt.h | 30 ++
 3 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 sha256/gcrypt.h

diff --git a/Makefile b/Makefile
index e99b7712f6..5a07e03100 100644
--- a/Makefile
+++ b/Makefile
@@ -179,6 +179,10 @@ all::
 # in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
 # wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
 #
+# Define BLK_SHA256 to use the built-in SHA-256 routines.
+#
+# Define GCRYPT_SHA256 to use the SHA-256 routines in libgcrypt.
+#
 # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl (Darwin).
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto (Darwin).
@@ -1634,8 +1638,13 @@ endif
 endif
 endif
 
-LIB_OBJS += sha256/block/sha256.o
-BASIC_CFLAGS += -DSHA256_BLK
+ifdef GCRYPT_SHA256
+   BASIC_CFLAGS += -DSHA256_GCRYPT
+   EXTLIBS += -lgcrypt
+else
+   LIB_OBJS += sha256/block/sha256.o
+   BASIC_CFLAGS += -DSHA256_BLK
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
diff --git a/hash.h b/hash.h
index a9bc624020..2ef098052d 100644
--- a/hash.h
+++ b/hash.h
@@ -15,7 +15,11 @@
 #include "block-sha1/sha1.h"
 #endif
 
+#if defined(SHA256_GCRYPT)
+#include "sha256/gcrypt.h"
+#else
 #include "sha256/block/sha256.h"
+#endif
 
 #ifndef platform_SHA_CTX
 /*
diff --git a/sha256/gcrypt.h b/sha256/gcrypt.h
new file mode 100644
index 00..09bd8bb200
--- /dev/null
+++ b/sha256/gcrypt.h
@@ -0,0 +1,30 @@
+#ifndef SHA256_GCRYPT_H
+#define SHA256_GCRYPT_H
+
+#include 
+
+#define SHA256_DIGEST_SIZE 32
+
+typedef gcry_md_hd_t gcrypt_SHA256_CTX;
+
+inline void gcrypt_SHA256_Init(gcrypt_SHA256_CTX *ctx)
+{
+   gcry_md_open(ctx, GCRY_MD_SHA256, 0);
+}
+
+inline void gcrypt_SHA256_Update(gcrypt_SHA256_CTX *ctx, const void *data, 
size_t len)
+{
+   gcry_md_write(*ctx, data, len);
+}
+
+inline void gcrypt_SHA256_Final(unsigned char *digest, gcrypt_SHA256_CTX *ctx)
+{
+   memcpy(digest, gcry_md_read(*ctx, GCRY_MD_SHA256), SHA256_DIGEST_SIZE);
+}
+
+#define platform_SHA256_CTX gcrypt_SHA256_CTX
+#define platform_SHA256_Init gcrypt_SHA256_Init
+#define platform_SHA256_Update gcrypt_SHA256_Update
+#define platform_SHA256_Final gcrypt_SHA256_Final
+
+#endif

[PATCH v5 10/12] Add a base implementation of SHA-256 support

[brian m. carlson]

SHA-1 is weak and we need to transition to a new hash function.  For
some time, we have referred to this new function as NewHash.  Recently,
we decided to pick SHA-256 as NewHash.  The reasons behind the choice of
SHA-256 are outlined in the thread starting at [1] and in the commit
history for the hash function transition document.

Add a basic implementation of SHA-256 based off libtomcrypt, which is in
the public domain.  Optimize it and restructure it to meet our coding
standards.  Pull in the update and final functions from the SHA-1 block
implementation, as we know these function correctly with all compilers.
This implementation is slower than SHA-1, but more performant
implementations will be introduced in future commits.

Wire up SHA-256 in the list of hash algorithms, and add a test that the
algorithm works correctly.

Note that with this patch, it is still not possible to switch to using
SHA-256 in Git.  Additional patches are needed to prepare the code to
handle a larger hash algorithm and further test fixes are needed.

[1] 
https://public-inbox.org/git/20180609224913.gc38...@genre.crustytoothpaste.net/

Signed-off-by: brian m. carlson 
---
 Makefile   |   4 +
 cache.h|  12 ++-
 hash.h |  19 +++-
 sha1-file.c|  45 ++
 sha256/block/sha256.c  | 196 +
 sha256/block/sha256.h  |  26 ++
 t/helper/test-sha256.c |   7 ++
 t/helper/test-tool.c   |   1 +
 t/helper/test-tool.h   |   1 +
 t/t0015-hash.sh|  25 ++
 10 files changed, 332 insertions(+), 4 deletions(-)
 create mode 100644 sha256/block/sha256.c
 create mode 100644 sha256/block/sha256.h
 create mode 100644 t/helper/test-sha256.c

diff --git a/Makefile b/Makefile
index 68169a7abb..e99b7712f6 100644
--- a/Makefile
+++ b/Makefile
@@ -739,6 +739,7 @@ TEST_BUILTINS_OBJS += test-run-command.o
 TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
 TEST_BUILTINS_OBJS += test-sha1.o
 TEST_BUILTINS_OBJS += test-sha1-array.o
+TEST_BUILTINS_OBJS += test-sha256.o
 TEST_BUILTINS_OBJS += test-sigchain.o
 TEST_BUILTINS_OBJS += test-strcmp-offset.o
 TEST_BUILTINS_OBJS += test-string-list.o
@@ -1633,6 +1634,9 @@ endif
 endif
 endif
 
+LIB_OBJS += sha256/block/sha256.o
+BASIC_CFLAGS += -DSHA256_BLK
+
 ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
BASIC_CFLAGS += -DSHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)"
diff --git a/cache.h b/cache.h
index 9e5d1dd85a..48ce1565e6 100644
--- a/cache.h
+++ b/cache.h
@@ -48,11 +48,17 @@ unsigned long git_deflate_bound(git_zstream *, unsigned 
long);
 /* The block size of SHA-1. */
 #define GIT_SHA1_BLKSZ 64
 
+/* The length in bytes and in hex digits of an object name (SHA-256 value). */
+#define GIT_SHA256_RAWSZ 32
+#define GIT_SHA256_HEXSZ (2 * GIT_SHA256_RAWSZ)
+/* The block size of SHA-256. */
+#define GIT_SHA256_BLKSZ 64
+
 /* The length in byte and in hex digits of the largest possible hash value. */
-#define GIT_MAX_RAWSZ GIT_SHA1_RAWSZ
-#define GIT_MAX_HEXSZ GIT_SHA1_HEXSZ
+#define GIT_MAX_RAWSZ GIT_SHA256_RAWSZ
+#define GIT_MAX_HEXSZ GIT_SHA256_HEXSZ
 /* The largest possible block size for any supported hash. */
-#define GIT_MAX_BLKSZ GIT_SHA1_BLKSZ
+#define GIT_MAX_BLKSZ GIT_SHA256_BLKSZ
 
 struct object_id {
unsigned char hash[GIT_MAX_RAWSZ];
diff --git a/hash.h b/hash.h
index 1bcf7ab6fd..a9bc624020 100644
--- a/hash.h
+++ b/hash.h
@@ -15,6 +15,8 @@
 #include "block-sha1/sha1.h"
 #endif
 
+#include "sha256/block/sha256.h"
+
 #ifndef platform_SHA_CTX
 /*
  * platform's underlying implementation of SHA-1; could be OpenSSL,
@@ -34,6 +36,18 @@
 #define git_SHA1_Updateplatform_SHA1_Update
 #define git_SHA1_Final platform_SHA1_Final
 
+#ifndef platform_SHA256_CTX
+#define platform_SHA256_CTXSHA256_CTX
+#define platform_SHA256_Init   SHA256_Init
+#define platform_SHA256_Update SHA256_Update
+#define platform_SHA256_Final  SHA256_Final
+#endif
+
+#define git_SHA256_CTX platform_SHA256_CTX
+#define git_SHA256_Initplatform_SHA256_Init
+#define git_SHA256_Update  platform_SHA256_Update
+#define git_SHA256_Final   platform_SHA256_Final
+
 #ifdef SHA1_MAX_BLOCK_SIZE
 #include "compat/sha1-chunked.h"
 #undef git_SHA1_Update
@@ -52,12 +66,15 @@
 #define GIT_HASH_UNKNOWN 0
 /* SHA-1 */
 #define GIT_HASH_SHA1 1
+/* SHA-256  */
+#define GIT_HASH_SHA256 2
 /* Number of algorithms supported (including unknown). */
-#define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)
+#define GIT_HASH_NALGOS (GIT_HASH_SHA256 + 1)
 
 /* A suitably aligned type for stack allocations of hash contexts. */
 union git_hash_ctx {
git_SHA_CTX sha1;
+   git_SHA256_CTX sha256;
 };
 typedef union git_hash_ctx git_hash_ctx;
 
diff --git a/sha1-file.c b/sha1-file.c
index 9bdd04ea45..c97d93a14a 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -40,10 +40,20 @@
 #define EMPTY_TREE_SHA1_BIN_LITERAL \
 "\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60" \
 

[PATCH v5 07/12] sha1-file: add a constant for hash block size

[brian m. carlson]

There is one place we need the hash algorithm block size: the HMAC code
for push certs.  Expose this constant in struct git_hash_algo and expose
values for SHA-1 and for the largest value of any hash.

Signed-off-by: brian m. carlson 
---
 cache.h | 4 
 hash.h  | 3 +++
 sha1-file.c | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/cache.h b/cache.h
index bab8e8964f..9e5d1dd85a 100644
--- a/cache.h
+++ b/cache.h
@@ -45,10 +45,14 @@ unsigned long git_deflate_bound(git_zstream *, unsigned 
long);
 /* The length in bytes and in hex digits of an object name (SHA-1 value). */
 #define GIT_SHA1_RAWSZ 20
 #define GIT_SHA1_HEXSZ (2 * GIT_SHA1_RAWSZ)
+/* The block size of SHA-1. */
+#define GIT_SHA1_BLKSZ 64
 
 /* The length in byte and in hex digits of the largest possible hash value. */
 #define GIT_MAX_RAWSZ GIT_SHA1_RAWSZ
 #define GIT_MAX_HEXSZ GIT_SHA1_HEXSZ
+/* The largest possible block size for any supported hash. */
+#define GIT_MAX_BLKSZ GIT_SHA1_BLKSZ
 
 struct object_id {
unsigned char hash[GIT_MAX_RAWSZ];
diff --git a/hash.h b/hash.h
index 80881eea47..1bcf7ab6fd 100644
--- a/hash.h
+++ b/hash.h
@@ -81,6 +81,9 @@ struct git_hash_algo {
/* The length of the hash in hex characters. */
size_t hexsz;
 
+   /* The block size of the hash. */
+   size_t blksz;
+
/* The hash initialization function. */
git_hash_init_fn init_fn;
 
diff --git a/sha1-file.c b/sha1-file.c
index 7e9dedc744..9bdd04ea45 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -90,6 +90,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
0x,
0,
0,
+   0,
git_hash_unknown_init,
git_hash_unknown_update,
git_hash_unknown_final,
@@ -102,6 +103,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
0x73686131,
GIT_SHA1_RAWSZ,
GIT_SHA1_HEXSZ,
+   GIT_SHA1_BLKSZ,
git_hash_sha1_init,
git_hash_sha1_update,
git_hash_sha1_final,

[PATCH v5 01/12] sha1-file: rename algorithm to "sha1"

[brian m. carlson]

The transition plan anticipates us using a syntax such as "^{sha1}" for
disambiguation.  Since this is a syntax some people will be typing a
lot, it makes sense to provide a short, easy-to-type syntax.  Omitting
the dash doesn't create any ambiguity; however, it does make the syntax
shorter and easier to type, especially for touch typists.  In addition,
the transition plan already uses "sha1" in this context.

Rename the name of SHA-1 implementation to "sha1".

Note that this change creates no backwards compatibility concerns, since
we haven't yet used this field in any configuration settings.

Signed-off-by: brian m. carlson 
---
 sha1-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1-file.c b/sha1-file.c
index dd0b6aa873..91311ebb3d 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -97,7 +97,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
NULL,
},
{
-   "sha-1",
+   "sha1",
/* "sha1", big-endian */
0x73686131,
GIT_SHA1_RAWSZ,

[PATCH v5 06/12] t: make the sha1 test-tool helper generic

[brian m. carlson]

Since we're going to have multiple hash algorithms to test, it makes
sense to share as much of the test code as possible.  Convert the sha1
helper for the test-tool to be generic and move it out into its own
module.  This will allow us to share most of this code with our NewHash
implementation.

Signed-off-by: brian m. carlson 
---
 Makefile  |  1 +
 t/helper/{test-sha1.c => test-hash.c} | 19 +-
 t/helper/test-sha1.c  | 52 +--
 t/helper/test-tool.h  |  2 ++
 4 files changed, 14 insertions(+), 60 deletions(-)
 copy t/helper/{test-sha1.c => test-hash.c} (65%)

diff --git a/Makefile b/Makefile
index d18ab0fe78..81dc9ac819 100644
--- a/Makefile
+++ b/Makefile
@@ -714,6 +714,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-genrandom.o
+TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
diff --git a/t/helper/test-sha1.c b/t/helper/test-hash.c
similarity index 65%
copy from t/helper/test-sha1.c
copy to t/helper/test-hash.c
index 1ba0675c75..0a31de66f3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-hash.c
@@ -1,13 +1,14 @@
 #include "test-tool.h"
 #include "cache.h"
 
-int cmd__sha1(int ac, const char **av)
+int cmd_hash_impl(int ac, const char **av, int algo)
 {
-   git_SHA_CTX ctx;
-   unsigned char sha1[20];
+   git_hash_ctx ctx;
+   unsigned char hash[GIT_MAX_HEXSZ];
unsigned bufsz = 8192;
int binary = 0;
char *buffer;
+   const struct git_hash_algo *algop = _algos[algo];
 
if (ac == 2) {
if (!strcmp(av[1], "-b"))
@@ -26,7 +27,7 @@ int cmd__sha1(int ac, const char **av)
die("OOPS");
}
 
-   git_SHA1_Init();
+   algop->init_fn();
 
while (1) {
ssize_t sz, this_sz;
@@ -38,20 +39,20 @@ int cmd__sha1(int ac, const char **av)
if (sz == 0)
break;
if (sz < 0)
-   die_errno("test-sha1");
+   die_errno("test-hash");
this_sz += sz;
cp += sz;
room -= sz;
}
if (this_sz == 0)
break;
-   git_SHA1_Update(, buffer, this_sz);
+   algop->update_fn(, buffer, this_sz);
}
-   git_SHA1_Final(sha1, );
+   algop->final_fn(hash, );
 
if (binary)
-   fwrite(sha1, 1, 20, stdout);
+   fwrite(hash, 1, algop->rawsz, stdout);
else
-   puts(sha1_to_hex(sha1));
+   puts(hash_to_hex_algop(hash, algop));
exit(0);
 }
diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c
index 1ba0675c75..d860c387c3 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-sha1.c
@@ -3,55 +3,5 @@
 
 int cmd__sha1(int ac, const char **av)
 {
-   git_SHA_CTX ctx;
-   unsigned char sha1[20];
-   unsigned bufsz = 8192;
-   int binary = 0;
-   char *buffer;
-
-   if (ac == 2) {
-   if (!strcmp(av[1], "-b"))
-   binary = 1;
-   else
-   bufsz = strtoul(av[1], NULL, 10) * 1024 * 1024;
-   }
-
-   if (!bufsz)
-   bufsz = 8192;
-
-   while ((buffer = malloc(bufsz)) == NULL) {
-   fprintf(stderr, "bufsz %u is too big, halving...\n", bufsz);
-   bufsz /= 2;
-   if (bufsz < 1024)
-   die("OOPS");
-   }
-
-   git_SHA1_Init();
-
-   while (1) {
-   ssize_t sz, this_sz;
-   char *cp = buffer;
-   unsigned room = bufsz;
-   this_sz = 0;
-   while (room) {
-   sz = xread(0, cp, room);
-   if (sz == 0)
-   break;
-   if (sz < 0)
-   die_errno("test-sha1");
-   this_sz += sz;
-   cp += sz;
-   room -= sz;
-   }
-   if (this_sz == 0)
-   break;
-   git_SHA1_Update(, buffer, this_sz);
-   }
-   git_SHA1_Final(sha1, );
-
-   if (binary)
-   fwrite(sha1, 1, 20, stdout);
-   else
-   puts(sha1_to_hex(sha1));
-   exit(0);
+   return cmd_hash_impl(ac, av, GIT_HASH_SHA1);
 }
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index e4890566da..29ac7b0b0d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,4 +50,6 @@ int cmd__windows_named_pipe(int argc, const char **argv);
 #endif
 int cmd__write_cache(int argc, const char **argv);
 

[PATCH v5 08/12] t/helper: add a test helper to compute hash speed

[brian m. carlson]

Add a utility (which is less for the testsuite and more for developers)
that can compute hash speeds for whatever hash algorithms are
implemented.  This allows developers to test their personal systems to
determine the performance characteristics of various algorithms.

Signed-off-by: brian m. carlson 
---
 Makefile   |  1 +
 t/helper/test-hash-speed.c | 61 ++
 t/helper/test-tool.c   |  1 +
 t/helper/test-tool.h   |  1 +
 4 files changed, 64 insertions(+)
 create mode 100644 t/helper/test-hash-speed.c

diff --git a/Makefile b/Makefile
index 81dc9ac819..68169a7abb 100644
--- a/Makefile
+++ b/Makefile
@@ -716,6 +716,7 @@ TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-genrandom.o
 TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
+TEST_BUILTINS_OBJS += test-hash-speed.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
 TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o
diff --git a/t/helper/test-hash-speed.c b/t/helper/test-hash-speed.c
new file mode 100644
index 00..432233c7f0
--- /dev/null
+++ b/t/helper/test-hash-speed.c
@@ -0,0 +1,61 @@
+#include "test-tool.h"
+#include "cache.h"
+
+#define NUM_SECONDS 3
+
+static inline void compute_hash(const struct git_hash_algo *algo, git_hash_ctx 
*ctx, uint8_t *final, const void *p, size_t len)
+{
+   algo->init_fn(ctx);
+   algo->update_fn(ctx, p, len);
+   algo->final_fn(final, ctx);
+}
+
+int cmd__hash_speed(int ac, const char **av)
+{
+   git_hash_ctx ctx;
+   unsigned char hash[GIT_MAX_RAWSZ];
+   clock_t initial, start, end;
+   unsigned bufsizes[] = { 64, 256, 1024, 8192, 16384 };
+   int i;
+   void *p;
+   const struct git_hash_algo *algo = NULL;
+
+   if (ac == 2) {
+   for (i = 1; i < GIT_HASH_NALGOS; i++) {
+   if (!strcmp(av[1], hash_algos[i].name)) {
+   algo = _algos[i];
+   break;
+   }
+   }
+   }
+   if (!algo)
+   die("usage: test-tool hash-speed algo_name");
+
+   /* Use this as an offset to make overflow less likely. */
+   initial = clock();
+
+   printf("algo: %s\n", algo->name);
+
+   for (i = 0; i < ARRAY_SIZE(bufsizes); i++) {
+   unsigned long j, kb;
+   double kb_per_sec;
+   p = xcalloc(1, bufsizes[i]);
+   start = end = clock() - initial;
+   for (j = 0; ((end - start) / CLOCKS_PER_SEC) < NUM_SECONDS; 
j++) {
+   compute_hash(algo, , hash, p, bufsizes[i]);
+
+   /*
+* Only check elapsed time every 128 iterations to avoid
+* dominating the runtime with system calls.
+*/
+   if (!(j & 127))
+   end = clock() - initial;
+   }
+   kb = j * bufsizes[i];
+   kb_per_sec = kb / (1024 * ((double)end - start) / 
CLOCKS_PER_SEC);
+   printf("size %u: %lu iters; %lu KiB; %0.2f KiB/s\n", 
bufsizes[i], j, kb, kb_per_sec);
+   free(p);
+   }
+
+   exit(0);
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 6b5836dc1b..e009c8186d 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -20,6 +20,7 @@ static struct test_cmd cmds[] = {
{ "example-decorate", cmd__example_decorate },
{ "genrandom", cmd__genrandom },
{ "hashmap", cmd__hashmap },
+   { "hash-speed", cmd__hash_speed },
{ "index-version", cmd__index_version },
{ "json-writer", cmd__json_writer },
{ "lazy-init-name-hash", cmd__lazy_init_name_hash },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 29ac7b0b0d..19a7e8332a 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -16,6 +16,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);
 int cmd__hashmap(int argc, const char **argv);
+int cmd__hash_speed(int argc, const char **argv);
 int cmd__index_version(int argc, const char **argv);
 int cmd__json_writer(int argc, const char **argv);
 int cmd__lazy_init_name_hash(int argc, const char **argv);

[PATCH v5 05/12] t: add basic tests for our SHA-1 implementation

[brian m. carlson]

We have in the past had some unfortunate endianness issues with some
SHA-1 implementations we ship, especially on big-endian machines.  Add
an explicit test using the test helper to catch these issues and point
them out prominently.  This test can also be used as a staging ground
for people testing additional algorithms to verify that their
implementations are working as expected.

Signed-off-by: brian m. carlson 
---
 t/t0015-hash.sh | 29 +
 1 file changed, 29 insertions(+)
 create mode 100755 t/t0015-hash.sh

diff --git a/t/t0015-hash.sh b/t/t0015-hash.sh
new file mode 100755
index 00..8e763c2c3d
--- /dev/null
+++ b/t/t0015-hash.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='test basic hash implementation'
+. ./test-lib.sh
+
+
+test_expect_success 'test basic SHA-1 hash values' '
+   test-tool sha1 actual &&
+   grep da39a3ee5e6b4b0d3255bfef95601890afd80709 actual &&
+   printf "a" | test-tool sha1 >actual &&
+   grep 86f7e437faa5a7fce15d1ddcb9eaeaea377667b8 actual &&
+   printf "abc" | test-tool sha1 >actual &&
+   grep a9993e364706816aba3e25717850c26c9cd0d89d actual &&
+   printf "message digest" | test-tool sha1 >actual &&
+   grep c12252ceda8be8994d5fa0290a47231c1d16aae3 actual &&
+   printf "abcdefghijklmnopqrstuvwxyz" | test-tool sha1 >actual &&
+   grep 32d10c7b8cf96570ca04ce37f2a19d84240d3a89 actual &&
+   perl -E "for (1..10) { print q{aa}; }" | \
+   test-tool sha1 >actual &&
+   grep 34aa973cd4c4daa4f61eeb2bdbad27316534016f actual &&
+   printf "blob 0\0" | test-tool sha1 >actual &&
+   grep e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 actual &&
+   printf "blob 3\0abc" | test-tool sha1 >actual &&
+   grep f2ba8f84ab5c1bce84a7b441cb1959cfc7093b7f actual &&
+   printf "tree 0\0" | test-tool sha1 >actual &&
+   grep 4b825dc642cb6eb9a060e54bf8d69288fbee4904 actual
+'
+
+test_done

[PATCH v5 04/12] cache: make hashcmp and hasheq work with larger hashes

[brian m. carlson]

In 183a638b7d ("hashcmp: assert constant hash size", 2018-08-23), we
modified hashcmp to assert that the hash size was always 20 to help it
optimize and inline calls to memcmp.  In a future series, we replaced
many calls to hashcmp and oidcmp with calls to hasheq and oideq to
improve inlining further.

However, we want to support hash algorithms other than SHA-1, namely
SHA-256.  When doing so, we must handle the case where these values are
32 bytes long as well as 20.  Adjust hashcmp to handle two cases:
20-byte matches, and maximum-size matches.  Therefore, when we include
SHA-256, we'll automatically handle it properly, while at the same time
teaching the compiler that there are only two possible options to
consider.  This will allow the compiler to write the most efficient
possible code.

Copy similar code into hasheq and perform an identical transformation.
At least with GCC 8.2.0, making hasheq defer to hashcmp when there are
two branches prevents the compiler from inlining the comparison, while
the code in this patch is inlined properly.  Add a comment to avoid an
accidental performance regression from well-intentioned refactoring.

Signed-off-by: brian m. carlson 
---
 cache.h | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 51580c4b77..bab8e8964f 100644
--- a/cache.h
+++ b/cache.h
@@ -1027,16 +1027,12 @@ extern const struct object_id null_oid;
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
 {
/*
-* This is a temporary optimization hack. By asserting the size here,
-* we let the compiler know that it's always going to be 20, which lets
-* it turn this fixed-size memcmp into a few inline instructions.
-*
-* This will need to be extended or ripped out when we learn about
-* hashes of different sizes.
+* Teach the compiler that there are only two possibilities of hash size
+* here, so that it can optimize for this case as much as possible.
 */
-   if (the_hash_algo->rawsz != 20)
-   BUG("hash size not yet supported by hashcmp");
-   return memcmp(sha1, sha2, the_hash_algo->rawsz);
+   if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+   return memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+   return memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
 }
 
 static inline int oidcmp(const struct object_id *oid1, const struct object_id 
*oid2)
@@ -1046,7 +1042,13 @@ static inline int oidcmp(const struct object_id *oid1, 
const struct object_id *o
 
 static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2)
 {
-   return !hashcmp(sha1, sha2);
+   /*
+* We write this here instead of deferring to hashcmp so that the
+* compiler can properly inline it and avoid calling memcmp.
+*/
+   if (the_hash_algo->rawsz == GIT_MAX_RAWSZ)
+   return !memcmp(sha1, sha2, GIT_MAX_RAWSZ);
+   return !memcmp(sha1, sha2, GIT_SHA1_RAWSZ);
 }
 
 static inline int oideq(const struct object_id *oid1, const struct object_id 
*oid2)

[PATCH v5 00/12] Base SHA-256 implementation

[brian m. carlson]

This series provides a functional SHA-256 implementation and wires it
up, along with some housekeeping patches to make it suitable for
testing.

Changes from v4:
* Downcase hex constants for consistency.
* Remove needless parentheses in return statement.
* Remove braces for single statement loops.
* Switch to +=.
* Add references to rationale for SHA-256.
* Remove inclusion of "git-compat-util.h" in header.

Changes from v3:
* Switch to using inline functions instead of macros in many cases.
* Undefine remaining macros at the top.

Changes from v2:
* Improve commit messages to include timing and performance information.
* Improve commit messages to be less ambiguous and more friendly to a
  wider variety of English speakers.
* Prefer functions taking struct git_hash_algo in hex.c.
* Port pieces of the block-sha1 implementation over to the block-sha256
  implementation for better compatibility.
* Drop patch 13 in favor of further discussion about the best way
  forward for versioning commit graph.
* Rename the test so as to have a different number from other tests.
* Rebase on master.

Changes from v1:
* Add a hash_to_hex function mirroring sha1_to_hex, but for
  the_hash_algo.
* Strip commit message explanation about why we chose SHA-256.
* Rebase on master
* Strip leading whitespace from commit message.
* Improve commit-graph patch to cover new code added since v1.
* Be more honest about the scope of work involved in porting the SHA-256
  implementation out of libtomcrypt.
* Revert change to limit hashcmp to 20 bytes.

brian m. carlson (12):
  sha1-file: rename algorithm to "sha1"
  sha1-file: provide functions to look up hash algorithms
  hex: introduce functions to print arbitrary hashes
  cache: make hashcmp and hasheq work with larger hashes
  t: add basic tests for our SHA-1 implementation
  t: make the sha1 test-tool helper generic
  sha1-file: add a constant for hash block size
  t/helper: add a test helper to compute hash speed
  commit-graph: convert to using the_hash_algo
  Add a base implementation of SHA-256 support
  sha256: add an SHA-256 implementation using libgcrypt
  hash: add an SHA-256 implementation using OpenSSL

 Makefile  |  22 +++
 cache.h   |  51 ---
 commit-graph.c|  33 ++---
 hash.h|  41 +-
 hex.c |  32 +++--
 sha1-file.c   |  70 -
 sha256/block/sha256.c | 196 ++
 sha256/block/sha256.h |  26 
 sha256/gcrypt.h   |  30 
 t/helper/test-hash-speed.c|  61 
 t/helper/{test-sha1.c => test-hash.c} |  19 +--
 t/helper/test-sha1.c  |  52 +--
 t/helper/test-sha256.c|   7 +
 t/helper/test-tool.c  |   2 +
 t/helper/test-tool.h  |   4 +
 t/t0015-hash.sh   |  54 +++
 16 files changed, 596 insertions(+), 104 deletions(-)
 create mode 100644 sha256/block/sha256.c
 create mode 100644 sha256/block/sha256.h
 create mode 100644 sha256/gcrypt.h
 create mode 100644 t/helper/test-hash-speed.c
 copy t/helper/{test-sha1.c => test-hash.c} (65%)
 create mode 100644 t/helper/test-sha256.c
 create mode 100755 t/t0015-hash.sh

Range-diff against v4:
 1:  a004a4c982 <  -:  -- :hash-impl
 2:  cf9f7f5620 =  1:  cf9f7f5620 sha1-file: rename algorithm to "sha1"
 3:  0144deaebe =  2:  0144deaebe sha1-file: provide functions to look up hash 
algorithms
 4:  b74858fb03 =  3:  b74858fb03 hex: introduce functions to print arbitrary 
hashes
 5:  e9703017a4 =  4:  e9703017a4 cache: make hashcmp and hasheq work with 
larger hashes
 6:  ab85a834fd =  5:  ab85a834fd t: add basic tests for our SHA-1 
implementation
 7:  962f6d8903 =  6:  962f6d8903 t: make the sha1 test-tool helper generic
 8:  53addf4d58 =  7:  53addf4d58 sha1-file: add a constant for hash block size
 9:  9ace10faa2 =  8:  9ace10faa2 t/helper: add a test helper to compute hash 
speed
10:  9adc56d01e =  9:  9adc56d01e commit-graph: convert to using the_hash_algo
11:  f48cb1ad27 ! 10:  90544c504c Add a base implementation of SHA-256 support
@@ -4,7 +4,9 @@
 
 SHA-1 is weak and we need to transition to a new hash function.  For
 some time, we have referred to this new function as NewHash.  Recently,
-we decided to pick SHA-256 as NewHash.
+we decided to pick SHA-256 as NewHash.  The reasons behind the choice 
of
+SHA-256 are outlined in the thread starting at [1] and in the commit
+history for the hash function transition document.
 
 Add a basic implementation of SHA-256 based off libtomcrypt, which is 
in
 the public domain.  Optimize it and restructure it to meet our coding
@@ -20,6 +22,8 @@
 SHA-256 in Git.  Additional patches are needed to prepare the code to
 handle a larger 

[PATCH v5 03/12] hex: introduce functions to print arbitrary hashes

[brian m. carlson]

Currently, we have functions that turn an arbitrary SHA-1 value or an
object ID into hex format, either using a static buffer or with a
user-provided buffer.  Add variants of these functions that can handle
an arbitrary hash algorithm, specified by constant.  Update the
documentation as well.

While we're at it, remove the "extern" declaration from this family of
functions, since it's not needed and our style now recommends against
it.

We use the variant taking the algorithm structure pointer as the
internal variant, since taking an algorithm pointer is the easiest way
to handle all of the variants in use.

Note that we maintain these functions because there are hashes which
must change based on the hash algorithm in use but are not object IDs
(such as pack checksums).

Signed-off-by: brian m. carlson 
---
 cache.h | 15 +--
 hex.c   | 32 
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/cache.h b/cache.h
index 59c8a93046..51580c4b77 100644
--- a/cache.h
+++ b/cache.h
@@ -1364,9 +1364,9 @@ extern int get_oid_hex(const char *hex, struct object_id 
*sha1);
 extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
 
 /*
- * Convert a binary sha1 to its hex equivalent. The `_r` variant is reentrant,
+ * Convert a binary hash to its hex equivalent. The `_r` variant is reentrant,
  * and writes the NUL-terminated output to the buffer `out`, which must be at
- * least `GIT_SHA1_HEXSZ + 1` bytes, and returns a pointer to out for
+ * least `GIT_MAX_HEXSZ + 1` bytes, and returns a pointer to out for
  * convenience.
  *
  * The non-`_r` variant returns a static buffer, but uses a ring of 4
@@ -1374,10 +1374,13 @@ extern int hex_to_bytes(unsigned char *binary, const 
char *hex, size_t len);
  *
  *   printf("%s -> %s", sha1_to_hex(one), sha1_to_hex(two));
  */
-extern char *sha1_to_hex_r(char *out, const unsigned char *sha1);
-extern char *oid_to_hex_r(char *out, const struct object_id *oid);
-extern char *sha1_to_hex(const unsigned char *sha1);   /* static buffer 
result! */
-extern char *oid_to_hex(const struct object_id *oid);  /* same static buffer 
as sha1_to_hex */
+char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash, const 
struct git_hash_algo *);
+char *sha1_to_hex_r(char *out, const unsigned char *sha1);
+char *oid_to_hex_r(char *out, const struct object_id *oid);
+char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo 
*);  /* static buffer result! */
+char *sha1_to_hex(const unsigned char *sha1);  
/* same static buffer */
+char *hash_to_hex(const unsigned char *hash);  
/* same static buffer */
+char *oid_to_hex(const struct object_id *oid); 
/* same static buffer */
 
 /*
  * Parse a 40-character hexadecimal object ID starting from hex, updating the
diff --git a/hex.c b/hex.c
index 10af1a29e8..d2e8bb9540 100644
--- a/hex.c
+++ b/hex.c
@@ -73,14 +73,15 @@ int parse_oid_hex(const char *hex, struct object_id *oid, 
const char **end)
return ret;
 }
 
-char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
+inline char *hash_to_hex_algop_r(char *buffer, const unsigned char *hash,
+   const struct git_hash_algo *algop)
 {
static const char hex[] = "0123456789abcdef";
char *buf = buffer;
int i;
 
-   for (i = 0; i < the_hash_algo->rawsz; i++) {
-   unsigned int val = *sha1++;
+   for (i = 0; i < algop->rawsz; i++) {
+   unsigned int val = *hash++;
*buf++ = hex[val >> 4];
*buf++ = hex[val & 0xf];
}
@@ -89,20 +90,35 @@ char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
return buffer;
 }
 
-char *oid_to_hex_r(char *buffer, const struct object_id *oid)
+char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
 {
-   return sha1_to_hex_r(buffer, oid->hash);
+   return hash_to_hex_algop_r(buffer, sha1, _algos[GIT_HASH_SHA1]);
 }
 
-char *sha1_to_hex(const unsigned char *sha1)
+char *oid_to_hex_r(char *buffer, const struct object_id *oid)
+{
+   return hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);
+}
+
+char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo 
*algop)
 {
static int bufno;
static char hexbuffer[4][GIT_MAX_HEXSZ + 1];
bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
-   return sha1_to_hex_r(hexbuffer[bufno], sha1);
+   return hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);
+}
+
+char *sha1_to_hex(const unsigned char *sha1)
+{
+   return hash_to_hex_algop(sha1, _algos[GIT_HASH_SHA1]);
+}
+
+char *hash_to_hex(const unsigned char *hash)
+{
+   return hash_to_hex_algop(hash, the_hash_algo);
 }
 
 char *oid_to_hex(const struct object_id *oid)
 {
-   return sha1_to_hex(oid->hash);
+   return 

[PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms

[brian m. carlson]

There are several ways we might refer to a hash algorithm: by name, such
as in the config file; by format ID, such as in a pack; or internally,
by a pointer to the hash_algos array.  Provide functions to look up hash
algorithms based on these various forms and return the internal constant
used for them.  If conversion to another form is necessary, this
internal constant can be used to look up the proper data in the
hash_algos array.

Signed-off-by: brian m. carlson 
---
 hash.h  | 13 +
 sha1-file.c | 21 +
 2 files changed, 34 insertions(+)

diff --git a/hash.h b/hash.h
index 7c8238bc2e..80881eea47 100644
--- a/hash.h
+++ b/hash.h
@@ -98,4 +98,17 @@ struct git_hash_algo {
 };
 extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
 
+/*
+ * Return a GIT_HASH_* constant based on the name.  Returns GIT_HASH_UNKNOWN if
+ * the name doesn't match a known algorithm.
+ */
+int hash_algo_by_name(const char *name);
+/* Identical, except based on the format ID. */
+int hash_algo_by_id(uint32_t format_id);
+/* Identical, except for a pointer to struct git_hash_algo. */
+static inline int hash_algo_by_ptr(const struct git_hash_algo *p)
+{
+   return p - hash_algos;
+}
+
 #endif
diff --git a/sha1-file.c b/sha1-file.c
index 91311ebb3d..7e9dedc744 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -122,6 +122,27 @@ const char *empty_blob_oid_hex(void)
return oid_to_hex_r(buf, the_hash_algo->empty_blob);
 }
 
+int hash_algo_by_name(const char *name)
+{
+   int i;
+   if (!name)
+   return GIT_HASH_UNKNOWN;
+   for (i = 1; i < GIT_HASH_NALGOS; i++)
+   if (!strcmp(name, hash_algos[i].name))
+   return i;
+   return GIT_HASH_UNKNOWN;
+}
+
+int hash_algo_by_id(uint32_t format_id)
+{
+   int i;
+   for (i = 1; i < GIT_HASH_NALGOS; i++)
+   if (format_id == hash_algos[i].format_id)
+   return i;
+   return GIT_HASH_UNKNOWN;
+}
+
+
 /*
  * This is meant to hold a *small* number of objects that you would
  * want read_sha1_file() to be able to return, but yet you do not want

RE: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Randall S. Becker]

On November 4, 2018 6:26 PM, Junio C Hamano, wrote:
> Johannes Sixt  writes:
> 
> > Am 03.11.18 um 09:14 schrieb Carlo Arenas:
> >> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt  wrote:
> >>>
> >>> +  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout -
> >>> + elapsed);
> >>
> >> nitpick: cast to DWORD instead of int
> >
> > No; timeout is of type int; after an explicit type cast we don't want
> > to have another implicit conversion.
> >
> > -- Hannes
> 
> OK, thanks.  It seems that the relative silence after this message is a
sign that
> the resulting patch after squashing is what everybody is happey with?

On my platform (HPE NonStop), DWORD is being defined as unsigned int
(32-bit) rather than unsigned long long (64 bit). The definition comes
through the odbc/windows.h include, not the compiler or any core definition.
It's only a nano-quibble (if even that), because GetTickCount64 is not
defined on the platform anyway, so this is probably not a big deal.

Cheers,
Randall

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Junio C Hamano]

Johannes Sixt  writes:

> Am 03.11.18 um 09:14 schrieb Carlo Arenas:
>> On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt  wrote:
>>>
>>> +  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - 
>>> elapsed);
>>
>> nitpick: cast to DWORD instead of int
>
> No; timeout is of type int; after an explicit type cast we don't want
> to have another implicit conversion.
>
> -- Hannes

OK, thanks.  It seems that the relative silence after this message
is a sign that the resulting patch after squashing is what everybody
is happey with?

-- >8 --
From: Steve Hoelzer 
Date: Wed, 31 Oct 2018 14:11:36 -0700
Subject: [PATCH] poll: use GetTickCount64() to avoid wrap-around issues

The value of timeout starts as an int value, and for this reason it
cannot overflow unsigned long long aka ULONGLONG. The unsigned version
of this initial value is available in orig_timeout. The difference
(orig_timeout - elapsed) cannot wrap around because it is protected by
a conditional (as can be seen in the patch text). Hence, the ULONGLONG
difference can only have values that are smaller than the initial
timeout value and truncation to int cannot overflow.

Signed-off-by: Johannes Sixt 
Acked-by: Steve Hoelzer 
Signed-off-by: Junio C Hamano 
---
 compat/poll/poll.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index ad5dcde439..4459408c7d 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -18,6 +18,9 @@
You should have received a copy of the GNU General Public License along
with this program; if not, see .  */
 
+/* To bump the minimum Windows version to Windows Vista */
+#include "git-compat-util.h"
+
 /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
 #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
 # pragma GCC diagnostic ignored "-Wtype-limits"
@@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   static HANDLE hEvent;
   WSANETWORKEVENTS ev;
   HANDLE h, handle_array[FD_SETSIZE + 2];
-  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
+  DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
+  ULONGLONG start = 0;
   fd_set rfds, wfds, xfds;
   BOOL poll_again;
   MSG msg;
@@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   if (timeout != INFTIM)
 {
   orig_timeout = timeout;
-  start = GetTickCount();
+  start = GetTickCount64();
 }
 
   if (!hEvent)
@@ -614,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
 
   if (!rc && orig_timeout && timeout != INFTIM)
 {
-  elapsed = GetTickCount() - start;
-  timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
+  ULONGLONG elapsed = GetTickCount64() - start;
+  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
 }
 
   if (!rc && timeout)
-- 
2.19.1-816-gcd69ec8cde

Re: [PATCH/RFC v2] sequencer.c: record revert/cherry-pick commit with trailer lines

[brian m. carlson]

On Sun, Nov 04, 2018 at 07:10:26PM +0100, Nguyễn Thái Ngọc Duy wrote:
> When a commit is reverted (or cherry-picked with -x) we add an English
> sentence recording that commit id in the new commit message. Make
> these real trailer lines instead so that they are more friendly to
> parsers (especially "git interpret-trailers").
> 
> A reverted commit will have a new trailer
> 
> Revert: 
> 
> Similarly a cherry-picked commit with -x will have
> 
> Cherry-picked-from: 
> 
> When reverting or cherry picking a merge, the reverted/cherry-picked
> branch will be shown using extended SHA-1 syntax, e.g.
> 
> Revert: ~2
> 
> Since we're not producing the old lines "This reverts commit ..." and
> "(cherry picked from commit .." anymore, scripts that look for these
> lines will need to be updated to handle both. Fresh new history could
> just rely on git-interpret-trailers instead.

Overall, I like the idea of this series.  This is a much cleaner way to
handle things and much better for machine-readability.  I foresee git
cherry potentially learning how to parse this, for example, for cases
where the patch-id doesn't match due to context changes.

However, I do have concerns about breaking compatibility with existing
scripts.  I wonder if we could add a long alias for git cherry-pick -x,
say "--notate" and have "--notate=text" mean "-x" and "--notate=trailer"
mean this new format.  Similarly, git revert could learn such an option
as well.

One final thought: since our trailers seem to act as if we wrote "this
commit" (has been), I wonder if we should say "Reverts" instead of
"Revert" for consistency.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

[Phillip Wood]

Hi Duy

On 04/11/2018 17:41, Duy Nguyen wrote:

On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood  wrote:


On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:

When a commit is reverted (or cherry-picked with -x) we add an English
sentence recording that commit id in the new commit message. Make
these real trailer lines instead so that they are more friendly to
parsers (especially "git interpret-trailers").

A reverted commit will have a new trailer

  Revert: 

Similarly a cherry-picked commit with -x will have

  Cherry-Pick: 


I think this is a good idea though I wonder if it will break someones
script that is looking for the messages generated by -x at the moment.


It will [1] but I still think it's worth the trouble. The script will
be less likely to break after, and you can use git-interpret-trailers
instead of plain grep.

[1] 
https://public-inbox.org/git/20181017143921.gr270...@devbig004.ftw2.facebook.com/


@@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
   base_label = msg.label;
   next = parent;
   next_label = msg.parent_label;
- strbuf_addstr(, "Revert \"");
- strbuf_addstr(, msg.subject);
- strbuf_addstr(, "\"\n\nThis reverts commit ");
- strbuf_addstr(, oid_to_hex(>object.oid));
-
- if (commit->parents && commit->parents->next) {
- strbuf_addstr(, ", reversing\nchanges made to ");
- strbuf_addstr(, oid_to_hex(>object.oid));
- }


As revert currently records the parent given on the command line when
reverting a merge commit it would probably be a good idea to add that
either as a separate trailer or to the Revert: trailer and possibly also
generate it for cherry picks.


My mistake. I didn't read carefully and thought it was logging
commit->parents, which is pointless.

So what should be the trailer for this (I don't think putting it in
Revert: is a good idea, too much to parse)? Revert-parent: ?
Revert-merge: ?


I think Revert-parent: is good, though you seem to have gone for 
including it the Revert: trailer in v2.

- strbuf_addstr(, ".\n");
+ strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);


If the message already contains trailers then should we just append the
Revert trailer those rather than inserting "\n\n"?


Umm.. but this \n\n is for separating the subject and the body. I
think we need it anyway, trailer or not.


Ah you're right, I had forgotten that the revert message body is empty, 
unlike cherry-pick where the message is copied (and that does the right 
thing already when there are existing trailers).


Best wishes

Phillip



@@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
   strbuf_complete_line();
   if (!has_conforming_footer(, NULL, 0))
   strbuf_addch(, '\n');
- strbuf_addstr(, cherry_picked_prefix);
- strbuf_addstr(, oid_to_hex(>object.oid));
- strbuf_addstr(, ")\n");
+ strbuf_addf(, "Cherry-Pick: %s\n",
+ oid_to_hex(>object.oid));


I will probably make this "Cherry-picked-from:" to match our S-o-b style.

Re: [RFC v1] Add virtual file system settings and hook proc

[brian m. carlson]

On Sun, Nov 04, 2018 at 07:34:01AM +0100, Duy Nguyen wrote:
> On Wed, Oct 31, 2018 at 9:53 PM Ben Peart  wrote:
> > It's more than a dynamic sparse-checkout because the same list is also
> > used to exclude any file/folder not listed.  That means any file not
> > listed won't ever be updated by git (like in 'checkout' for example) so
> > 'stale' files could be left in the working directory.  It also means git
> > won't find new/untracked files unless they are specifically added to the
> > list.
> 
> OK. I'm not at all interested in carrying maintenance burden for some
> software behind closed doors. I could see values in having a more
> flexible sparse checkout but this now seems like very tightly designed
> for GVFS. So unless there's another use case (preferably open source)
> for this, I don't think this should be added in git.git.

I should point out that VFS for Git is an open-source project and will
likely have larger use than just at Microsoft.  There are both Windows
and Mac clients and there are plans for a Linux client as well.
Ideally, it would work with an unmodified upstream Git, which is (I
assume) why Ben is sending this series.

Personally, I don't love the current name used in this series.  I don't
see this patch as introducing a virtual file system in the Unix sense of
that word, and I think calling it that in Git core will be confusing to
Unix users.  I would prefer to see it as a hook (maybe called
"sparse-checkout" or "sparse-exclude"; better names are okay), and
simply turn it on based on whether or not there's an appropriate hook
file there and whether core.sparseCheckout is on (or possibly with
hook.sparseExclude or something).  With a design more like that, I don't
see a problem with it in principle.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re-assignment of Bank Draft

[Laura Stevens]

Hello
I’m Laura Stevens of Federal clearance credit department, we discovered Payment 
that was approved for an Italian business man living in the USA, Presently, I 
was asked to raise a Bank Draft for $41million Dollars in his name and have it 
mailed to his bank. I put up a telephone call and discovered that Mr. Sergio 
Died two weeks ago in ITALY his home country. 
Now i would like to issue an approval letter in your name to enable me change 
this bank draft to your name and have it sent to your Bank for cash-out. If 
this is acceptable by you kindly send your banking details and your address and 
phone number for further discussions on sharing terms.
Thanks
Laura Stevens
Federal Reserves
International Credit Settlement
USA

"git checkout" obliterates not-yet-ignored files

[Matthias Urlichs]

Hi,

the problem: suppose I decide that a local file should no longer be
controlled by git. Thus I add it to .gitignore and then "git rm
--cached" it. So far so good.

However, if I subsequently modify that file and then go back to a commit
that still contains it, my local changes will be obliterated.

IMHO that's a bug – .gitignore should only be used for (not) adding
non-version-controlled files. It does not tell git to ignore changes (in
files that *are* under version control), and thus it should not allow
any git command to simply overwrite a file.

-- 
-- Matthias Urlichs




signature.asc
Description: OpenPGP digital signature

"git checkout" safety feature

[Matthias Urlichs]

Hi,

A recent discussion on LWN https://lwn.net/Articles/770642/ noted that
"git checkout  " does not warn if one if the files has
been modified locally, nor is there an option to do so.

IMHO that should be fixed, preferably by somebody who knows git's
internals well enough to do so in half an hour ;-)

-- 
-- Matthias Urlichs




signature.asc
Description: OpenPGP digital signature

Re: git-rebase is ignoring working-tree-encoding

[brian m. carlson]

On Sun, Nov 04, 2018 at 05:37:09PM +0100, Adrián Gimeno Balaguer wrote:
> I wrote a "counterpart" easy fix which instead only prohibites BOM for
> the opposite endianness (for example if
> working-tree-encoding=UTF-16LE, then finding an UTF-16BE BOM in the
> file would cause Git to signal the error right before committing,
> diffing, etc.). That way user would be encouraged to modify the file's
> encoding to match the one specified in working-tree-encoding before
> allowing these actions, therefore preventing Git from encoding to the
> wrong endianness after file is written out. With few repository tests,
> this new behaviour worked as expected. But then I realized this
> solution would perhaps be unacceptable for Git's source code as it
> would violate that Unicode standard. Anyways, here is a PR in my Git
> fork with the changes I did, for reference:

I actually think such a solution (although I haven't looked at your
patch) would be fine, and I would encourage you to send it to the list.
It's my understanding that many people on Windows want to write things
in UTF-16 encoding but only little-endian with a BOM.  Allowing them to
write that, even if Git won't be able to guarantee producing that, would
be fine, as long as the data is what we expect.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Eric Sunshine]

On Sun, Nov 4, 2018 at 10:24 AM Anders Waldenborg  wrote:
> Adds a new "key=X" option to "%(trailers)" which will cause it to only
> print trailers lines which matches the specified key.
>
> Signed-off-by: Anders Waldenborg 
> ---
> diff --git a/Documentation/pretty-formats.txt 
> b/Documentation/pretty-formats.txt
> @@ -209,11 +209,14 @@ endif::git-rev-list[]
>  - %(trailers[:options]): display the trailers of the body as interpreted
>by linkgit:git-interpret-trailers[1]. The `trailers` string may be
> +  followed by a colon and zero or more comma-separated options. The
> +  allowed options are `only` which omits non-trailer lines from the
> +  trailer block, `unfold` to make it behave as if interpret-trailer's
> +  `--unfold` option was given, and `key=T` to only show trailers with
> +  specified key (matching is done
> +  case-insensitively).

Does the user have to include the colon when specifying  of
'key='? I can see from peeking at the implementation that the
colon must not be used, but this should be documented. Should the code
tolerate a trailing colon? (Genuine question; it's easy to do and
would be more user-friendly.)

Does 'key=', do a full or partial match on trailers? And, if
partial, is the match anchored at the start or can it match anywhere
in the trailer key? I see from the implementation that it does a full
match, but this behavior should be documented.

What happens if 'key=...' is specified multiple times? Are the
multiple keys conjunctive? Disjunctive? Last-wins? I can see from the
implementation that it is last-wins, but this behavior should be
documented. (I wonder how painful it will be for people who want to
match multiple keys. This doesn't have to be answered yet, as the
behavior can always be loosened later to allow multiple-key matching
since the current syntax does not disallow such expansion.)

Thinking further on the last two points, should  be a regular expression?

> +  shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
> +  unfolds and shows trailer lines with key `Reviewed-by`.
> diff --git a/pretty.c b/pretty.c
> @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* 
> in UTF-8 */
> +   opts.filter_key = xstrndup(arg, end - 
> arg);
> @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* 
> in UTF-8 */
> format_trailers_from_commit(sb, msg + c->subject_off, 
> );
> }
> +   free(opts.filter_key);

If I understand correctly, this is making a copy of  so that it
will be NUL-terminated since the code added to trailer.c uses a simple
strcasecmp() to match it. Would it make sense to avoid the copy by
adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
strncasecmp() instead? (Genuine question; not necessarily a request
for change.)

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
> +   git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
> +   {
> +   echo "Acked-by: A U Thor " &&
> +   echo
> +   } >expect &&
> +   test_cmp expect actual
> +'

I guess these new tests are modeled after one or two existing tests
which use a series of 'echo' statements, but an alternative would be:

cat <<-\EOF >expect &&
Acked-by: A U Thor 

EOF

or, even:

test_write_lines "Acked-by: A U Thor " "" &&

though, that's probably less readable.

[PATCH/RFC v2] sequencer.c: record revert/cherry-pick commit with trailer lines

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

When a commit is reverted (or cherry-picked with -x) we add an English
sentence recording that commit id in the new commit message. Make
these real trailer lines instead so that they are more friendly to
parsers (especially "git interpret-trailers").

A reverted commit will have a new trailer

Revert: 

Similarly a cherry-picked commit with -x will have

Cherry-picked-from: 

When reverting or cherry picking a merge, the reverted/cherry-picked
branch will be shown using extended SHA-1 syntax, e.g.

Revert: ~2

Since we're not producing the old lines "This reverts commit ..." and
"(cherry picked from commit .." anymore, scripts that look for these
lines will need to be updated to handle both. Fresh new history could
just rely on git-interpret-trailers instead.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 v2 adds the third syntax "~2" and renames Cherry-Pick: to
 Cherry-picked-from: and acknowledge the problem with breaking
 scripts. I don't have a pretty solution for that though.

 Documentation/git-cherry-pick.txt |  5 ++---
 sequencer.c   | 33 +--
 t/t3510-cherry-pick-sequence.sh   | 12 +--
 t/t3511-cherry-pick-x.sh  | 26 
 4 files changed, 39 insertions(+), 37 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index d35d771fc8..54e73e74c0 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -58,9 +58,8 @@ OPTIONS
message prior to committing.
 
 -x::
-   When recording the commit, append a line that says
-   "(cherry picked from commit ...)" to the original commit
-   message in order to indicate which commit this change was
+   When recording the commit, append "Cherry-picked-from:" trailer line
+   recording the commit name which commit this change was
cherry-picked from.  This is done only for cherry
picks without conflicts.  Do not use this option if
you are cherry-picking from your private branch because
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..f804406b68 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -36,7 +36,6 @@
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 const char sign_off_header[] = "Signed-off-by: ";
-static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
 
@@ -1669,7 +1668,7 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
char *author = NULL;
struct commit_message msg = { NULL, NULL, NULL, NULL };
struct strbuf msgbuf = STRBUF_INIT;
-   int res, unborn = 0, allow;
+   int res, unborn = 0, allow, parent_id = -1;
 
if (opts->no_commit) {
/*
@@ -1716,6 +1715,7 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
return error(_("commit %s does not have parent %d"),
oid_to_hex(>object.oid), 
opts->mainline);
parent = p->item;
+   parent_id = cnt;
} else if (0 < opts->mainline)
return error(_("mainline was specified but commit %s is not a 
merge."),
oid_to_hex(>object.oid));
@@ -1758,16 +1758,15 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
-   strbuf_addstr(, "Revert \"");
-   strbuf_addstr(, msg.subject);
-   strbuf_addstr(, "\"\n\nThis reverts commit ");
-   strbuf_addstr(, oid_to_hex(>object.oid));
-
-   if (commit->parents && commit->parents->next) {
-   strbuf_addstr(, ", reversing\nchanges made to ");
-   strbuf_addstr(, oid_to_hex(>object.oid));
-   }
-   strbuf_addstr(, ".\n");
+   strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
+
+   if (parent_id != -1)
+   strbuf_addf(, "Revert: %s~%d\n",
+   oid_to_hex(>object.oid),
+   parent_id);
+   else
+   strbuf_addf(, "Revert: %s\n",
+   oid_to_hex(>object.oid));
} else {
const char *p;
 
@@ -1784,9 +1783,13 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
strbuf_complete_line();
if (!has_conforming_footer(, NULL, 0))
strbuf_addch(, '\n');
-   strbuf_addstr(, cherry_picked_prefix);
-   strbuf_addstr(, oid_to_hex(>object.oid));
-   strbuf_addstr(, ")\n");
+   if (parent_id != -1)
+   

if YOU use a Windows GUI for Git, i would appreciate knowing which one and why

[_g e r r y _ _l o w r y _]

PREAMBLE [START] - please feel free to skip this first section

Forgive me for asking this question on a mailing list.

stackoverflow would probably kill such a question before the bits were fully 
saved to a server drive.

Let me explain why i am asking and why i am not being a troll.

[a] i'm "old school", i.e., > 50% on my way to being age 72 [born 1947]

[b] when i started programming in 1967, most of my work input was via punched 
cards

[c] punching my own cards was cool

[d] IBM System/360 mainframe assembler was cool and patching previously punched 
card encoded machine code output was a fun risky but
at times necessary challenge.

[e] using command windows and coding batch files for Gary Kildall's CP/M and 
the evil empire's PC/MS-DOS was how i accomplished many
tasks for early non-GUI environments (i still continue this practice even in 
Windows 10 (a.k.a. please don't update my PC O/S behind
my back again versions of MS Windows)).

[f] my introduction to Git was via a command line based awesome video that has 
disappeared (i asked this community about that in a
previous thread).

BOTTOM LINE:  virtually 100% of my Git use has been via Git Bash command line 
[probably downloaded from https://git-scm.com/]

For me, and i suspect even for most people who live with GUI platforms, [a well 
kept secret fact] using the keyboard is faster than
using the mouse [especially when one's fingers are already over one's 
keyboard-example, closing one or more "windows" via Alt+F4.

Also for me, i am happy to change some code and/or write some new code, Alt+Tab 
to Git Bash frequently, ADD/COMMIT, then Alt+Tab
back to whatever IDE i'm using [mostly LINQPad and vs2017]; i know that's quite 
a bit schizophrenic of me-command line Git but GUI
IDE.

PREAMBLE [END]


QUESTION:  if YOU use a Windows GUI for Git, i would appreciate knowing which 
one and why

i have been asked to look at GUI versions of Git for Windows.

https://git-scm.com/download/gui/windows currently lists 22 options.

if i had more time left in my life and the option, because of my own nature, 
i'd likely download and evaluate all 22 - Mr.T would
pity the fool that i often can be.

CAUTION:  i am not looking for anyone to disparage other Git Windows GUIs.

Let me break down the question into 4 parts:

[1a] Which do you prefer:  Git GUI, Git command line?
[1b] What is your reason for your [1a] preference?

[2a] if applicable, which Git GUI do you prefer?
[2b] What is your reason for your [2a] preference?


if you are uncomfortable replying to git@vger.kernel.org please feel free to 
reply directly to my e-mail address.

i look forward to hearing from members of this Git community.

Thank you for reading and thank you for your valuable time.

gerry (lowry)-wasaga beach-ontario-canada
gerry.lo...@abilitybusinesscomputerservices.com

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

[Duy Nguyen]

On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood  wrote:
>
> On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:
> > When a commit is reverted (or cherry-picked with -x) we add an English
> > sentence recording that commit id in the new commit message. Make
> > these real trailer lines instead so that they are more friendly to
> > parsers (especially "git interpret-trailers").
> >
> > A reverted commit will have a new trailer
> >
> >  Revert: 
> >
> > Similarly a cherry-picked commit with -x will have
> >
> >  Cherry-Pick: 
>
> I think this is a good idea though I wonder if it will break someones
> script that is looking for the messages generated by -x at the moment.

It will [1] but I still think it's worth the trouble. The script will
be less likely to break after, and you can use git-interpret-trailers
instead of plain grep.

[1] 
https://public-inbox.org/git/20181017143921.gr270...@devbig004.ftw2.facebook.com/

> > @@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command 
> > command, struct commit *commit,
> >   base_label = msg.label;
> >   next = parent;
> >   next_label = msg.parent_label;
> > - strbuf_addstr(, "Revert \"");
> > - strbuf_addstr(, msg.subject);
> > - strbuf_addstr(, "\"\n\nThis reverts commit ");
> > - strbuf_addstr(, oid_to_hex(>object.oid));
> > -
> > - if (commit->parents && commit->parents->next) {
> > - strbuf_addstr(, ", reversing\nchanges made to 
> > ");
> > - strbuf_addstr(, 
> > oid_to_hex(>object.oid));
> > - }
>
> As revert currently records the parent given on the command line when
> reverting a merge commit it would probably be a good idea to add that
> either as a separate trailer or to the Revert: trailer and possibly also
> generate it for cherry picks.

My mistake. I didn't read carefully and thought it was logging
commit->parents, which is pointless.

So what should be the trailer for this (I don't think putting it in
Revert: is a good idea, too much to parse)? Revert-parent: ?
Revert-merge: ?

> > - strbuf_addstr(, ".\n");
> > + strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
>
> If the message already contains trailers then should we just append the
> Revert trailer those rather than inserting "\n\n"?

Umm.. but this \n\n is for separating the subject and the body. I
think we need it anyway, trailer or not.

> > @@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, 
> > struct commit *commit,
> >   strbuf_complete_line();
> >   if (!has_conforming_footer(, NULL, 0))
> >   strbuf_addch(, '\n');
> > - strbuf_addstr(, cherry_picked_prefix);
> > - strbuf_addstr(, 
> > oid_to_hex(>object.oid));
> > - strbuf_addstr(, ")\n");
> > + strbuf_addf(, "Cherry-Pick: %s\n",
> > + oid_to_hex(>object.oid));

I will probably make this "Cherry-picked-from:" to match our S-o-b style.
-- 
Duy

Re: [PATCH v2 0/5] %(trailers) improvements in pretty format

[Eric Sunshine]

On Sun, Nov 4, 2018 at 10:23 AM Anders Waldenborg  wrote:
> This adds support for three new options to %(trailers):
>  * key -- show only trailers with specified key
>  * nokey -- don't show key part of trailers
>  * separator -- allow specifying custom separator between trailers

If "key" is for including particular trailers, intuition might lead
people to think that "nokey" is for excluding certain trailers.
Perhaps a different name for "nokey", such as "valueonly" or
"stripkey", would be better.

Re: git-rebase is ignoring working-tree-encoding

[Torsten Bögershausen]

On Fri, Nov 02, 2018 at 03:30:17AM +0100, Adrián Gimeno Balaguer wrote:
> I’m attempting to perform fixups via git-rebase of UTF-16 LE files
> (the project I’m working on requires that exact encoding on certain
> files). When the rebase is complete, Git changes that file’s encoding
> to UTF-16 BE. I have been using the newer working-tree-encoding
> attribute in .gitattributes. I’m using Git for Windows.
> 
> $ git version
> git version 2.19.1.windows.1
> 
> Here is a sample UTF-16 LE file (with BOM and LF endings) with
> following atributes in .gitattributes:
> 
> test.txt eol=lf -text working-tree-encoding=UTF-16
> 
> I put eol=lf and -text to tell Git to not change the encoding of the
> file on checkout, but that doesn’t even help. Asides, the newer
> working-tree-encoding allows me to view human-readable diffs of that
> file (in GitHub Desktop and Git Bash). Now, note that doing for
> example consecutive commits to the same file does not affect the
> UTF-16 LE encoding. And before I discovered this attribute, the whole
> thing was even worse when squash/fixup rebasing, as Git would modify
> the file with Chinese characters (when manually setting it as text via
> .gitattributes).
> 
> So, again the problem with the exposed .gitattributes line is that
> after fixup rebasing, UTF-16 LE files encoding change to UTF-16 BE.
> 
> For long, I have been working with the involved UTF-16 LE files set as
> binary via .gitattributes (e.g. “test.txt binary”), so that Git would
> not modify the file encoding, but this doesn’t allow me to view the
> diffs upon changes in GitHub Desktop, which I want (and neither via
> git diff).

Thanks for the report.
I have tried to follow the problem from your verbal descriptions
(and the PR) but I need to admit that I don't fully understand the
problem (yet).

Could you try to create some instructions how to reproduce it?
A numer of shell istructions would be great,
in best case some kind of "test case", like the tests in
the t/ directory in Git.

It would be nice to be able to re-produce it.
And if there is a bug, to get it fixed.

Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

[Phillip Wood]

On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:

When a commit is reverted (or cherry-picked with -x) we add an English
sentence recording that commit id in the new commit message. Make
these real trailer lines instead so that they are more friendly to
parsers (especially "git interpret-trailers").

A reverted commit will have a new trailer

 Revert: 

Similarly a cherry-picked commit with -x will have

 Cherry-Pick: 


I think this is a good idea though I wonder if it will break someones 
script that is looking for the messages generated by -x at the moment. 
I've got a couple of comments below.



Signed-off-by: Nguyễn Thái Ngọc Duy 
---
  I think standardizing how we record commit ids in the commit message
  is a good idea. Though to be honest this started because of my irk of
  an English string "cherry picked from..." that cannot be translated.
  It might as well be a computer language that happens to look like
  English.

  Documentation/git-cherry-pick.txt |  5 ++---
  sequencer.c   | 20 ++--
  t/t3510-cherry-pick-sequence.sh   | 12 ++--
  t/t3511-cherry-pick-x.sh  | 26 +-
  4 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index d35d771fc8..8ffbaed1a0 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -58,9 +58,8 @@ OPTIONS
message prior to committing.
  
  -x::

-   When recording the commit, append a line that says
-   "(cherry picked from commit ...)" to the original commit
-   message in order to indicate which commit this change was
+   When recording the commit, append "Cherry-Pick:" trailer line
+   recording the commit name which commit this change was
cherry-picked from.  This is done only for cherry
picks without conflicts.  Do not use this option if
you are cherry-picking from your private branch because
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..f7318f2862 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -36,7 +36,6 @@
  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
  
  const char sign_off_header[] = "Signed-off-by: ";

-static const char cherry_picked_prefix[] = "(cherry picked from commit ";
  
  GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
  
@@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,

base_label = msg.label;
next = parent;
next_label = msg.parent_label;
-   strbuf_addstr(, "Revert \"");
-   strbuf_addstr(, msg.subject);
-   strbuf_addstr(, "\"\n\nThis reverts commit ");
-   strbuf_addstr(, oid_to_hex(>object.oid));
-
-   if (commit->parents && commit->parents->next) {
-   strbuf_addstr(, ", reversing\nchanges made to ");
-   strbuf_addstr(, oid_to_hex(>object.oid));
-   }


As revert currently records the parent given on the command line when 
reverting a merge commit it would probably be a good idea to add that 
either as a separate trailer or to the Revert: trailer and possibly also 
generate it for cherry picks.



-   strbuf_addstr(, ".\n");
+   strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);


If the message already contains trailers then should we just append the 
Revert trailer those rather than inserting "\n\n"?


Best Wishes

Phillip


+
+   strbuf_addf(, "Revert: %s\n",
+   oid_to_hex(>object.oid));



} else {
const char *p;
  
@@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,

strbuf_complete_line();
if (!has_conforming_footer(, NULL, 0))
strbuf_addch(, '\n');
-   strbuf_addstr(, cherry_picked_prefix);
-   strbuf_addstr(, oid_to_hex(>object.oid));
-   strbuf_addstr(, ")\n");
+   strbuf_addf(, "Cherry-Pick: %s\n",
+   oid_to_hex(>object.oid));
}
if (!is_fixup(command))
author = get_author(msg.message);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeefdc9..89b6e7fc1e 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
-   ! grep "cherry picked from" initial_msg &&
-   grep "cherry picked from" unrelatedpick_msg &&
-   grep "cherry picked from" picked_msg &&
-   grep "cherry picked from" 

Re: git-rebase is ignoring working-tree-encoding

[Adrián Gimeno Balaguer]

El dom., 4 nov. 2018 a las 16:48, brian m. carlson
() escribió:
> Do things work for you if you write this as "UTF-16LE"?  When you use
> working-tree-encoding, the file is stored internally as UTF-8, but it's
> serialized to the specified encoding when written out.

When I use UTF-16LE or UTF-16BE, then I can't commit or view diffs of
specified files, as Git prohibites BOM existance in these cases,
showing an error when attempting to commit. But BOM must also exist
for the project. I even experimented for fixing this issue within
Git's source. It turns out that Git is following an Unicode rule that
says that BOM is not permitted when declaring exact UTF-16BE/UTF-16LE
MIME (and UTF-32 variants) encoding types:

https://github.com/git/git/blob/master/utf8.h#L87

> Asking for "UTF-16" is ambiguous: there are two endiannesses, and so as
> long as you get a BOM in the output, either one is an acceptable option.
> Which one you get is dependent on what the underlying code thinks is the
> default, and traditionally for Unix systems and Unix tools that's been
> big-endian.  If you want a particular endianness, you should specify it.

I wrote a "counterpart" easy fix which instead only prohibites BOM for
the opposite endianness (for example if
working-tree-encoding=UTF-16LE, then finding an UTF-16BE BOM in the
file would cause Git to signal the error right before committing,
diffing, etc.). That way user would be encouraged to modify the file's
encoding to match the one specified in working-tree-encoding before
allowing these actions, therefore preventing Git from encoding to the
wrong endianness after file is written out. With few repository tests,
this new behaviour worked as expected. But then I realized this
solution would perhaps be unacceptable for Git's source code as it
would violate that Unicode standard. Anyways, here is a PR in my Git
fork with the changes I did, for reference:

https://github.com/AdRiAnIlloO/git/pull/1

Ah this point, the solution I came with recently for my project was
writing some code in Shell to fix the endianness of the re-encoded
files to UTF-16BE after the Git's write out process (or a "working
tree refresh" in my own words), within the same script that I use to
pack assets including the localization files.

> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204



-- 
Adrián

Re: [PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key

[Michał Górny]

On Sun, 2018-11-04 at 15:10 +, brian m. carlson wrote:
> On Sun, Nov 04, 2018 at 10:47:10AM +0100, Michał Górny wrote:
> > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> > index e8377286d..86d3f93fa 100755
> > --- a/t/t7510-signed-commit.sh
> > +++ b/t/t7510-signed-commit.sh
> > @@ -197,9 +197,9 @@ test_expect_success GPG 'show bad signature with custom 
> > format' '
> >  test_expect_success GPG 'show untrusted signature with custom format' '
> > cat >expect <<-\EOF &&
> > U
> > -   61092E85B7227189
> > +   65A0EEA02E30CAD7
> > Eris Discordia 
> > -   D4BE22311AD3131E5EDA29A461092E85B7227189
> > +   F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
> > D4BE22311AD3131E5EDA29A461092E85B7227189
> > EOF
> > git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual 
> > &&
> > @@ -209,7 +209,7 @@ test_expect_success GPG 'show untrusted signature with 
> > custom format' '
> >  test_expect_success GPG 'show unknown signature with custom format' '
> > cat >expect <<-\EOF &&
> > E
> > -   61092E85B7227189
> > +   65A0EEA02E30CAD7
> 
> It's my understanding that GnuPG will use the most recent subkey
> suitable for a particular purpose, and I think the test relies on that
> behavior.  However, I'm not sure that's documented.  Do we want to rely
> on that behavior or be more explicit?  (This is a question, not an
> opinion.)

To be honest, I don't recall which suitable subkey is used.  However, it
definitely will prefer a subkey with signing capabilities over
the primary key if one is present, and this is well-known and expected
behavior.

In fact, if you have a key with two signing subkeys A and B and it
considers A better, then even if you explicitly pass keyid of B, it will
use A.  To force another subkey you have to append '!' to keyid.

Therefore, I think this is a behavior we can rely on.

-- 
Best regards,
Michał Górny


signature.asc
Description: This is a digitally signed message part

Re: git-rebase is ignoring working-tree-encoding

[brian m. carlson]

On Fri, Nov 02, 2018 at 03:30:17AM +0100, Adrián Gimeno Balaguer wrote:
> I’m attempting to perform fixups via git-rebase of UTF-16 LE files
> (the project I’m working on requires that exact encoding on certain
> files). When the rebase is complete, Git changes that file’s encoding
> to UTF-16 BE. I have been using the newer working-tree-encoding
> attribute in .gitattributes. I’m using Git for Windows.
> 
> $ git version
> git version 2.19.1.windows.1
> 
> Here is a sample UTF-16 LE file (with BOM and LF endings) with
> following atributes in .gitattributes:
> 
> test.txt eol=lf -text working-tree-encoding=UTF-16

Do things work for you if you write this as "UTF-16LE"?  When you use
working-tree-encoding, the file is stored internally as UTF-8, but it's
serialized to the specified encoding when written out.

Asking for "UTF-16" is ambiguous: there are two endiannesses, and so as
long as you get a BOM in the output, either one is an acceptable option.
Which one you get is dependent on what the underlying code thinks is the
default, and traditionally for Unix systems and Unix tools that's been
big-endian.  If you want a particular endianness, you should specify it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH v2 3/5] pretty: add support for "nokey" option in %(trailers)

[Anders Waldenborg]

With the new "key=" option to %trailers it often makes little sense to
show the key, as it by definition already is know which trailer is
printed there. This new "nokey" option makes it omit key trailer key
when printing trailers.

E.g.:
 $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,nokey)' 88182
will show:
 > upload-pack: fix broken if/else chain in config callback
 > Jeff King 
 > Junio C Hamano 

Signed-off-by: Anders Waldenborg 
---
 Documentation/pretty-formats.txt | 11 ++-
 pretty.c |  2 ++
 t/t4205-log-pretty-formats.sh|  9 +
 trailer.c|  6 --
 trailer.h|  1 +
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8326fc45e..e115e355d 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -212,11 +212,12 @@ endif::git-rev-list[]
   followed by a colon and zero or more comma-separated options. The
   allowed options are `only` which omits non-trailer lines from the
   trailer block, `unfold` to make it behave as if interpret-trailer's
-  `--unfold` option was given, and `key=T` to only show trailers with
-  specified key (matching is done
-  case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and
-  shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
-  unfolds and shows trailer lines with key `Reviewed-by`.
+  `--unfold` option was given, `key=T` to only show trailers with
+  specified key (matching is done case-insensitively), and `nokey`
+  which makes it skip over the key part of the trailer and only show
+  value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer
+  lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
+  trailer lines with key `Reviewed-by`.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index cdca9dce2..f87ba4f18 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1323,6 +1323,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
opts.only_trailers = 1;
else if (match_placeholder_arg(arg, "unfold", 
))
opts.unfold = 1;
+   else if (match_placeholder_arg(arg, "nokey", 
))
+   opts.no_key = 1;
else if (skip_prefix(arg, "key=", )) {
const char *end = arg + strcspn(arg, 
",)");
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 0f5207242..e7de3b18a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -643,6 +643,15 @@ test_expect_success '%(trailers:key=foo,unfold) properly 
unfolds' '
test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,nokey) shows only value' '
+   git log --no-walk --pretty="%(trailers:key=Acked-by,nokey)" >actual &&
+   {
+   echo "A U Thor " &&
+   echo
+   } >expect &&
+   test_cmp expect actual
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
git commit --allow-empty -F - <<-\EOF &&
this is the subject
diff --git a/trailer.c b/trailer.c
index cbbb553e4..4f19c34cb 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,
if (!opts->filter_key || !strcasecmp (tok.buf, 
opts->filter_key)) {
if (opts->unfold)
unfold_value();
-
-   strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+   if (opts->no_key)
+   strbuf_addf(out, "%s\n", val.buf);
+   else
+   strbuf_addf(out, "%s: %s\n", tok.buf, 
val.buf);
}
strbuf_release();
strbuf_release();
diff --git a/trailer.h b/trailer.h
index d052d02ae..83de87ee9 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,7 @@ struct process_trailer_options {
int only_input;
int unfold;
int no_divider;
+   int no_key;
char *filter_key;
 };
 
-- 
2.17.1

[PATCH v2 2/5] pretty: allow showing specific trailers

[Anders Waldenborg]

Adds a new "key=X" option to "%(trailers)" which will cause it to only
print trailers lines which matches the specified key.

Signed-off-by: Anders Waldenborg 
---
 Documentation/pretty-formats.txt | 13 +
 pretty.c | 15 ++-
 t/t4205-log-pretty-formats.sh| 45 
 trailer.c|  8 +++---
 trailer.h|  1 +
 5 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..8326fc45e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -209,11 +209,14 @@ endif::git-rev-list[]
   respectively, but padding both sides (i.e. the text is centered)
 - %(trailers[:options]): display the trailers of the body as interpreted
   by linkgit:git-interpret-trailers[1]. The `trailers` string may be
-  followed by a colon and zero or more comma-separated options. If the
-  `only` option is given, omit non-trailer lines from the trailer block.
-  If the `unfold` option is given, behave as if interpret-trailer's
-  `--unfold` option was given.  E.g., `%(trailers:only,unfold)` to do
-  both.
+  followed by a colon and zero or more comma-separated options. The
+  allowed options are `only` which omits non-trailer lines from the
+  trailer block, `unfold` to make it behave as if interpret-trailer's
+  `--unfold` option was given, and `key=T` to only show trailers with
+  specified key (matching is done
+  case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and
+  shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
+  unfolds and shows trailer lines with key `Reviewed-by`.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index aa03d5b23..cdca9dce2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
opts.only_trailers = 1;
else if (match_placeholder_arg(arg, "unfold", 
))
opts.unfold = 1;
-   else
+   else if (skip_prefix(arg, "key=", )) {
+   const char *end = arg + strcspn(arg, 
",)");
+
+   if (opts.filter_key)
+   free(opts.filter_key);
+
+   opts.filter_key = xstrndup(arg, end - 
arg);
+   arg = end;
+   if (*arg == ',')
+   arg++;
+
+   opts.only_trailers = 1;
+   } else
break;
}
}
@@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
format_trailers_from_commit(sb, msg + c->subject_off, 
);
ret = arg - placeholder + 1;
}
+   free(opts.filter_key);
return ret;
}
 
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..0f5207242 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
+   git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
+   {
+   echo "Acked-by: A U Thor " &&
+   echo
+   } >expect &&
+   test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '
+   git log --no-walk --pretty="%(trailers:key=AcKed-bY)" >actual &&
+   {
+   echo "Acked-by: A U Thor " &&
+   echo
+   } >expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=nonexistant) becomes empty' '
+   git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
+   {
+   echo "xx"
+   } >expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if 
folded' '
+   git log --no-walk --pretty="%(trailers:key=Signed-Off-by)" >actual &&
+   {
+   grep -v patch.description expect &&
+   test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+   git log --no-walk --pretty="%(trailers:key=Signed-Off-by,unfold)" 
>actual &&
+   {
+   echo "Signed-off-by: A U Thor " &&
+   echo "Signed-off-by: A U Thor " &&
+   echo
+ 

[PATCH v2 4/5] pretty: extract fundamental placeholders to separate function

[Anders Waldenborg]

No functional change intended

Signed-off-by: Anders Waldenborg 
---
 pretty.c | 37 ++---
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index f87ba4f18..9fdddce9d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, 
const char *candidate,
return 0;
 }
 
+static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */
+const char *placeholder,
+void *context)
+{
+   int ch;
+
+   switch (placeholder[0]) {
+   case 'n':   /* newline */
+   strbuf_addch(sb, '\n');
+   return 1;
+   case 'x':
+   /* %x00 == NUL, %x0a == LF, etc. */
+   ch = hex2chr(placeholder + 1);
+   if (ch < 0)
+   return 0;
+   strbuf_addch(sb, ch);
+   return 3;
+   }
+   return 0;
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
const char *msg = c->message;
struct commit_list *p;
const char *arg;
-   int ch;
+   size_t res;
 
/* these are independent of the commit */
+   res = format_fundamental(sb, placeholder, NULL);
+   if (res)
+   return res;
+
switch (placeholder[0]) {
case 'C':
if (starts_with(placeholder + 1, "(auto)")) {
@@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
 */
return ret;
}
-   case 'n':   /* newline */
-   strbuf_addch(sb, '\n');
-   return 1;
-   case 'x':
-   /* %x00 == NUL, %x0a == LF, etc. */
-   ch = hex2chr(placeholder + 1);
-   if (ch < 0)
-   return 0;
-   strbuf_addch(sb, ch);
-   return 3;
case 'w':
if (placeholder[1] == '(') {
unsigned long width = 0, indent1 = 0, indent2 = 0;
-- 
2.17.1

[PATCH v2 5/5] pretty: add support for separator option in %(trailers)

[Anders Waldenborg]

By default trailer lines are terminated by linebreaks ('\n'). By
specifying the new 'separator' option they will instead be separated by
user provided string and have separator semantics rather than terminator
semantics. The separator string can contain the fundamental formatting
codes %n and %xNN allowing it to be things that are otherwise hard to
type as %x00, or command and end-parenthesis which would break parsing.

E.g:
 $ git log --pretty='%(trailers:key=Reviewed-by,nokey,separator=%x00)'

Signed-off-by: Anders Waldenborg 
---
 Documentation/pretty-formats.txt | 13 -
 pretty.c | 13 +
 t/t4205-log-pretty-formats.sh|  6 ++
 trailer.c| 20 +---
 trailer.h|  1 +
 5 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index e115e355d..3312850e6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -213,11 +213,14 @@ endif::git-rev-list[]
   allowed options are `only` which omits non-trailer lines from the
   trailer block, `unfold` to make it behave as if interpret-trailer's
   `--unfold` option was given, `key=T` to only show trailers with
-  specified key (matching is done case-insensitively), and `nokey`
-  which makes it skip over the key part of the trailer and only show
-  value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer
-  lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
-  trailer lines with key `Reviewed-by`.
+  specified key (matching is done case-insensitively), `nokey` which
+  makes it skip over the key part of the trailer and only show value
+  and `separator` which allows specifying an alternative separator
+  than the default line
+  break. E.g. `%(trailers:only,unfold,separator=%x00)` unfolds and
+  shows all trailer lines separated by NUL character,
+  `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer lines
+  with key `Reviewed-by`.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 9fdddce9d..f73a2b0dc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1327,6 +1327,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
+   struct strbuf sepbuf = STRBUF_INIT;
size_t ret = 0;
 
opts.no_divider = 1;
@@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
arg++;
 
opts.only_trailers = 1;
+   } else if (skip_prefix(arg, "separator=", 
)) {
+   size_t seplen = strcspn(arg, ",)");
+   strbuf_reset();
+   char *fmt = xstrndup(arg, seplen);
+   strbuf_expand(, fmt, 
format_fundamental, NULL);
+   free(fmt);
+   opts.separator = 
+
+   arg += seplen;
+   if (*arg == ',')
+   arg++;
} else
break;
}
@@ -1360,6 +1372,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
format_trailers_from_commit(sb, msg + c->subject_off, 
);
ret = arg - placeholder + 1;
}
+   strbuf_release ();
free(opts.filter_key);
return ret;
}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e7de3b18a..71218d22e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -652,6 +652,12 @@ test_expect_success '%(trailers:key=foo,nokey) shows only 
value' '
test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:separator) changes separator' '
+   git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" 
>actual &&
+   printf "XSigned-off-by: A U Thor \0Acked-by: A U 
Thor \0[ v2 updated patch description ]\0Signed-off-by: A U 
Thor X" >expect &&
+   test_cmp expect actual
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
git commit --allow-empty -F - <<-\EOF &&
this is the subject
diff --git a/trailer.c b/trailer.c
index 4f19c34cb..a79e4e36a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,
const struct 

[PATCH v2 1/5] pretty: single return path in %(trailers) handling

[Anders Waldenborg]

No functional change intended.

This change may not seem useful on its own, but upcoming commits will do
memory allocation in there, and a single return path makes deallocation
easier.

Signed-off-by: Anders Waldenborg 
---
 pretty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index b83a3ecd2..aa03d5b23 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1312,6 +1312,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
 
if (skip_prefix(placeholder, "(trailers", )) {
struct process_trailer_options opts = 
PROCESS_TRAILER_OPTIONS_INIT;
+   size_t ret = 0;
 
opts.no_divider = 1;
 
@@ -1328,8 +1329,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
}
if (*arg == ')') {
format_trailers_from_commit(sb, msg + c->subject_off, 
);
-   return arg - placeholder + 1;
+   ret = arg - placeholder + 1;
}
+   return ret;
}
 
return 0;   /* unknown placeholder */
-- 
2.17.1

[PATCH v2 0/5] %(trailers) improvements in pretty format

[Anders Waldenborg]

This adds support for three new options to %(trailers):
 * key -- show only trailers with specified key
 * nokey -- don't show key part of trailers
 * separator -- allow specifying custom separator between trailers


Anders Waldenborg (5):
  pretty: single return path in %(trailers) handling
  pretty: allow showing specific trailers
  pretty: add support for "nokey" option in %(trailers)
  pretty: extract fundamental placeholders to separate function
  pretty: add support for separator option in %(trailers)

 Documentation/pretty-formats.txt | 17 +---
 pretty.c | 71 ++--
 t/t4205-log-pretty-formats.sh| 60 +++
 trailer.c| 28 ++---
 trailer.h|  3 ++
 5 files changed, 156 insertions(+), 23 deletions(-)

-- 
2.17.1

Re: [PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key

[brian m. carlson]

On Sun, Nov 04, 2018 at 10:47:10AM +0100, Michał Górny wrote:
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index e8377286d..86d3f93fa 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -197,9 +197,9 @@ test_expect_success GPG 'show bad signature with custom 
> format' '
>  test_expect_success GPG 'show untrusted signature with custom format' '
>   cat >expect <<-\EOF &&
>   U
> - 61092E85B7227189
> + 65A0EEA02E30CAD7
>   Eris Discordia 
> - D4BE22311AD3131E5EDA29A461092E85B7227189
> + F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
>   D4BE22311AD3131E5EDA29A461092E85B7227189
>   EOF
>   git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual 
> &&
> @@ -209,7 +209,7 @@ test_expect_success GPG 'show untrusted signature with 
> custom format' '
>  test_expect_success GPG 'show unknown signature with custom format' '
>   cat >expect <<-\EOF &&
>   E
> - 61092E85B7227189
> + 65A0EEA02E30CAD7

It's my understanding that GnuPG will use the most recent subkey
suitable for a particular purpose, and I think the test relies on that
behavior.  However, I'm not sure that's documented.  Do we want to rely
on that behavior or be more explicit?  (This is a question, not an
opinion.)
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key

[Michał Górny]

Add a dedicated signing subkey to the key identified as 'Eris
Discordia', and update tests appropriately.  GnuPG will now sign commits
using the dedicated signing subkey, changing the value of %GK and %GF,
and effectively creating a test case for %GF!=%GP.

Signed-off-by: Michał Górny 
---
 t/lib-gpg/keyring.gpg| 62 
 t/t7510-signed-commit.sh |  6 ++--
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/t/lib-gpg/keyring.gpg b/t/lib-gpg/keyring.gpg
index d4754a1f1..918dfce33 100644
--- a/t/lib-gpg/keyring.gpg
+++ b/t/lib-gpg/keyring.gpg
@@ -30,7 +30,6 @@ Cezx4Q2khACcCs+/LtE8Lb9hC+2cvr3uH5p82AI=
 =aEiU
 -END PGP PRIVATE KEY BLOCK-
 -BEGIN PGP PRIVATE KEY BLOCK-
-Version: GnuPG v1
 
 lQOYBFFMlkcBCADJi/xnAF8yI34PHilSCbM7VtOFO17oFMkpu4cgN2QpPuM5MVjy
 cvrzKSguZFvPCDLzeAFJW1uPxL4SHaHSkisCrFhijH7OJWcOPNPSFCwu+inAoAsv
@@ -83,11 +82,43 @@ 
fn1sY/IG5atoKK+ypmV/TlBlMZqFQzuPIJQT8VLbmxtLlDhJG04LbI6c8axIZxOO
 ZKLy5nTTSy16ztqEeS7eifHLPZg1UFFyEEIQ1XW0CNDAeuWKh90ERjyl4Cg7PnWS
 Z9Ei+zj6JD5Pcdi3BJhQo9WOLOVEJ0NHmewTYqk9QVXH/0v1Hdl4LMJtgcbdbDWk
 4UTkXbg9pn3umCgkNJ3Vs8fWnIWO9Izdr2/wrFY2JvUT7Yvl+wsNIWatvOEzGy7n
-BOW78WUxzhu0YJTLKy+iKCjg5HS5dx6OC+e4aEEgfhNPCMkbvDsJjtQ=
-=hieJ
+BOW78WUxzhu0YJTLKy+iKCjg5HS5dx6OC+e4aEEgfhNPCMkbvDsJjtSdA5gEW967
+3AEIAKjseT0sTQjyN39fOn0fzxWp89REMUUKgLigb01MKuuNI3cedBZsz3hpFOKV
+cii5rldw8uf3yS3Okht2DfHPSD4NrGzLGEzSTpQ10S8N2q0DUYwyLU6C0U8HnMZm
+/n+lCGBbUoxvnruohAvKAjpHO3rmJ8D4De9hlWg/fwdAxQQ0Sve0kN8Vwk2p1GuO
+OWQKV1SU9c+kBiou7dewQmbilPRanKmP5ZSU4emhpTOMlJFXF+kmYSODQk1cMvWW
+Ob3ttll2llX0Gul7Sjf+haq/FcRyRk7Tw5MHwZjr5aWiCny0/0+byvfF6SBIfzyE
+qlyWURQ2gHZUqSiG3QPMZiYr04cAEQEAAQAH/Am4rv/oQF6wodgz5y4zc6JJiTDA
+4+nKdIuR7OKqUxk1oo7eZjJML/xvMumygNyUvJ9nodl1SlMKilOhdAswfkKj9gJY
+BdDJLm1OufhW3pJwy6ahbjeqEgwJFVENtSPF0zkuyED9kElrpbD2ZTGfzwdM0e9D
+10ZDFWtODCw8rzOFcijujgI8oilLtxSNrkkTKW+25WJFRNPSHgIkMIm8UlPAG+rj
+3Yj9UqodeXTSvXwG2zceOxjFJadV77sOFJDgwWslN6J8El4+GcgwFVepJxoZEj7e
+cKkmVr0Dc9/Q04D5dWATc1FYcIhZbTu3oImCAh45ep4u9WYLUV5PGyeMviEEAMwo
+mJbYBxWuPjpNa722HQcbvMUiZWWDwHfLCib/SaP0AgfDahid8/PcZwxOPHPByBrm
+GDi0z7ibn/pgJr07kpp1Cic9ntfc2FvkI0QMzG0EuiekzQyPEnzjoDHF+V4nJIj2
+GWVjLYYqlZWEmhsfKt1CnlPXBunKoDJ30ABPcHJ/BADT0WxAIVKF4lO2HlrDVP44
+bufBEG9Ct7dl/G08Qve4Ag3VEZpT82vEFp0LzX0mTCDIUKJUYAYLxAIPhP7IvIfc
+EZXrwyDUxU7YSgKTHMKo9nFC6fIc1GeGPRalIF1gmTY32qlYJC6y5BTDhZNV5ydG
+u8QL2P/orP7XuRrJyeyK+QP/XTekr/DS6Jkct826MPA52ciIkWVgYLatH5fO4HCq
+ssDU8vz7FbbvGs0G1Xn7GA4m9dNYVOZtKwX++3nf2IEOpgPiZVTn/nP2u3HutpJb
+/HMLlcfZGiGdxS6n/vdz6wsEobJoi6STkHkA+VFNOSZmdsw6eKl3X911tpCTYfOG
+2U47/IkCbAQYAQgAIBYhBNS+IjEa0xMeXtoppGEJLoW3InGJBQJb3rvcAhsCAUAJ
+EGEJLoW3InGJwHQgBBkBCAAdFiEE+DZKWeB//p9NYwBaZaDuoC4wytcFAlveu9wA
+CgkQZaDuoC4wytcD9gf/WigtHl7lFyl8RaE/uqROFEelZyM00v1h55fd/IGRG88E
+tN0Lr4FaqBqPkMZjU/LN9UMBaTd+748vHlHaweZqljXJu99CO9Id7Y4w7WzF3C3Y
+yQsGZ92EGxthsPK0+rhHV0MbaINupI1oO9gATFglSxq17o83FJatGRjaXCZau8jr
+57/By1MGtjk+Iq1NkzGkrX778LdRQGLKDw2Qa7lsdHY8d3lUPAH8mbb97ELmIc9t
+PG2aM7ATJL7nBmFuTHo6hmEcIw32Ei9KK1zxM0ZylEYkjBjHAlklWmKb9MiayMC5
+uHW7Iyhjl+NbgbIEr2JTamW/9tL6UrIIxiDEdqaHNfCaB/9D+V31Upcohc9azwB4
+AF8diQwt5nfiVpnVeF/W8+eS1By2W6QrwLNthNRabYFnuSf9USHAY6atDWe+egId
+MLIv4ce0i3ykoczSu0oMoUCMxdl9kQrsNHZCqWX/OiDDLSb05u/P/3he900y6tSB
+15MbIPA6i5Bw/693nHguqxS1ASbBB/LiIu3vCXdFEs9RMvIJ+qkP3xQA96oImQiK
+R3U6OGv593eONKijUINNqHRq6+UxIyJ+OCAi+L2QTidAhJLRCp6EZD96u02cthYq
+8KA8j1+rx9BcbeacVVHepeG1JsgxsXX8BTJ7ZuS5VVndZOjag8URW/9nJMf01w/h
+el64
+=Iv7W
 -END PGP PRIVATE KEY BLOCK-
 -BEGIN PGP PUBLIC KEY BLOCK-
-Version: GnuPG v1
 
 mQGiBEZnyykRBACzCPjIpTYNL7Y2tQqlEGTTDlvZcWNLjF5f7ZzuyOqNOidLUgFD
 36qch1LZLSZkShdR3Gae+bsolyjxrlFuFP0eXRPMtqK20aLw7WZvPFpEV1ThMne+
@@ -137,6 +168,25 @@ 
bGPyBuWraCivsqZlf05QZTGahUM7jyCUE/FS25sbS5Q4SRtOC2yOnPGsSGcTjmSi
 8uZ000stes7ahHku3onxyz2YNVBRchBCENV1tAjQwHrliofdBEY8peAoOz51kmfR
 Ivs4+iQ+T3HYtwSYUKPVjizlRCdDR5nsE2KpPUFVx/9L9R3ZeCzCbYHG3Ww1pOFE
 5F24PaZ97pgoJDSd1bPH1pyFjvSM3a9v8KxWNib1E+2L5fsLDSFmrbzhMxsu5wTl
-u/FlMc4btGCUyysvoigo4OR0uXcejgvnuGhBIH4TTwjJG7w7CY7U
-=iYv/
+u/FlMc4btGCUyysvoigo4OR0uXcejgvnuGhBIH4TTwjJG7w7CY7UuQENBFveu9wB
+CACo7Hk9LE0I8jd/Xzp9H88VqfPURDFFCoC4oG9NTCrrjSN3HnQWbM94aRTilXIo
+ua5XcPLn98ktzpIbdg3xz0g+DaxsyxhM0k6UNdEvDdqtA1GMMi1OgtFPB5zGZv5/
+pQhgW1KMb567qIQLygI6Rzt65ifA+A3vYZVoP38HQMUENEr3tJDfFcJNqdRrjjlk
+CldUlPXPpAYqLu3XsEJm4pT0Wpypj+WUlOHpoaUzjJSRVxfpJmEjg0JNXDL1ljm9
+7bZZdpZV9Brpe0o3/oWqvxXEckZO08OTB8GY6+Wlogp8tP9Pm8r3xekgSH88hKpc
+llEUNoB2VKkoht0DzGYmK9OHABEBAAGJAmwEGAEIACAWIQTUviIxGtMTHl7aKaRh
+CS6FtyJxiQUCW9673AIbAgFACRBhCS6FtyJxicB0IAQZAQgAHRYhBPg2Slngf/6f
+TWMAWmWg7qAuMMrXBQJb3rvcAAoJEGWg7qAuMMrXA/YH/1ooLR5e5RcpfEWhP7qk
+ThRHpWcjNNL9YeeX3fyBkRvPBLTdC6+BWqgaj5DGY1PyzfVDAWk3fu+PLx5R2sHm
+apY1ybvfQjvSHe2OMO1sxdwt2MkLBmfdhBsbYbDytPq4R1dDG2iDbqSNaDvYAExY
+JUsate6PNxSWrRkY2lwmWrvI6+e/wctTBrY5PiKtTZMxpK1++/C3UUBiyg8NkGu5
+bHR2PHd5VDwB/Jm2/exC5iHPbTxtmjOwEyS+5wZhbkx6OoZhHCMN9hIvSitc8TNG
+cpRGJIwYxwJZJVpim/TImsjAubh1uyMoY5fjW4GyBK9iU2plv/bS+lKyCMYgxHam
+hzXwmgf/Q/ld9VKXKIXPWs8AeABfHYkMLeZ34laZ1Xhf1vPnktQctlukK8CzbYTU

[PATCH 1/2] t/t7510-signed-commit.sh: Add %GP to custom format checks

[Michał Górny]

Test %GP in addition to %GF in custom format checks.  With current
keyring, both have the same value.

Signed-off-by: Michał Górny 
---
 t/t7510-signed-commit.sh | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 19ccae286..e8377286d 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -176,8 +176,9 @@ test_expect_success GPG 'show good signature with custom 
format' '
13B6F51ECDDE430D
C O Mitter 
73D758744BE721698EC54E8713B6F51ECDDE430D
+   73D758744BE721698EC54E8713B6F51ECDDE430D
EOF
-   git log -1 --format="%G?%n%GK%n%GS%n%GF" sixth-signed >actual &&
+   git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
test_cmp expect actual
 '
 
@@ -187,8 +188,9 @@ test_expect_success GPG 'show bad signature with custom 
format' '
13B6F51ECDDE430D
C O Mitter 
 
+
EOF
-   git log -1 --format="%G?%n%GK%n%GS%n%GF" $(cat forged1.commit) >actual 
&&
+   git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) 
>actual &&
test_cmp expect actual
 '
 
@@ -198,8 +200,9 @@ test_expect_success GPG 'show untrusted signature with 
custom format' '
61092E85B7227189
Eris Discordia 
D4BE22311AD3131E5EDA29A461092E85B7227189
+   D4BE22311AD3131E5EDA29A461092E85B7227189
EOF
-   git log -1 --format="%G?%n%GK%n%GS%n%GF" eighth-signed-alt >actual &&
+   git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual 
&&
test_cmp expect actual
 '
 
@@ -209,8 +212,9 @@ test_expect_success GPG 'show unknown signature with custom 
format' '
61092E85B7227189
 
 
+
EOF
-   GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 
--format="%G?%n%GK%n%GS%n%GF" eighth-signed-alt >actual &&
+   GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 
--format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
test_cmp expect actual
 '
 
@@ -220,8 +224,9 @@ test_expect_success GPG 'show lack of signature with custom 
format' '
 
 
 
+
EOF
-   git log -1 --format="%G?%n%GK%n%GS%n%GF" seventh-unsigned >actual &&
+   git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual 
&&
test_cmp expect actual
 '
 
@@ -261,8 +266,9 @@ test_expect_success GPG 'show double signature with custom 
format' '
 
 
 
+
EOF
-   git log -1 --format="%G?%n%GK%n%GS%n%GF" $(cat double-commit.commit) 
>actual &&
+   git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat 
double-commit.commit) >actual &&
test_cmp expect actual
 '
 
-- 
2.19.1

Re: Git Test Coverage Report (Friday, Nov 2)

[Junio C Hamano]

Michał Górny  writes:

> I've just did a little research and came to the following results:

Wonderful.

> 1. modifying the 'C. O. Mitter' key would require changes to 4 tests,
>
> 2. modifying the 'Eris Discordia' key would require changes to 2 tests
>(both in 7510).
>
> Do you think 2. would be an acceptable option?

Yeah, that sounds like the best way to go.  Thanks for digging.

[PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

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

When a commit is reverted (or cherry-picked with -x) we add an English
sentence recording that commit id in the new commit message. Make
these real trailer lines instead so that they are more friendly to
parsers (especially "git interpret-trailers").

A reverted commit will have a new trailer

Revert: 

Similarly a cherry-picked commit with -x will have

Cherry-Pick: 

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I think standardizing how we record commit ids in the commit message
 is a good idea. Though to be honest this started because of my irk of
 an English string "cherry picked from..." that cannot be translated.
 It might as well be a computer language that happens to look like
 English.

 Documentation/git-cherry-pick.txt |  5 ++---
 sequencer.c   | 20 ++--
 t/t3510-cherry-pick-sequence.sh   | 12 ++--
 t/t3511-cherry-pick-x.sh  | 26 +-
 4 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index d35d771fc8..8ffbaed1a0 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -58,9 +58,8 @@ OPTIONS
message prior to committing.
 
 -x::
-   When recording the commit, append a line that says
-   "(cherry picked from commit ...)" to the original commit
-   message in order to indicate which commit this change was
+   When recording the commit, append "Cherry-Pick:" trailer line
+   recording the commit name which commit this change was
cherry-picked from.  This is done only for cherry
picks without conflicts.  Do not use this option if
you are cherry-picking from your private branch because
diff --git a/sequencer.c b/sequencer.c
index 9e1ab3a2a7..f7318f2862 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -36,7 +36,6 @@
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 const char sign_off_header[] = "Signed-off-by: ";
-static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
 
@@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
base_label = msg.label;
next = parent;
next_label = msg.parent_label;
-   strbuf_addstr(, "Revert \"");
-   strbuf_addstr(, msg.subject);
-   strbuf_addstr(, "\"\n\nThis reverts commit ");
-   strbuf_addstr(, oid_to_hex(>object.oid));
-
-   if (commit->parents && commit->parents->next) {
-   strbuf_addstr(, ", reversing\nchanges made to ");
-   strbuf_addstr(, oid_to_hex(>object.oid));
-   }
-   strbuf_addstr(, ".\n");
+   strbuf_addf(, "Revert \"%s\"\n\n", msg.subject);
+
+   strbuf_addf(, "Revert: %s\n",
+   oid_to_hex(>object.oid));
} else {
const char *p;
 
@@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, 
struct commit *commit,
strbuf_complete_line();
if (!has_conforming_footer(, NULL, 0))
strbuf_addch(, '\n');
-   strbuf_addstr(, cherry_picked_prefix);
-   strbuf_addstr(, oid_to_hex(>object.oid));
-   strbuf_addstr(, ")\n");
+   strbuf_addf(, "Cherry-Pick: %s\n",
+   oid_to_hex(>object.oid));
}
if (!is_fixup(command))
author = get_author(msg.message);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeefdc9..89b6e7fc1e 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -390,10 +390,10 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
-   ! grep "cherry picked from" initial_msg &&
-   grep "cherry picked from" unrelatedpick_msg &&
-   grep "cherry picked from" picked_msg &&
-   grep "cherry picked from" anotherpick_msg
+   ! grep "Cherry-Pick:" initial_msg &&
+   grep "Cherry-Pick: " unrelatedpick_msg &&
+   grep "Cherry-Pick: " picked_msg &&
+   grep "Cherry-Pick: " anotherpick_msg
 '
 
 test_expect_success '--continue of single-pick respects -x' '
@@ -404,7 +404,7 @@ test_expect_success '--continue of single-pick respects -x' 
'
git cherry-pick --continue &&
test_path_is_missing .git/sequencer &&
git cat-file commit HEAD >msg &&
-   grep "cherry picked from" msg
+   grep "Cherry-Pick:" msg
 '
 
 test_expect_success '--continue respects -x in first commit in multi-pick' '
@@ -416,7 +416,7 @@ 

Re: [RFC v1] Add virtual file system settings and hook proc

[Duy Nguyen]

On Wed, Oct 31, 2018 at 9:53 PM Ben Peart  wrote:
> >> +core.virtualFilesystem::
> >> +   If set, the value of this variable is used as a command which
> >> +   will identify all files and directories that are present in
> >> +   the working directory.  Git will only track and update files
> >> +   listed in the virtual file system.  Using the virtual file system
> >> +   will supersede the sparse-checkout settings which will be ignored.
> >> +   See the "virtual file system" section of linkgit:githooks[6].
> >
> > It sounds like "virtual file system" is just one of the use cases for
> > this feature, which is more about a dynamic source of sparse-checkout
> > bits. Perhaps name the config key with something along sparse checkout
> > instead of naming it after a use case.
>
> It's more than a dynamic sparse-checkout because the same list is also
> used to exclude any file/folder not listed.  That means any file not
> listed won't ever be updated by git (like in 'checkout' for example) so
> 'stale' files could be left in the working directory.  It also means git
> won't find new/untracked files unless they are specifically added to the
> list.

OK. I'm not at all interested in carrying maintenance burden for some
software behind closed doors. I could see values in having a more
flexible sparse checkout but this now seems like very tightly designed
for GVFS. So unless there's another use case (preferably open source)
for this, I don't think this should be added in git.git.
-- 
Duy

Re: [PATCH] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching

[Duy Nguyen]

On Sun, Nov 4, 2018 at 7:27 AM Eric Sunshine  wrote:
>
> On Sat, Nov 3, 2018 at 8:25 PM Eric Sunshine  wrote:
> > On Sat, Nov 3, 2018 at 11:31 AM Nguyễn Thái Ngọc Duy  
> > wrote:
> > > +test_expect_success 't_e_i() exclude case #8' '
> > > +   git init case8 &&
> > > +   (
> > > +   cd case8 &&
> > > +   echo file >file1 &&
> > > +   echo file >file2 &&
> > > +   git add . &&
> >
> > Won't this loose git-add invocation end up adding all the junk files
> > from earlier tests? One might have expected to see the more restricted
> > invocation:
> > git add file1 file2 &&
> > to make it easier to reason about the test and not worry about someone
> > later inserting tests above this one which might interfere with it.
>
> Upon reflection, there shouldn't be any junk files since this test is
> creating a new repository and "file1" and "file2" are the only files
> present. Apparently, I wasn't paying close enough attention when I
> made the earlier observation.

Yup.

> Anyhow, the more restricted git-add invocation you used in the re-roll
> is still preferable since it makes the intention obvious. Thanks.

Which is why I did it anyway :)
-- 
Duy

Re: [PATCH] tree-walk.c: fix overoptimistic inclusion in :(exclude) matching

[Eric Sunshine]

On Sat, Nov 3, 2018 at 8:25 PM Eric Sunshine  wrote:
> On Sat, Nov 3, 2018 at 11:31 AM Nguyễn Thái Ngọc Duy  
> wrote:
> > +test_expect_success 't_e_i() exclude case #8' '
> > +   git init case8 &&
> > +   (
> > +   cd case8 &&
> > +   echo file >file1 &&
> > +   echo file >file2 &&
> > +   git add . &&
>
> Won't this loose git-add invocation end up adding all the junk files
> from earlier tests? One might have expected to see the more restricted
> invocation:
> git add file1 file2 &&
> to make it easier to reason about the test and not worry about someone
> later inserting tests above this one which might interfere with it.

Upon reflection, there shouldn't be any junk files since this test is
creating a new repository and "file1" and "file2" are the only files
present. Apparently, I wasn't paying close enough attention when I
made the earlier observation.

Anyhow, the more restricted git-add invocation you used in the re-roll
is still preferable since it makes the intention obvious. Thanks.