Re: [RFC/PATCH] reset --hard: make use of the pretty machinery

[Jeff King]

On Thu, Feb 01, 2018 at 08:57:21PM +, Thomas Gummerer wrote:

> It is a slight change of the output if the second line of the commit
> message is not a blank line, i.e. if the commit message is
> 
> foo
> bar
> 
> previously we would print "HEAD is now at 00 foo", while after
> this change we print "HEAD is now at 00 foo bar", same as 'git log
> --oneline' shows "00 foo bar".
> 
> So this does make the output more consistent with other commands, and
> 'reset' is a porcelain command, so nobody should be parsing the output
> in scripts.

Yeah, I'd say it's definitely fine to change the (already translated!)
stderr output of git-reset. I agree that it's an improvement to be
consistent with other commands here.

> The current behaviour dates back to 0e5a7faa3a ("Make "git reset" a
> builtin.", 2007-09-11), so I assume (without digging into the old
> codebase too much) that the logic was implemented because there was
> no convenience function such as 'pp_commit_easy' that would do this
> already.

Yes, there used to be quite a lot of ad hoc parsing of commit objects,
but these days we have safer and more readable alternatives.  Even
without the visible behavior change, I think it would be worth doing
this patch just as a code cleanup.

> + struct strbuf buf = STRBUF_INIT;
> +
> + printf(_("HEAD is now at %s"),
> + find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
> +
> + pp_commit_easy(CMIT_FMT_ONELINE, commit, );
> + if (buf.len > 0)
> + printf(" %s", buf.buf);
> + putchar('\n');
> + strbuf_release();

I was disappointed you still had to call find_unique_abbrev(). It seems
like we ought to be able to do this in a single formatting call. But
CMIT_FMT_ONELINE is just about the subject, and doesn't include the hash
at all. There's no equivalent without turning to the user-format code.

You can do:

  struct pretty_print_context ctx = {0, DEFAULT_ABBREV};
  format_commit_message(commit, _("HEAD is now at %H %s"), , );
  puts(buf.buf);

but I was annoyed at having to say "DEFAULT_ABBREV". It seems like that
ought to be the default.

Anyway, I'm fine with your patch as-is. I just needed a little
formatting code-golf in my day.

-Peff

Re: how to ignore whitespace changes with --color-moved (git diff move detection)?

[Jeff King]

On Thu, Feb 01, 2018 at 06:13:52PM -0800, Timothee Cour wrote:

> this PR from october 2017 was discussing a patch that'd introduce
> `--color-moved-[no-]ignore-space-change`
> https://public-inbox.org/git/20171025224620.27657-3-sbel...@google.com/
> 
> however not sure what happened since then as I can't find in `git help
> diff` options even after `brew install --HEAD git`
> 
> it's a really useful feature as it's a common use case (ppl move
> blocks and reformat in same PR)

I think you can still do "--color-moved -w" to ignore whitespace. It's
just that the defaults did not end up getting flipped.

-Peff

[PATCH] rebase: add --allow-empty-message option

[Genki Sky]

--> Motivation

commit 4bee95847 ("cherry-pick: add --allow-empty-message option", 2012-08-02)
started doing this work, but it was never completed. For more discussion
on why this approach was chosen, see the thread beginning here:

  https://public-inbox.org/git/2012080658.ga21...@arachsys.com/

https://stackoverflow.com/q/8542304 also shows this as a desirable
feature, and that the workaround is non-trivial to get right.

--> Implementation

Add a new --allow-empty-message flag. Propagate it to all calls of 'git
commit', 'git cherry-pick', and 'git rebase--helper' within the rebase
scripts.

Signed-off-by: Genki Sky 
---
 Documentation/git-rebase.txt|  5 +++
 builtin/rebase--helper.c|  2 ++
 git-rebase--am.sh   |  1 +
 git-rebase--interactive.sh  | 25 +++--
 git-rebase--merge.sh|  3 +-
 git-rebase.sh   |  5 +++
 t/t3430-rebase-empty-message.sh | 79 +
 7 files changed, 109 insertions(+), 11 deletions(-)
 create mode 100755 t/t3430-rebase-empty-message.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8a861c1e0..d713951b8 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -244,6 +244,11 @@ leave out at most one of A and B, in which case it 
defaults to HEAD.
Keep the commits that do not change anything from its
parents in the result.

+--allow-empty-message::
+   By default, rebasing commits with an empty message will fail.
+   This option overrides that behavior, allowing commits with empty
+   messages to be rebased.
+
 --skip::
Restart the rebasing process by skipping the current patch.

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7daee544b..2090114e9 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -22,6 +22,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
commits")),
+   OPT_BOOL(0, "allow-empty-message", _empty_message,
+   N_("allow commits with empty messages")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 14c50782e..0f7805179 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -46,6 +46,7 @@ then
# makes this easy
git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
$allow_rerere_autoupdate --right-only "$revisions" \
+   $allow_empty_message \
${restrict_revision+^$restrict_revision}
ret=$?
 else
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d47bd2959..81c5b4287 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -281,7 +281,7 @@ pick_one () {

test -d "$rewritten" &&
pick_one_preserving_merges "$@" && return
-   output eval git cherry-pick $allow_rerere_autoupdate \
+   output eval git cherry-pick $allow_rerere_autoupdate 
$allow_empty_message \
${gpg_sign_opt:+$(git rev-parse --sq-quote 
"$gpg_sign_opt")} \
"$strategy_args" $empty_args $ff "$@"

@@ -406,6 +406,7 @@ pick_one_preserving_merges () {
;;
*)
output eval git cherry-pick $allow_rerere_autoupdate \
+   $allow_empty_message \
${gpg_sign_opt:+$(git rev-parse --sq-quote 
"$gpg_sign_opt")} \
"$strategy_args" "$@" ||
die_with_patch $sha1 "$(eval_gettext "Could not 
pick \$sha1")"
@@ -559,7 +560,8 @@ do_next () {

mark_action_done
do_pick $sha1 "$rest"
-   git commit --amend --no-post-rewrite 
${gpg_sign_opt:+"$gpg_sign_opt"} || {
+   git commit --amend --no-post-rewrite 
${gpg_sign_opt:+"$gpg_sign_opt"} \
+   $allow_empty_message || {
warn "$(eval_gettext "\
 Could not amend commit after successfully picking \$sha1... \$rest
 This is most likely due to an empty commit message, or the pre-commit hook
@@ -607,7 +609,7 @@ you are able to reword the commit.")"
# This is an intermediate commit; its message will only 
be
# used in case of trouble.  So use the long version:
do_with_author output git commit --amend --no-verify -F 
"$squash_msg" \
-   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
+   ${gpg_sign_opt:+"$gpg_sign_opt"} 
$allow_empty_message ||
   

Re: [PATCH v1 0/5] Incremental rewrite of git-submodules: git-foreach

[Prathamesh Chavan]

Since due to some reason, the previous patch-series list was
unavailable on the mailing list, I have re-posted the series.
It is available at:
https://public-inbox.org/git/20180202045745.5076-1-pc44...@gmail.com/

Thanks,
Prathamesh Chavan

[PATCH v1 5/5] submodule: port submodule subcommand 'foreach' from shell to C

[Prathamesh Chavan]

This aims to make git-submodule foreach a builtin. This is the very
first step taken in this direction. Hence, 'foreach' is ported to
submodule--helper, and submodule--helper is called from git-submodule.sh.
The code is split up to have one function to obtain all the list of
submodules. This function acts as the front-end of git-submodule foreach
subcommand. It calls the function for_each_listed_submodule(), which basically
loops through the list and calls function fn, which in this case is
runcommand_in_submodule_cb(). This third function is a callback function that
calls runcommand_in_submodule() with the appropriate parameters and then
takes care of running the command in that submodule, and recursively
performing the same when --recursive is flagged.

The first function module_foreach first parses the options present in
argv, and then with the help of module_list_compute(), generates the list of
submodules present in the current working tree.

The second function for_each_listed_submodule() traverses through the
list, and calls function fn (which in case of submodule subcommand
foreach is runcommand_in_submodule_cb()) is called for each entry.

The third function runcommand_in_submodule_cb() calls the function
runcommand_in_submodule() after passing appropraite parameters.

The fourth function runcommand_in_submodule(), generates a submodule struct sub
for $name, value and then later prepends name=sub->name; and other
value assignment to the env argv_array structure of a child_process.
Also the  of submodule-foreach is push to args argv_array
structure and finally, using run_command the commands are executed
using a shell.

The fourth function also takes care of the recursive flag, by creating
a separate child_process structure and prepending "--super-prefix displaypath",
to the args argv_array structure. Other required arguments and the
input  of submodule-foreach is also appended to this argv_array.

Helped-by: Brandon Williams 
Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 151 
 git-submodule.sh|  39 +---
 2 files changed, 152 insertions(+), 38 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a5c4a8a69..46dee6bf5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -718,6 +718,156 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct cb_foreach {
+   int argc;
+   const char **argv;
+   const char *prefix;
+   unsigned int flags;
+};
+#define CB_FOREACH_INIT { 0, NULL, NULL, 0 }
+
+static void runcommand_in_submodule(const char *path, const struct object_id 
*ce_oid,
+   int argc, const char **argv, const char 
*prefix,
+   unsigned int flags)
+{
+   const struct submodule *sub;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   char *displaypath;
+
+   displaypath = get_submodule_displaypath(path, prefix);
+
+   sub = submodule_from_path(_oid, path);
+
+   if (!sub)
+   die(_("No url found for submodule path '%s' in .gitmodules"),
+ displaypath);
+
+   if (!is_submodule_populated_gently(path, NULL))
+   goto cleanup;
+
+   prepare_submodule_repo_env(_array);
+
+   /*
+* For the purpose of executing  in the submodule,
+* separate shell is used for the purpose of running the
+* child process.
+*/
+   cp.use_shell = 1;
+   cp.dir = path;
+
+   /*
+* NEEDSWORK: the command currently has access to the variables $name,
+* $sm_path, $displaypath, $sha1 and $toplevel only when the command
+* contains a single argument. This is done for maintianing a faithful
+* translation from shell script.
+*/
+   if (argc == 1) {
+   char *toplevel = xgetcwd();
+
+   argv_array_pushf(_array, "name=%s", sub->name);
+   argv_array_pushf(_array, "sm_path=%s", path);
+   argv_array_pushf(_array, "displaypath=%s", displaypath);
+   argv_array_pushf(_array, "sha1=%s",
+oid_to_hex(ce_oid));
+   argv_array_pushf(_array, "toplevel=%s", toplevel);
+
+   /*
+* Since the path variable was accessible from the script
+* before porting, it is also made available after porting.
+* The environment variable "PATH" has a very special purpose
+* on windows. And since environment variables are
+* case-insensitive in windows, it interferes with the
+* existing PATH variable. Hence, to avoid that, we expose
+* path via the 

[PATCH v1 2/5] submodule foreach: document '$sm_path' instead of '$path'

[Prathamesh Chavan]

As using a variable '$path' may be harmful to users due to
capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't
use $path variable in eval_gettext string, 2012-04-17). Adjust
the documentation to advocate for using $sm_path,  which contains
the same value. We still make the 'path' variable available and
document it as a deprecated synonym of 'sm_path'.

Discussed-with: Ramsay Jones 
Signed-off-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 Documentation/git-submodule.txt | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index ff612001d..a23baef62 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -183,12 +183,14 @@ information too.
 
 foreach [--recursive] ::
Evaluates an arbitrary shell command in each checked out submodule.
-   The command has access to the variables $name, $path, $sha1 and
+   The command has access to the variables $name, $sm_path, $sha1 and
$toplevel:
$name is the name of the relevant submodule section in `.gitmodules`,
-   $path is the name of the submodule directory relative to the
-   superproject, $sha1 is the commit as recorded in the superproject,
-   and $toplevel is the absolute path to the top-level of the superproject.
+   $sm_path is the path of the submodule as recorded in the superproject,
+   $sha1 is the commit as recorded in the superproject, and
+   $toplevel is the absolute path to the top-level of the superproject.
+   Note that to avoid conflicts with '$PATH' on Windows, the '$path'
+   variable is now a deprecated synonym of '$sm_path' variable.
Any submodules defined in the superproject but not checked out are
ignored by this command. Unless given `--quiet`, foreach prints the name
of each submodule before evaluating the command.
-- 
2.15.1

[PATCH v1 4/5] submodule foreach: document variable '$displaypath'

[Prathamesh Chavan]

It was observed that the variable '$displaypath' was accessible but
undocumented. Hence, document it.

Discussed-with: Ramsay Jones 
Signed-off-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 Documentation/git-submodule.txt |  6 --
 t/t7407-submodule-foreach.sh| 22 +++---
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 8e7930ebc..0cca702cb 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -183,10 +183,12 @@ information too.
 
 foreach [--recursive] ::
Evaluates an arbitrary shell command in each checked out submodule.
-   The command has access to the variables $name, $sm_path, $sha1 and
-   $toplevel:
+   The command has access to the variables $name, $sm_path, $displaypath,
+   $sha1 and $toplevel:
$name is the name of the relevant submodule section in `.gitmodules`,
$sm_path is the path of the submodule as recorded in the superproject,
+   $displaypath contains the relative path from the current working
+   directory to the submodules root directory,
$sha1 is the commit as recorded in the superproject, and
$toplevel is the absolute path to its superproject, such that
$toplevel/$sm_path is the absolute path of the submodule.
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 0663622a4..6ad57e061 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -82,16 +82,16 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect <../../actual
+   git submodule foreach "echo 
\$toplevel-\$name-\$sm_path-\$displaypath-\$sha1" >../../actual
) &&
test_i18ncmp expect actual
 '
@@ -206,25 +206,25 @@ submodulesha1=$(cd 
clone2/nested1/nested2/nested3/submodule && git rev-parse HEA
 
 cat >expect <../../actual
+   git submodule foreach --recursive "echo 
\$toplevel-\$name-\$sm_path-\$displaypath-\$sha1" >../../actual
) &&
test_i18ncmp expect actual
 '
-- 
2.15.1

[PATCH v1 3/5] submodule foreach: clarify the '$toplevel' variable documentation

[Prathamesh Chavan]

It does not contain the topmost superproject as the author assumed,
but the direct superproject, such that $toplevel/$sm_path is the
actual absolute path of the submodule.

Discussed-with: Ramsay Jones 
Signed-off-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 Documentation/git-submodule.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index a23baef62..8e7930ebc 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -188,7 +188,8 @@ foreach [--recursive] ::
$name is the name of the relevant submodule section in `.gitmodules`,
$sm_path is the path of the submodule as recorded in the superproject,
$sha1 is the commit as recorded in the superproject, and
-   $toplevel is the absolute path to the top-level of the superproject.
+   $toplevel is the absolute path to its superproject, such that
+   $toplevel/$sm_path is the absolute path of the submodule.
Note that to avoid conflicts with '$PATH' on Windows, the '$path'
variable is now a deprecated synonym of '$sm_path' variable.
Any submodules defined in the superproject but not checked out are
-- 
2.15.1

[PATCH v1 0/5] Incremental rewrite of git-submodules

[Prathamesh Chavan]

Following series of patches focuses on porting submodule subcommand
git-foreach from shell to C.
An initial attempt for porting was introduced about 9 months back,
and since then then patches have undergone many changes. Some of the 
notable discussion thread which I would like to point out is: [1] 
The previous version of this patch series which was floated is
available at: [2].

The following changes were made to that:
* As it was observed in other submodule subcommand's ported function
  that the number of params increased a lot, the variables quiet and 
  recursive, were replaced in the cb_foreach struct with a single
  unsigned integer variable called flags.

* To accomodate the possiblity of a direct call to the functions
  runcommand_in_submodule(), callback function
  runcommand_in_submodule_cb() was introduced.

[1]: https://public-inbox.org/git/20170419170513.16475-1-pc44...@gmail.com/T/#u
[2]: https://public-inbox.org/git/20170807211900.15001-14-pc44...@gmail.com/

As before you can find this series at: 
https://github.com/pratham-pc/git/commits/patch-series-3

And its build report is available at: 
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-3
Build #202

Prathamesh Chavan (5):
  submodule foreach: correct '$path' in nested submodules from a
subdirectory
  submodule foreach: document '$sm_path' instead of '$path'
  submodule foreach: clarify the '$toplevel' variable documentation
  submodule foreach: document variable '$displaypath'
  submodule: port submodule subcommand 'foreach' from shell to C

 Documentation/git-submodule.txt |  15 ++--
 builtin/submodule--helper.c | 151 
 git-submodule.sh|  40 +--
 t/t7407-submodule-foreach.sh|  38 +-
 4 files changed, 197 insertions(+), 47 deletions(-)

-- 
2.15.1

[PATCH v1 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory

[Prathamesh Chavan]

When running 'git submodule foreach' from a subdirectory of your
repository, nested submodules get a bogus value for $sm_path:
For a submodule 'sub' that contains a nested submodule 'nested',
running 'git -C dir submodule foreach echo $path' would report
path='../nested' for the nested submodule. The first part '../' is
derived from the logic computing the relative path from $pwd to the
root of the superproject. The second part is the submodule path inside
the submodule. This value is of little use and is hard to document.

There are two different possible solutions that have more value:
(a) The path value is documented as the path from the toplevel of the
superproject to the mount point of the submodule.
In this case we would want to have path='sub/nested'.

(b) As Ramsay noticed the documented value is wrong. For the non-nested
case the path is equal to the relative path from $pwd to the
submodules working directory. When following this model,
the expected value would be path='../sub/nested'.

The behavior for (b) was introduced in 091a6eb0fe (submodule: drop the
top-level requirement, 2013-06-16) the intent for $path seemed to be
relative to $cwd to the submodule worktree, but that did not work for
nested submodules, as the intermittent submodules were not included in
the path.

If we were to fix the meaning of the $path using (a) such that "path"
is "the path from the toplevel of the superproject to the mount point
of the submodule", we would break any existing submodule user that runs
foreach from non-root of the superproject as the non-nested submodule
'../sub' would change its path to 'sub'.

If we would fix the meaning of the $path using (b), such that "path"
is "the relative path from $pwd to the submodule", then we would break
any user that uses nested submodules (even from the root directory) as
the 'nested' would become 'sub/nested'.

Both groups can be found in the wild.  The author has no data if one group
outweighs the other by large margin, and offending each one seems equally
bad at first.  However in the authors imagination it is better to go with
(a) as running from a sub directory sounds like it is carried out
by a human rather than by some automation task.  With a human on
the keyboard the feedback loop is short and the changed behavior can be
adapted to quickly unlike some automation that can break silently.

Discussed-with: Ramsay Jones 
Signed-off-by: Prathamesh Chavan 
Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  1 -
 t/t7407-submodule-foreach.sh | 36 ++--
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 156255a9e..7305ee25f 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -345,7 +345,6 @@ cmd_foreach()
prefix="$prefix$sm_path/"
sanitize_submodule_env
cd "$sm_path" &&
-   sm_path=$(git submodule--helper relative-path 
"$sm_path" "$wt_prefix") &&
# we make $path available to scripts ...
path=$sm_path &&
if test $# -eq 1
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..0663622a4 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -82,9 +82,9 @@ test_expect_success 'test basic "submodule foreach" usage' '
 
 cat >expect  expect <

Re: [PATCH v2 08/14] commit-graph: implement git-commit-graph --clear

[SZEDER Gábor]

> Teach Git to delete the current 'graph_head' file and the commit graph
> it references.

And will it leave other, non-important graph files behind?  Looking at
the code it indeed does.  What is the use case for keeping the
non-important graph files?

> This is a good safety valve if somehow the file is
> corrupted and needs to be recalculated. Since the commit graph is a
> summary of contents already in the ODB, it can be regenerated.

Wouldn't a simple 'git commit-graph --write --update-head' regenerate
it on it's own, without cleaning first?  It appears, after running a
few tests, that a corrupt graph file can be recreated without
cleaning, which is great.  However, if graph-head is corrupt, then the
command errors out with 'failed to read graph-head'.  I don't
understand the rationale behind this, it would be overwritten anyway,
and its content is not necessary for recreating the graph.  And
indeed, after commenting out that get_graph_head_hash() call in
cmd_commit_graph() it doesn't want to read my corrupted graph-head
file, and recreates both the graph and graph-head files just fine.

I think the requirement for explicitly cleaning a corrupt graph-head
before re-writing it is just unnecessary complication.

On second thought, what's the point of '--write' without
'--update-head', when consumers (thinking 'log --topo-order...) will
need the graph-head anyway?  I think '--write' should create a
graph-head without requiring an additional option.

Hmph, another second thought: the word 'head' has a rather specific
meaning in Git, although it's usually capitalized.  Using this word in
options and filenames may lead to confusion, especially the option
'--update-head'.


> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt | 16 ++--
>  builtin/commit-graph.c | 32 +++-
>  t/t5318-commit-graph.sh|  7 ++-
>  3 files changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 99ced16ddc..33d6567f11 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -11,6 +11,7 @@ SYNOPSIS
>  [verse]
>  'git commit-graph' --write  [--pack-dir ]
>  'git commit-graph' --read  [--pack-dir ]
> +'git commit-graph' --clear [--pack-dir ]
>  
>  OPTIONS
>  ---
> @@ -18,16 +19,21 @@ OPTIONS
>   Use given directory for the location of packfiles, graph-head,
>   and graph files.
>  
> +--clear::
> + Delete the graph-head file and the graph file it references.
> + (Cannot be combined with --read or --write.)
> +
>  --read::
>   Read a graph file given by the graph-head file and output basic
> - details about the graph file. (Cannot be combined with --write.)
> + details about the graph file. (Cannot be combined with --clear
> + or --write.)
>  
>  --graph-id::
>   When used with --read, consider the graph file graph-.graph.
>  
>  --write::
>   Write a new graph file to the pack directory. (Cannot be combined
> - with --read.)
> + with --clear or --read.)

All these "cannot be combined with --this and --that" remarks make
subcommands more and more appealing.

>  
>  --update-head::
>   When used with --write, update the graph-head file to point to
> @@ -61,6 +67,12 @@ $ git commit-graph --write --update-head
>  $ git commit-graph --read --graph-hash=
>  
>  
> +* Delete /graph-head and the file it references.
> ++
> +
> +$ git commit-graph --clear --pack-dir=
> +
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index d73cbc907d..4970dec133 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -10,6 +10,7 @@
>  
>  static char const * const builtin_commit_graph_usage[] = {
>   N_("git commit-graph [--pack-dir ]"),
> + N_("git commit-graph --clear [--pack-dir ]"),
>   N_("git commit-graph --read [--graph-hash=]"),
>   N_("git commit-graph --write [--pack-dir ] [--update-head]"),
>   NULL
> @@ -17,6 +18,7 @@ static char const * const builtin_commit_graph_usage[] = {
>  
>  static struct opts_commit_graph {
>   const char *pack_dir;
> + int clear;
>   int read;
>   const char *graph_hash;
>   int write;
> @@ -25,6 +27,30 @@ static struct opts_commit_graph {
>   struct object_id old_graph_hash;
>  } opts;
>  
> +static int graph_clear(void)
> +{
> + struct strbuf head_path = STRBUF_INIT;
> + char *old_path;
> +
> + if (!opts.has_existing)
> + return 0;
> +
> + strbuf_addstr(_path, opts.pack_dir);
> + strbuf_addstr(_path, "/");
> + strbuf_addstr(_path, "graph-head");

strbuf_addstr(_path, "/graph-head")

Although, considering that this is 

Re: [PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head

[SZEDER Gábor]

> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index da565624e3..d1a23bcdaf 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh


> @@ -107,6 +112,9 @@ test_expect_success 'setup bare repo' \
>  test_expect_success 'write graph in bare repo' \
>  'graphbare=$(git commit-graph --write) &&
>   test_path_is_file ${baredir}/graph-${graphbare}.graph &&
> + test_path_is_file ${baredir}/graph-head &&

This test and the one preceeding it are wrong.

Note that 'git commit-graph --write' above is missing the
'--update-head' option, so there should be no graph-head file written,
yet this 'this test_path_is_file' doesn't fail the test.

The devil lies in the previous test 'setup bare repo', where this bare
repo is created by cloning from a local remote: a simple 'git clone
--bare full bare' hardlinks all files under .git/objects, including
all graph and graph-head files that exist in the remote repo.

The previous test should run 'git clone --bare --no-local full bare'
instead, and then this test would fail because of the missing
graph-head file, as it should.  Specifying '--update-head' will make
it work again.


> + echo ${graphbare} >expect &&
> + cmp -n 40 expect ${baredir}/graph-head &&
>   git commit-graph --read --graph-hash=${graphbare} >output &&
>   _graph_read_expect "18" "${baredir}" &&
>   cmp expect output'

how to ignore whitespace changes with --color-moved (git diff move detection)?

[Timothee Cour]

this PR from october 2017 was discussing a patch that'd introduce
`--color-moved-[no-]ignore-space-change`
https://public-inbox.org/git/20171025224620.27657-3-sbel...@google.com/

however not sure what happened since then as I can't find in `git help
diff` options even after `brew install --HEAD git`

it's a really useful feature as it's a common use case (ppl move
blocks and reformat in same PR)

If it's not merged in git repo yet is there an easy way to try out
this feature? (even if experimental)

Re: [PATCH v2 11/14] commit: integrate commit graph with commit parsing

[Jonathan Tan]

On Tue, 30 Jan 2018 16:39:40 -0500
Derrick Stolee  wrote:

> +/* global storage */
> +struct commit_graph *commit_graph = 0;

NULL, not 0.

> +static int bsearch_graph(struct commit_graph *g, struct object_id *oid, 
> uint32_t *pos)
> +{
> + uint32_t last, first = 0;
> +
> + if (oid->hash[0])
> + first = ntohl(*(uint32_t*)(g->chunk_oid_fanout + 4 * 
> (oid->hash[0] - 1)));
> + last = ntohl(*(uint32_t*)(g->chunk_oid_fanout + 4 * oid->hash[0]));
> +
> + while (first < last) {
> + uint32_t mid = first + (last - first) / 2;
> + const unsigned char *current;
> + int cmp;
> +
> + current = g->chunk_oid_lookup + g->hdr->hash_len * mid;
> + cmp = hashcmp(oid->hash, current);
> + if (!cmp) {
> + *pos = mid;
> + return 1;
> + }
> + if (cmp > 0) {
> + first = mid + 1;
> + continue;
> + }
> + last = mid;
> + }
> +
> + *pos = first;
> + return 0;
> +}

This would be better in sha1-lookup.c, something like the reverse of commit
f1068efefe6d ("sha1_file: drop experimental GIT_USE_LOOKUP search",
2017-08-09), except that it can be done using a simple binary search.

> +static int full_parse_commit(struct commit *item, struct commit_graph *g,
> +  uint32_t pos, const unsigned char *commit_data)
> +{
> + struct object_id oid;
> + struct commit *new_parent;
> + uint32_t new_parent_pos;
> + uint32_t *parent_data_ptr;
> + uint64_t date_low, date_high;
> + struct commit_list **pptr;
> +
> + item->object.parsed = 1;
> + item->graph_pos = pos;
> +
> + hashcpy(oid.hash, commit_data);
> + item->tree = lookup_tree();
> +
> + date_high = ntohl(*(uint32_t*)(commit_data + g->hdr->hash_len + 8)) & 
> 0x3;
> + date_low = ntohl(*(uint32_t*)(commit_data + g->hdr->hash_len + 12));
> + item->date = (timestamp_t)((date_high << 32) | date_low);
> +
> + pptr = >parents;
> +
> + new_parent_pos = ntohl(*(uint32_t*)(commit_data + g->hdr->hash_len));
> + if (new_parent_pos == GRAPH_PARENT_NONE)
> + return 1;
> + get_nth_commit_oid(g, new_parent_pos, );
> + new_parent = lookup_commit();
> + if (new_parent) {
> + new_parent->graph_pos = new_parent_pos;
> + pptr = _list_insert(new_parent, pptr)->next;
> + } else {
> + die("could not find commit %s", oid_to_hex());
> + }
> +
> + new_parent_pos = ntohl(*(uint32_t*)(commit_data + g->hdr->hash_len + 
> 4));
> + if (new_parent_pos == GRAPH_PARENT_NONE)
> + return 1;
> + if (!(new_parent_pos & GRAPH_LARGE_EDGES_NEEDED)) {
> + get_nth_commit_oid(g, new_parent_pos, );
> + new_parent = lookup_commit();
> + if (new_parent) {
> + new_parent->graph_pos = new_parent_pos;
> + pptr = _list_insert(new_parent, pptr)->next;
> + } else
> + die("could not find commit %s", oid_to_hex());
> + return 1;
> + }
> +
> + parent_data_ptr = (uint32_t*)(g->chunk_large_edges + 4 * 
> (new_parent_pos ^ GRAPH_LARGE_EDGES_NEEDED));
> + do {
> + new_parent_pos = ntohl(*parent_data_ptr);
> +
> + get_nth_commit_oid(g, new_parent_pos & GRAPH_EDGE_LAST_MASK, 
> );
> + new_parent = lookup_commit();
> + if (new_parent) {
> + new_parent->graph_pos = new_parent_pos & 
> GRAPH_EDGE_LAST_MASK;
> + pptr = _list_insert(new_parent, pptr)->next;
> + } else
> + die("could not find commit %s", oid_to_hex());
> + parent_data_ptr++;
> + } while (!(new_parent_pos & GRAPH_LAST_EDGE));
> +
> + return 1;
> +}

The part that converts  into 
seems to be duplicated 3 times. Refactor into a function?

> +/**
> + * Fill 'item' to contain all information that would be parsed by 
> parse_commit_buffer.
> + */
> +static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, 
> uint32_t pos)
> +{
> + uint32_t new_parent_pos;
> + uint32_t *parent_data_ptr;
> + const unsigned char *commit_data = g->chunk_commit_data + 
> (g->hdr->hash_len + 16) * pos;
> +
> + new_parent_pos = ntohl(*(uint32_t*)(commit_data + g->hdr->hash_len));
> +
> + if (new_parent_pos == GRAPH_PARENT_MISSING)
> + return 0;
> +
> + if (new_parent_pos == GRAPH_PARENT_NONE)
> + return full_parse_commit(item, g, pos, commit_data);
> +
> + new_parent_pos = ntohl(*(uint32_t*)(commit_data + g->hdr->hash_len + 
> 4));
> +
> + if (new_parent_pos == GRAPH_PARENT_MISSING)
> + return 0;
> + if (!(new_parent_pos & GRAPH_LARGE_EDGES_NEEDED))
> + return full_parse_commit(item, g, pos, commit_data);
> +
> + 

Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

[SZEDER Gábor]

> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> new file mode 100755
> index 00..6bcd1cc264
> --- /dev/null
> +++ b/t/t5318-commit-graph.sh
> @@ -0,0 +1,96 @@
> +#!/bin/sh
> +
> +test_description='commit graph'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup full repo' \
> +'rm -rf .git &&
> + mkdir full &&
> + cd full &&
> + git init &&
> + git config core.commitgraph true &&

This config variable is unknown at this point.
I think the test shouldn't set it before it's introduced in patch 10.

> + git config pack.threads 1 &&
> + packdir=".git/objects/pack"'


> +test_expect_success 'setup bare repo' \
> +'cd .. &&
> + git clone --bare full bare &&
> + cd bare &&
> + git config core.graph true &&

Likewise, and its name should be updated as well.

> + git config pack.threads 1 &&
> + baredir="objects/pack"'

Re: [PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head

[SZEDER Gábor]

> It is possible to have multiple commit graph files in a pack directory,
> but only one is important at a time. Use a 'graph_head' file to point
> to the important file.

This implies that all those other files are ignored, right?

> Teach git-commit-graph to write 'graph_head' upon
> writing a new commit graph file.
> 
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt | 34 ++
>  builtin/commit-graph.c | 38 
> +++---
>  commit-graph.c | 25 +
>  commit-graph.h |  2 ++
>  t/t5318-commit-graph.sh| 12 ++--
>  5 files changed, 106 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 09aeaf6c82..99ced16ddc 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -12,15 +12,49 @@ SYNOPSIS
>  'git commit-graph' --write  [--pack-dir ]
>  'git commit-graph' --read  [--pack-dir ]
>  
> +OPTIONS
> +---

Oh, look, the 'OPTIONS' section I missed in the previous patches! ;)

This should be split up and squashed into the previous patches where
the individual --options are first mentioned.

> +--pack-dir::
> + Use given directory for the location of packfiles, graph-head,
> + and graph files.
> +
> +--read::
> + Read a graph file given by the graph-head file and output basic
> + details about the graph file. (Cannot be combined with --write.)

>From the output of 'git commit-graph --read' it seems that it's not a
generally useful option to the user.  Perhaps it should be mentioned
that it's only intended as a debugging aid?  Or maybe it doesn't
really matter, because eventually this command will become irrelevant,
as other commands (clone, fetch, gc) will invoke it automagically...

> +--graph-id::
> + When used with --read, consider the graph file graph-.graph.
> +
> +--write::
> + Write a new graph file to the pack directory. (Cannot be combined
> + with --read.)

I think this should also mention that it prints the generated graph
file's checksum.

> +
> +--update-head::
> + When used with --write, update the graph-head file to point to
> + the written graph file.

So it should be used with '--write', noted.

>  EXAMPLES
>  
>  
> +* Output the hash of the graph file pointed to by /graph-head.
> ++
> +
> +$ git commit-graph --pack-dir=
> +
> +
>  * Write a commit graph file for the packed commits in your local .git folder.
>  +
>  
>  $ git commit-graph --write
>  
>  
> +* Write a graph file for the packed commits in your local .git folder,
> +* and update graph-head.
> ++
> +
> +$ git commit-graph --write --update-head
> +
> +
>  * Read basic information from a graph file.
>  +
>  
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 218740b1f8..d73cbc907d 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -11,7 +11,7 @@
>  static char const * const builtin_commit_graph_usage[] = {
>   N_("git commit-graph [--pack-dir ]"),
>   N_("git commit-graph --read [--graph-hash=]"),
> - N_("git commit-graph --write [--pack-dir ]"),
> + N_("git commit-graph --write [--pack-dir ] [--update-head]"),
>   NULL
>  };
>  
> @@ -20,6 +20,9 @@ static struct opts_commit_graph {
>   int read;
>   const char *graph_hash;
>   int write;
> + int update_head;
> + int has_existing;
> + struct object_id old_graph_hash;
>  } opts;
>  
>  static int graph_read(void)
> @@ -30,8 +33,8 @@ static int graph_read(void)
>  
>   if (opts.graph_hash && strlen(opts.graph_hash) == GIT_MAX_HEXSZ)
>   get_oid_hex(opts.graph_hash, _hash);
> - else
> - die("no graph hash specified");
> + else if (!get_graph_head_hash(opts.pack_dir, _hash))
> + die("no graph-head exists");
>  
>   graph_file = get_commit_graph_filename_hash(opts.pack_dir, _hash);
>   graph = load_commit_graph_one(graph_file, opts.pack_dir);
> @@ -62,10 +65,33 @@ static int graph_read(void)
>   return 0;
>  }
>  
> +static void update_head_file(const char *pack_dir, const struct object_id 
> *graph_hash)
> +{
> + struct strbuf head_path = STRBUF_INIT;
> + int fd;
> + struct lock_file lk = LOCK_INIT;
> +
> + strbuf_addstr(_path, pack_dir);
> + strbuf_addstr(_path, "/");
> + strbuf_addstr(_path, "graph-head");

strbuf_addstr(_path, "/graph-head"); ?

> +
> + fd = hold_lock_file_for_update(, head_path.buf, LOCK_DIE_ON_ERROR);
> + 

Re: [PATCHv2] tag: add --edit option

[Eric Sunshine]

On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
 wrote:
> Add a --edit option whichs allows modifying the messages provided by -m or -F,
> the same way git commit --edit does.
>
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
> Changes since v1:
> - Fix usage string
> - Use write_script to generate editor
> - Rename editor to fakeeditor to match the other tests in the testsuite

Thanks for explaining what changed since the previous attempt. It is
also helpful for reviewers if you include a reference to the previous
iteration, like this:
https://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7...@suse.com/T/#u

Cc:'ing reviewers of previous iterations is also good etiquette when
submitting a new version.

> - I'll post another series to fix the misleading messages in both commit.c 
> and tag.c when launch_editor fails

Typically, it's easier on Junio, from a patch management standpoint,
if you submit all these related patches as a single series.
Alternately, if you do want to submit those changes separately, before
the current patch lands in "master", be sure to mention atop which
patch (this one) the additional patch(es) should live. Thanks.

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> @@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
> annotation lines.
> +-e::
> +--edit::
> +   The message taken from file with `-F` and command line with
> +   `-m` are usually used as the tag message unmodified.
> +   This option lets you further edit the message taken from these 
> sources.

You probably ought to add this new option to the command synopsis. In
the git-commit man page, the synopsis mentions only '-e' (not --edit),
so perhaps this man page could mirror that one. (Sorry for not
noticing this earlier.)

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -452,6 +452,21 @@ test_expect_success \
> +get_tag_header annotated-tag-edit $commit commit $time >expect
> +echo "An edited message" >>expect

Modern practice is to perform these "expect" setup actions (and all
other actions) within tests themselves rather than outside of tests.
However, consistency also has value, and since this test script is
filled with this sort of stylized "expect" setup already, this may be
fine, and probably not worth a re-roll. (A "modernization" patch by
someone can come later if warranted.)

> +test_expect_success 'set up editor' '
> +   write_script fakeeditor <<-\EOF
> +   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
> +   mv "$1-" "$1"
> +   EOF
> +'

Re: [PATCH v2 03/14] commit-graph: create git-commit-graph builtin

[SZEDER Gábor]

> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> new file mode 100644
> index 00..c8ea548dfb
> --- /dev/null
> +++ b/Documentation/git-commit-graph.txt
> @@ -0,0 +1,7 @@
> +git-commit-graph(1)
> +

Here the length of the '' must match the length of the title line
above, or AsciiDoc will complain about missing document title.

Re: 2 conflicts referencing the same path?

[Elijah Newren]

Hi,

On Thu, Feb 1, 2018 at 3:00 PM, Bryan Turner  wrote:
> While investigating an issue with rendering conflicts on a pull
> request, I noticed that the merge was producing this output (sanitized
> for paths)
>
> $ git merge --no-ff --log -m "Test" 190a25b6e0f32c7b8ccddf8c31e054149dece8b7
> CONFLICT (rename/add): Rename A->B in HEAD. B added in
> 190a25b6e0f32c7b8ccddf8c31e054149dece8b7
> Adding as B~190a25b6e0f32c7b8ccddf8c31e054149dece8b7 instead
> ...
> Auto-merging B
> CONFLICT (content): Merge conflicts in B
>
> (There are several other conflicts listed "between" the two I'm
> showing here, various rename/add, add/add and content conflicts, but
> I'm trimming those out to focus on the lines that I think are relevant
> to my question.)
>
> This merge produces 2 (or is it 3?) conflicts for the same B path:
> - Rename A to B in HEAD, add B in 190a25b
> - Content conflicts in B

Right, so the merge-base has just one (relevant) file, A.  For sake of
argument, let's say it's contents is "hello\nworld".

One side of history, leading to HEAD, also has one (relevant) file,
which was a rename of A->B which also changed its contents to say
"hello\nbeautiful\nworld"

The other side of history, leading to commit 190a25 has two files.
The original A, whose contents has changed to say
"hello\namazing\nworld", and a new file called B.

When you merge the two, the "hello world" file has been modified
differently on the two sides as well as having been renamed from A->B,
AND there was a separate file also placed at B on the other side of
history which gets in the way.  Git resolves the
two-files-getting-in-the-way-of-each-other (the rename/add) by moving
one of the two out of the way (though it really ought to move both out
of the way, but that's a tangent).  And it resolves the conflicting
content changes in the other B by doing a content merge with conflict
markers, giving a file with contents of the form:

"""
hello
<<
beautiful
==
amazing
>>
world
"""

and it treats that B (from the "rename") as more important than other
(the "add") which it shows by recording it at B.

> I'm still trying to produce a set of steps that will allow a minimal
> reproduction, but I thought I'd post to the list just to see if anyone
> had any thoughts on how it could happen. Is it a "normal" (albeit
> rare) case? Or could it represent some sort of issue in Git's 3-way
> merge algorithm (in its behavior or perhaps in how the merge conflicts
> are logged)?

It is "normal" to get this, and functioning as intended, albeit fairly
rare.  rename/rename(1to2) and rename/rename(2to1) conflicts could
provide very similar situations.

rename/add conflicts have three issues I know about[1], but that
didn't include the output messages being suboptimal.  That might just
mean my mind has been warped by reading merge-recursive.c or that I'm
too familiar with it, so I don't notice how much it could be improved.
If you can think of how to improve the messages, I'm happy to listen,
especially since I'm trying to find time to continue my rewrite of
merge-recursive.  It'd have to apply to other rename cases, as noted
above (and, in particular, each side of a rename/rename(1to2) can
further be involved in other collisions, such as rename/add or
rename/rename(2to1), so it could get hairy quick if the solution isn't
simple enough.)

[1] https://public-inbox.org/git/20171120221944.15431-6-new...@gmail.com/;
that patch and the others in the series are waiting for the directory
rename detection series to progress before I resubmit.

Elijah

Re: [PATCH v2 06/14] commit-graph: implement git-commit-graph --read

[Jonathan Tan]

On Tue, 30 Jan 2018 16:39:35 -0500
Derrick Stolee  wrote:

> Teach git-commit-graph to read commit graph files and summarize their 
> contents.

One overall question - is the "read" command meant to be a command used
by the end user, or is it here just to test that some aspects of reading
works? If the former, I'm not sure how useful it is. And if the latter,
I think that it is more useful to just implementing something that reads
it, then make the 11/14 change (modifying parse_commit_gently) and
include a perf test to show that your commit graph reading is both
correct and (performance-)effective.

Re: [PATCH v2 06/14] commit-graph: implement git-commit-graph --read

[SZEDER Gábor]

> Teach git-commit-graph to read commit graph files and summarize their 
> contents.
> 
> Use the --read option to verify the contents of a commit graph file in the
> tests.
> 
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt |   7 ++
>  builtin/commit-graph.c |  55 +++
>  commit-graph.c | 138 
> -
>  commit-graph.h |  25 +++
>  t/t5318-commit-graph.sh|  28 ++--
>  5 files changed, 247 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 3f3790d9a8..09aeaf6c82 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  
>  [verse]
>  'git commit-graph' --write  [--pack-dir ]
> +'git commit-graph' --read  [--pack-dir ]

Again, what does this option do?

>  EXAMPLES
>  
> @@ -20,6 +21,12 @@ EXAMPLES
>  $ git commit-graph --write
>  
>  
> +* Read basic information from a graph file.
> ++
> +
> +$ git commit-graph --read --graph-hash=
> +
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 7affd512f1..218740b1f8 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c

> +int close_commit_graph(struct commit_graph *g)

static, perhaps?  I see it's declared as extern in the headeer file
below, but I don't see it called outside of this source file by the
end of the patch series.

> +{
> + if (g->graph_fd < 0)
> + return 0;
> +
> + munmap((void *)g->data, g->data_len);
> + g->data = 0;
> +
> + close(g->graph_fd);
> + g->graph_fd = -1;
> +
> + return 1;
> +}
> +
> +static void free_commit_graph(struct commit_graph **g)
> +{
> + if (!g || !*g)
> + return;
> +
> + close_commit_graph(*g);
> +
> + free(*g);
> + *g = NULL;
> +}
> +
> +struct commit_graph *load_commit_graph_one(const char *graph_file, const 
> char *pack_dir)
> +{
> + void *graph_map;
> + const unsigned char *data;
> + struct commit_graph_header *hdr;
> + size_t graph_size;
> + struct stat st;
> + uint32_t i;
> + struct commit_graph *graph;
> + int fd = git_open(graph_file);
> + uint64_t last_chunk_offset;
> + uint32_t last_chunk_id;
> +
> + if (fd < 0)
> + return 0;
> + if (fstat(fd, )) {
> + close(fd);
> + return 0;
> + }
> + graph_size = xsize_t(st.st_size);
> +
> + if (graph_size < GRAPH_MIN_SIZE) {
> + close(fd);
> + die("graph file %s is too small", graph_file);
> + }
> + graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
> + data = (const unsigned char *)graph_map;
> +
> + hdr = graph_map;
> + if (ntohl(hdr->graph_signature) != GRAPH_SIGNATURE) {
> + uint32_t signature = ntohl(hdr->graph_signature);
> + munmap(graph_map, graph_size);
> + close(fd);
> + die("graph signature %X does not match signature %X",
> + signature, GRAPH_SIGNATURE);
> + }
> + if (hdr->graph_version != GRAPH_VERSION) {
> + unsigned char version = hdr->graph_version;
> + munmap(graph_map, graph_size);
> + close(fd);
> + die("graph version %X does not match version %X",
> + version, GRAPH_VERSION);
> + }
> +
> + graph = alloc_commit_graph(strlen(pack_dir) + 1);
> +
> + graph->hdr = hdr;
> + graph->graph_fd = fd;
> + graph->data = graph_map;
> + graph->data_len = graph_size;
> +
> + last_chunk_id = 0;
> + last_chunk_offset = (uint64_t)sizeof(*hdr);
> + for (i = 0; i < hdr->num_chunks; i++) {
> + uint32_t chunk_id = ntohl(*(uint32_t*)(data + sizeof(*hdr) + 12 
> * i));
> + uint64_t chunk_offset1 = ntohl(*(uint32_t*)(data + sizeof(*hdr) 
> + 12 * i + 4));
> + uint32_t chunk_offset2 = ntohl(*(uint32_t*)(data + sizeof(*hdr) 
> + 12 * i + 8));

There are a lot of magic number in these three lines, but at least
they are all multiples of 4.

> + uint64_t chunk_offset = (chunk_offset1 << 32) | chunk_offset2;
> +
> + if (chunk_offset > graph_size - GIT_MAX_RAWSZ)
> + die("improper chunk offset %08x%08x", 
> (uint32_t)(chunk_offset >> 32),
> + (uint32_t)chunk_offset);
> +
> + switch (chunk_id) {
> + case GRAPH_CHUNKID_OIDFANOUT:
> + graph->chunk_oid_fanout = data + chunk_offset;
> + break;
> +
> + case GRAPH_CHUNKID_OIDLOOKUP:
> +   

Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

[SZEDER Gábor]

> Teach git-commit-graph to write graph files. Create new test script to verify
> this command succeeds without failure.
> 
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt | 18 +++
>  builtin/commit-graph.c | 30 
>  t/t5318-commit-graph.sh| 96 
> ++
>  3 files changed, 144 insertions(+)
>  create mode 100755 t/t5318-commit-graph.sh
> 
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index c8ea548dfb..3f3790d9a8 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -5,3 +5,21 @@ NAME
>  
>  git-commit-graph - Write and verify Git commit graphs (.graph files)
>  
> +
> +SYNOPSIS
> +
> +[verse]
> +'git commit-graph' --write  [--pack-dir ]
> +

What do these options do and what is the command's output?  IOW, an
'OPTIONS' section would be nice.

> +EXAMPLES
> +
> +
> +* Write a commit graph file for the packed commits in your local .git folder.
> ++
> +
> +$ git commit-graph --write
> +
> +
> +GIT
> +---
> +Part of the linkgit:git[1] suite

> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> new file mode 100755
> index 00..6bcd1cc264
> --- /dev/null
> +++ b/t/t5318-commit-graph.sh
> @@ -0,0 +1,96 @@
> +#!/bin/sh
> +
> +test_description='commit graph'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup full repo' \
> +'rm -rf .git &&
> + mkdir full &&
> + cd full &&
> + git init &&
> + git config core.commitgraph true &&
> + git config pack.threads 1 &&

Does this pack.threads=1 make a difference?

> + packdir=".git/objects/pack"'

We tend to put single quotes around tests like this:

  test_expect_success 'setup full repo' '
do-this &&
check-that
  '

This is not a mere style nit: those newlines before and after the test
block make the test's output with '--verbose-log' slightly more
readable.

Furthermore, we prefer tabs for indentation.

Finally, 'cd'-ing around such that it affects subsequent tests is
usually frowned upon.  However, in this particular case (going into
one repo, doing a bunch of tests there, then going into another repo,
and doing another bunch of tests) I think it's better than changing
directory in a subshell in every single test.

> +
> +test_expect_success 'write graph with no packs' \
> +'git commit-graph --write --pack-dir .'
> +
> +test_expect_success 'create commits and repack' \
> +'for i in $(test_seq 5)
> + do
> +echo $i >$i.txt &&
> +git add $i.txt &&
> +git commit -m "commit $i" &&
> +git branch commits/$i
> + done &&
> + git repack'
> +
> +test_expect_success 'write graph' \
> +'graph1=$(git commit-graph --write) &&
> + test_path_is_file ${packdir}/graph-${graph1}.graph'

Style nit:  those {} around the variable names are unnecessary, but I
see you use them a lot.

> +
> +t_expect_success 'Add more commits' \

This must be test_expect_success.

> +'git reset --hard commits/3 &&
> + for i in $(test_seq 6 10)
> + do
> +echo $i >$i.txt &&
> +git add $i.txt &&
> +git commit -m "commit $i" &&
> +git branch commits/$i
> + done &&
> + git reset --hard commits/3 &&
> + for i in $(test_seq 11 15)
> + do
> +echo $i >$i.txt &&
> +git add $i.txt &&
> +git commit -m "commit $i" &&
> +git branch commits/$i
> + done &&
> + git reset --hard commits/7 &&
> + git merge commits/11 &&
> + git branch merge/1 &&
> + git reset --hard commits/8 &&
> + git merge commits/12 &&
> + git branch merge/2 &&
> + git reset --hard commits/5 &&
> + git merge commits/10 commits/15 &&
> + git branch merge/3 &&
> + git repack'
> +
> +# Current graph structure:
> +#
> +#  M3
> +# / |\_
> +#/ 10  15
> +#   /   |  |
> +#  /9 M2   14
> +# | |/  \  |
> +# | 8 M1 | 13
> +# | |/ | \_|
> +# 5 7  |   12
> +# | |   \__|
> +# 4 6  11
> +# |/__/
> +# 3
> +# |
> +# 2
> +# |
> +# 1
> +
> +test_expect_success 'write graph with merges' \
> +'graph2=$(git commit-graph --write) &&
> + test_path_is_file ${packdir}/graph-${graph2}.graph'
> +
> +test_expect_success 'setup bare repo' \
> +'cd .. &&
> + git clone --bare full bare &&
> + cd bare &&
> + git config core.graph true &&
> + git config pack.threads 1 &&
> + baredir="objects/pack"'
> +
> +test_expect_success 'write graph in bare repo' \
> +'graphbare=$(git commit-graph --write) &&
> + test_path_is_file ${baredir}/graph-${graphbare}.graph'
> +
> +test_done
> -- 
> 2.16.0.15.g9c3cf44.dirty

Re: [PATCH v2 04/14] commit-graph: implement construct_commit_graph()

[SZEDER Gábor]

> Teach Git to write a commit graph file by checking all packed objects
> to see if they are commits, then store the file in the given pack
> directory.
> 
> Signed-off-by: Derrick Stolee 
> ---
>  Makefile   |   1 +
>  commit-graph.c | 376 
> +
>  commit-graph.h |  20 +++
>  3 files changed, 397 insertions(+)
>  create mode 100644 commit-graph.c
>  create mode 100644 commit-graph.h


> diff --git a/commit-graph.c b/commit-graph.c
> new file mode 100644
> index 00..db2b7390c7
> --- /dev/null
> +++ b/commit-graph.c

> +struct packed_commit_list {
> + struct commit **list;
> + int num;
> + int size;
> +};
> +
> +struct packed_oid_list {
> + struct object_id **list;
> + int num;
> + int size;
> +};

When we manage the memory allocation of an array with the ALLOC_GROW
macro, then we tend to call the helper fields as 'alloc' and 'nr'.

> +static int if_packed_commit_add_to_list(const struct object_id *oid,
> + struct packed_git *pack,
> + uint32_t pos,
> + void *data)
> +{
> + struct packed_oid_list *list = (struct packed_oid_list*)data;
> + enum object_type type;
> + unsigned long size;
> + void *inner_data;
> + off_t offset = nth_packed_object_offset(pack, pos);
> + inner_data = unpack_entry(pack, offset, , );
> +
> + if (inner_data)
> + free(inner_data);
> +
> + if (type != OBJ_COMMIT)
> + return 0;
> +
> + ALLOC_GROW(list->list, list->num + 1, list->size);
> + list->list[list->num] = (struct object_id *)malloc(sizeof(struct 
> object_id));
> + oidcpy(list->list[list->num], oid);
> + (list->num)++;
> +
> + return 0;
> +}
> +
> +struct object_id *construct_commit_graph(const char *pack_dir)
> +{
> + struct packed_oid_list oids;
> + struct packed_commit_list commits;
> + struct commit_graph_header hdr;
> + struct sha1file *f;
> + int i, count_distinct = 0;
> + struct strbuf tmp_file = STRBUF_INIT;
> + unsigned char final_hash[GIT_MAX_RAWSZ];
> + char *graph_name;
> + int fd;
> + uint32_t chunk_ids[5];
> + uint64_t chunk_offsets[5];
> + int num_long_edges;
> + struct object_id *f_hash;
> + char *fname;
> + struct commit_list *parent;
> +
> + oids.num = 0;
> + oids.size = 1024;
> + ALLOC_ARRAY(oids.list, oids.size);
> + for_each_packed_object(if_packed_commit_add_to_list, , 0);
> + QSORT(oids.list, oids.num, commit_compare);
> +
> + count_distinct = 1;
> + for (i = 1; i < oids.num; i++) {
> + if (oidcmp(oids.list[i-1], oids.list[i]))
> + count_distinct++;
> + }
> +
> + commits.num = 0;
> + commits.size = count_distinct;
> + ALLOC_ARRAY(commits.list, commits.size);
> +
> + num_long_edges = 0;
> + for (i = 0; i < oids.num; i++) {
> + int num_parents = 0;
> + if (i > 0 && !oidcmp(oids.list[i-1], oids.list[i]))
> + continue;
> +
> + commits.list[commits.num] = lookup_commit(oids.list[i]);
> + parse_commit(commits.list[commits.num]);
> +
> + for (parent = commits.list[commits.num]->parents;
> +  parent; parent = parent->next)
> + num_parents++;
> +
> + if (num_parents > 2)
> + num_long_edges += num_parents - 1;
> +
> + commits.num++;
> + }
> +
> + strbuf_addstr(_file, pack_dir);
> + strbuf_addstr(_file, "/tmp_graph_XX");
> +
> + fd = git_mkstemp_mode(tmp_file.buf, 0444);
> + if (fd < 0)
> + die_errno("unable to create '%s'", tmp_file.buf);
> +
> + graph_name = strbuf_detach(_file, NULL);
> + f = sha1fd(fd, graph_name);
> +
> + hdr.graph_signature = htonl(GRAPH_SIGNATURE);
> + hdr.graph_version = GRAPH_VERSION;
> + hdr.hash_version = GRAPH_OID_VERSION;
> + hdr.hash_len = GRAPH_OID_LEN;
> + hdr.num_chunks = 4;
> +
> + assert(sizeof(hdr) == 8);
> + sha1write(f, , sizeof(hdr));
> +
> + chunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;
> + chunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;
> + chunk_ids[2] = GRAPH_CHUNKID_DATA;
> + chunk_ids[3] = GRAPH_CHUNKID_LARGEEDGES;
> + chunk_ids[4] = 0;
> +
> + chunk_offsets[0] = sizeof(hdr) + GRAPH_CHUNKLOOKUP_SIZE;
> + chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
> + chunk_offsets[2] = chunk_offsets[1] + GRAPH_OID_LEN * commits.num;
> + chunk_offsets[3] = chunk_offsets[2] + (GRAPH_OID_LEN + 16) * 
> commits.num;
> + chunk_offsets[4] = chunk_offsets[3] + 4 * num_long_edges;
> +
> + for (i = 0; i <= hdr.num_chunks; i++) {
> + uint32_t chunk_write[3];
> +
> + chunk_write[0] = htonl(chunk_ids[i]);
> + chunk_write[1] = htonl(chunk_offsets[i] >> 32);
> 

Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

[Jonathan Tan]

On Tue, 30 Jan 2018 16:39:34 -0500
Derrick Stolee  wrote:

> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index c8ea548dfb..3f3790d9a8 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -5,3 +5,21 @@ NAME
>  
>  git-commit-graph - Write and verify Git commit graphs (.graph files)
>  
> +
> +SYNOPSIS
> +
> +[verse]
> +'git commit-graph' --write  [--pack-dir ]

Subcommands (like those in git submodule) generally don't take "--", as
far as I know.

> +static int graph_write(void)
> +{
> + struct object_id *graph_hash = construct_commit_graph(opts.pack_dir);

I should have noticed this when I was reviewing the previous patch, but
this is probably better named write_commit_graph.

> +test_expect_success 'create commits and repack' \
> +'for i in $(test_seq 5)
> + do
> +echo $i >$i.txt &&
> +git add $i.txt &&
> +git commit -m "commit $i" &&

You can generate commits more easily with test_commit. Also, the final
character of the test_expect_success line should be the apostrophe that
starts the big text block, like in other files. (That also means that
the backslash is unnecessary.)

> +# Current graph structure:
> +#
> +#  M3
> +# / |\_
> +#/ 10  15
> +#   /   |  |
> +#  /9 M2   14
> +# | |/  \  |
> +# | 8 M1 | 13
> +# | |/ | \_|
> +# 5 7  |   12
> +# | |   \__|
> +# 4 6  11
> +# |/__/
> +# 3
> +# |
> +# 2
> +# |
> +# 1

I don't think we need such a complicated graph structure - maybe it's
sufficient to have one 2-way merge and one 3-way merge. E.g.

6
|\-.
5 \ \
|\ \ \
1 2 3 4

Also, I wonder if it is possible to test the output to a greater extent.
We don't want anything that relies on the ordering of commits
(especially since we plan on transitioning away from using SHA-1 as the
hash function) but we could still test, for example, that a 3-way merge
results in an edge list of the form "EDGE_..._..." (where the first _
does not have its high bit set, but the second one does). This could be
done by using my hex_unpack() function as seen here [1] with grep (e.g.
"45444745[0-7]...[8-9a-f]...").

[1] 
https://public-inbox.org/git/b9ea93edabc42754dc3643d6307c22a947eabaf3.1506714999.git.jonathanta...@google.com/

Re: Git on Windows maps creation time onto changed time

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 2, 2018 at 12:10 AM, Keith Goldfarb
 wrote:
> Dear git,
>
> While tracking down a problem with a filesystem shared by Windows and Ubuntu, 
> I came across the following code in compat/mingw.c (ming_fstat(), also in 
> do_lstat()):
>
> if (GetFileInformationByHandle(fh, )) {
> buf->st_ino = 0;
> buf->st_gid = 0;
> buf->st_uid = 0;
> buf->st_nlink = 1;
> buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
> buf->st_size = fdata.nFileSizeLow |
> (((off_t)fdata.nFileSizeHigh)<<32);
> buf->st_dev = buf->st_rdev = 0; /* not used by Git */
> buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
> buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
> buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
> return 0;
> }
>
> The assignment of buf->st_ctime doesn’t seem right to me. I understand 
> there’s no good choice here, but I think a better choice would be to 
> duplicate the definition used for st_mtime.
>
> Background: When I do a git status on Windows and then later on Ubuntu (or 
> the other order), it is extremely slow, as the entire tree is being 
> traversed. I tracked it down to this difference in definition of c_time. Yes, 
> I know about the core.trustctime variable, but my problem aside this seems 
> like an unwise choice.
>
> Thanks for listening,

Let's CC Marius Storm-Olsen who added this behavior (CC'd).

Git on Windows maps creation time onto changed time

[Keith Goldfarb]

Dear git,

While tracking down a problem with a filesystem shared by Windows and Ubuntu, I 
came across the following code in compat/mingw.c (ming_fstat(), also in 
do_lstat()):

if (GetFileInformationByHandle(fh, )) {
buf->st_ino = 0;
buf->st_gid = 0;
buf->st_uid = 0;
buf->st_nlink = 1;
buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
buf->st_size = fdata.nFileSizeLow |
(((off_t)fdata.nFileSizeHigh)<<32);
buf->st_dev = buf->st_rdev = 0; /* not used by Git */
buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
return 0;
}

The assignment of buf->st_ctime doesn’t seem right to me. I understand there’s 
no good choice here, but I think a better choice would be to duplicate the 
definition used for st_mtime.

Background: When I do a git status on Windows and then later on Ubuntu (or the 
other order), it is extremely slow, as the entire tree is being traversed. I 
tracked it down to this difference in definition of c_time. Yes, I know about 
the core.trustctime variable, but my problem aside this seems like an unwise 
choice.

Thanks for listening,

K.

2 conflicts referencing the same path?

[Bryan Turner]

While investigating an issue with rendering conflicts on a pull
request, I noticed that the merge was producing this output (sanitized
for paths)

$ git merge --no-ff --log -m "Test" 190a25b6e0f32c7b8ccddf8c31e054149dece8b7
CONFLICT (rename/add): Rename A->B in HEAD. B added in
190a25b6e0f32c7b8ccddf8c31e054149dece8b7
Adding as B~190a25b6e0f32c7b8ccddf8c31e054149dece8b7 instead
...
Auto-merging B
CONFLICT (content): Merge conflicts in B

(There are several other conflicts listed "between" the two I'm
showing here, various rename/add, add/add and content conflicts, but
I'm trimming those out to focus on the lines that I think are relevant
to my question.)

This merge produces 2 (or is it 3?) conflicts for the same B path:
- Rename A to B in HEAD, add B in 190a25b
- Content conflicts in B

The version of B left in place _does_ contain conflict markers, after
the merge processes completes. The version added as B~190a25b has no
conflict markers; it just shows a small number of inserted lines.

The merge in question produces 23 different CONFLICT lines, but aside
from "composite" conflicts (rename/add, add/add) that can include the
same path multiple times in their output, only this one path is
mentioned in multiple CONFLICT lines.

I'm still trying to produce a set of steps that will allow a minimal
reproduction, but I thought I'd post to the list just to see if anyone
had any thoughts on how it could happen. Is it a "normal" (albeit
rare) case? Or could it represent some sort of issue in Git's 3-way
merge algorithm (in its behavior or perhaps in how the merge conflicts
are logged)?

Any insights appreciated!
Bryan Turner

Re: [PATCH 5/5] dir.c: stop ignoring opendir() error in open_cached_dir()

[Ævar Arnfjörð Bjarmason]

On Wed, Jan 24 2018, Nguyễn Thái Ngọc Duy jotted:

> A follow-up to the recently fixed bugs in the untracked
> invalidation. If opendir() fails it should show a warning, perhaps
> this should die, but if this ever happens the error is probably
> recoverable for the user, and dying would just make things worse.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  dir.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/dir.c b/dir.c
> index 44b4dd2ec8..55736d3e2a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1787,11 +1787,16 @@ static int open_cached_dir(struct cached_dir *cdir,
>  struct strbuf *path,
>  int check_only)
>  {
> + const char *c_path;
> +
>   memset(cdir, 0, sizeof(*cdir));
>   cdir->untracked = untracked;
>   if (valid_cached_dir(dir, untracked, istate, path, check_only))
>   return 0;
> - cdir->fdir = opendir(path->len ? path->buf : ".");
> + c_path = path->len ? path->buf : ".";
> + cdir->fdir = opendir(c_path);
> + if (!cdir->fdir)
> + warning_errno(_("could not open directory '%s'"), c_path);
>   if (dir->untracked) {
>   invalidate_directory(dir->untracked, untracked);
>   dir->untracked->dir_opened++;

I haven't found the root cause yet, but we should not release a 2.17
with this patch.

I tried deploying a 2.16.1 + various patches (including this) internally
today, and on a test set of 236 machines with existing checkouts
(already using untracked cache) 79 would spew "warning: could not open
directory" warnings, warning about a lot of directories that did not
exist at the currently checked-out commit.

The warnings go away on a one-off:

git -c core.untrackedCache=false status

So there's some issue where an existing index with stale UC will be used
by a newer version of git, and will start very verbosely warning about
every directory referenced that can't be found anymore.

Re: [PATCH v2 04/14] commit-graph: implement construct_commit_graph()

[Jonathan Tan]

On Tue, 30 Jan 2018 16:39:33 -0500
Derrick Stolee  wrote:

> +#define GRAPH_SIGNATURE 0x43475048 /* "CGPH" */
> +#define GRAPH_CHUNKID_OIDFANOUT 0x4f494446 /* "OIDF" */
> +#define GRAPH_CHUNKID_OIDLOOKUP 0x4f49444c /* "OIDL" */
> +#define GRAPH_CHUNKID_DATA 0x43444154 /* "CDAT" */
> +#define GRAPH_CHUNKID_LARGEEDGES 0x45444745 /* "EDGE" */

Could all these just be string constants? sha1write can handle them well
enough.

> +static void write_graph_chunk_fanout(struct sha1file *f,
> +  struct commit **commits,
> +  int nr_commits)
> +{
> + uint32_t i, count = 0;
> + struct commit **list = commits;
> + struct commit **last = commits + nr_commits;
> +
> + /*
> +  * Write the first-level table (the list is sorted,
> +  * but we use a 256-entry lookup to be able to avoid
> +  * having to do eight extra binary search iterations).
> +  */
> + for (i = 0; i < 256; i++) {
> + uint32_t swap_count;
> +
> + while (list < last) {
> + if ((*list)->object.oid.hash[0] != i)
> + break;
> + count++;
> + list++;
> + }
> +
> + swap_count = htonl(count);
> + sha1write(f, _count, 4);

You can use sha1write_be32() instead of swapping.

> +static void write_graph_chunk_large_edges(struct sha1file *f,
> +   struct commit **commits,
> +   int nr_commits)
> +{
> + struct commit **list = commits;
> + struct commit **last = commits + nr_commits;
> + struct commit_list *parent;
> +
> + while (list < last) {
> + int num_parents = 0;
> + for (parent = (*list)->parents; num_parents < 3 && parent;
> +  parent = parent->next)
> + num_parents++;
> +
> + if (num_parents <= 2) {
> + list++;
> + continue;
> + }
> +
> + for (parent = (*list)->parents; parent; parent = parent->next) {
> + uint32_t int_id, swap_int_id;
> + uint32_t last_edge = 0;
> +
> + if (parent == (*list)->parents)
> + continue;

Probably better to just initialize "parent = (*list)->parents->next".
Also probably best to add a comment describing why you are doing this
(e.g. "The first parent is already in the main commit table; the large
edges table only contains the second parent onwards").

> +struct packed_commit_list {
> + struct commit **list;
> + int num;
> + int size;
> +};
> +
> +struct packed_oid_list {
> + struct object_id **list;
> + int num;
> + int size;
> +};

What are num and size? If they're nr and alloc, maybe use those names
instead.

> +static int if_packed_commit_add_to_list(const struct object_id *oid,
> + struct packed_git *pack,
> + uint32_t pos,
> + void *data)
> +{
> + struct packed_oid_list *list = (struct packed_oid_list*)data;
> + enum object_type type;
> + unsigned long size;
> + void *inner_data;
> + off_t offset = nth_packed_object_offset(pack, pos);
> + inner_data = unpack_entry(pack, offset, , );
> +
> + if (inner_data)
> + free(inner_data);
> +
> + if (type != OBJ_COMMIT)
> + return 0;
> +
> + ALLOC_GROW(list->list, list->num + 1, list->size);
> + list->list[list->num] = (struct object_id *)malloc(sizeof(struct 
> object_id));

No need to cast return value of malloc. Also, use xmalloc?

> +struct object_id *construct_commit_graph(const char *pack_dir)
> +{
> + struct packed_oid_list oids;
> + struct packed_commit_list commits;
> + struct commit_graph_header hdr;
> + struct sha1file *f;
> + int i, count_distinct = 0;
> + struct strbuf tmp_file = STRBUF_INIT;
> + unsigned char final_hash[GIT_MAX_RAWSZ];
> + char *graph_name;
> + int fd;
> + uint32_t chunk_ids[5];
> + uint64_t chunk_offsets[5];
> + int num_long_edges;
> + struct object_id *f_hash;
> + char *fname;
> + struct commit_list *parent;
> +
> + oids.num = 0;
> + oids.size = 1024;
> + ALLOC_ARRAY(oids.list, oids.size);
> + for_each_packed_object(if_packed_commit_add_to_list, , 0);
> + QSORT(oids.list, oids.num, commit_compare);
> +
> + count_distinct = 1;
> + for (i = 1; i < oids.num; i++) {
> + if (oidcmp(oids.list[i-1], oids.list[i]))
> + count_distinct++;
> + }
> +
> + commits.num = 0;
> + commits.size = count_distinct;
> + ALLOC_ARRAY(commits.list, commits.size);
> +
> + num_long_edges = 0;
> + for (i = 0; i < oids.num; i++) {
> + int num_parents = 0;
> + if (i 

Re: [PATCH v2 00/12] object_id part 11 (the_hash_algo)

[Stefan Beller]

On Wed, Jan 31, 2018 at 6:18 PM, brian m. carlson
 wrote:
> This series includes various changes to adopt the use of the_hash_algo
> for abstracting hash algorithms away.

This series looks good to me.

>
> Changes from v1:
> * Fix comments referring to SHA-1.
> * Switch hash function wrappers to take git_hash_ctx *.
> * Use a const int variable for a constant value.
> * Wrap an overly long line.
>
> brian m. carlson (12):
>   hash: move SHA-1 macros to hash.h
>   hash: create union for hash context allocation
>   builtin/index-pack: improve hash function abstraction
>   builtin/unpack-objects: switch uses of SHA-1 to the_hash_algo
>   sha1_file: switch uses of SHA-1 to the_hash_algo
>   fast-import: switch various uses of SHA-1 to the_hash_algo
>   pack-check: convert various uses of SHA-1 to abstract forms
>   pack-write: switch various SHA-1 values to abstract forms
>   read-cache: abstract away uses of SHA-1
>   csum-file: rename sha1file to hashfile
>   csum-file: abstract uses of SHA-1
>   bulk-checkin: abstract SHA-1 usage
>
>  builtin/index-pack.c | 108 
> +++
>  builtin/pack-objects.c   |  52 +++
>  builtin/unpack-objects.c |  18 
>  bulk-checkin.c   |  28 ++--
>  cache.h  |  25 ---
>  csum-file.c  |  46 ++--
>  csum-file.h  |  38 -
>  fast-import.c|  70 +++---
>  hash.h   |  40 +++---
>  pack-bitmap-write.c  |  30 ++---
>  pack-check.c |  32 +++---
>  pack-write.c |  77 -
>  pack.h   |   4 +-
>  read-cache.c |  58 -
>  sha1_file.c  |  72 +++
>  15 files changed, 351 insertions(+), 347 deletions(-)
>

Re: [PATCH v2 01/14] commit-graph: add format document

[Jonathan Tan]

On Tue, 30 Jan 2018 16:39:30 -0500
Derrick Stolee  wrote:

> +  Commit Data (ID: {'C', 'G', 'E', 'T' }) (N * (H + 16) bytes)
> +* The first H bytes are for the OID of the root tree.
> +* The next 8 bytes are for the int-ids of the first two parents
> +  of the ith commit. Stores value 0x if no parent in that
> +  position. If there are more than two parents, the second value
> +  has its most-significant bit on and the other bits store an array
> +  position into the Large Edge List chunk.

[snip]

> +  Large Edge List (ID: {'E', 'D', 'G', 'E'})
> +  This list of 4-byte values store the second through nth parents for
> +  all octopus merges. The second parent value in the commit data is a
> +  negative number pointing into this list. 

Looking at the paragraph which I quoted before the [snip], this sentence
is confusing (in particular, the second parent value is not interpreted
as the normal two's-complement negative value, and the semantics of
negative values indexing into the list is unclear). Maybe it's better to
omit it entirely.

[RFC/PATCH] reset --hard: make use of the pretty machinery

[Thomas Gummerer]

reset --hard currently uses its own logic for printing the first line of
the commit message in its output.  Instead of just using the first line,
use the pretty machinery to create the output.

In addition to the easier to follow code, this makes the output more
consistent with other commands that print the title of the commit, such
as 'git commit --oneline' or 'git checkout', which both use
'pp_commit_easy()' with the CMIT_FMT_ONELINE modifier.

It is a slight change of the output if the second line of the commit
message is not a blank line, i.e. if the commit message is

foo
bar

previously we would print "HEAD is now at 00 foo", while after
this change we print "HEAD is now at 00 foo bar", same as 'git log
--oneline' shows "00 foo bar".

So this does make the output more consistent with other commands, and
'reset' is a porcelain command, so nobody should be parsing the output
in scripts.

The current behaviour dates back to 0e5a7faa3a ("Make "git reset" a
builtin.", 2007-09-11), so I assume (without digging into the old
codebase too much) that the logic was implemented because there was
no convenience function such as 'pp_commit_easy' that would do this
already.

Signed-off-by: Thomas Gummerer 
---

Sending this as RFC/PATCH, as I'm not 100% sure this change in
behaviour is acceptable, and that I'm not missing some other edge
case, but I noticed this while trying to find out how this message
gets constructed, and just using the pretty machinery seems much
simpler, and more consisent

 builtin/reset.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index e15f595799..5da0f75de9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -106,24 +106,16 @@ static int reset_index(const struct object_id *oid, int 
reset_type, int quiet)
 
 static void print_new_head_line(struct commit *commit)
 {
-   const char *hex, *body;
-   const char *msg;
-
-   hex = find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
-   printf(_("HEAD is now at %s"), hex);
-   msg = logmsg_reencode(commit, NULL, get_log_output_encoding());
-   body = strstr(msg, "\n\n");
-   if (body) {
-   const char *eol;
-   size_t len;
-   body = skip_blank_lines(body + 2);
-   eol = strchr(body, '\n');
-   len = eol ? eol - body : strlen(body);
-   printf(" %.*s\n", (int) len, body);
-   }
-   else
-   printf("\n");
-   unuse_commit_buffer(commit, msg);
+   struct strbuf buf = STRBUF_INIT;
+
+   printf(_("HEAD is now at %s"),
+   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
+
+   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
+   if (buf.len > 0)
+   printf(" %s", buf.buf);
+   putchar('\n');
+   strbuf_release();
 }
 
 static void update_index_from_diff(struct diff_queue_struct *q,
-- 
2.16.1.101.gde0f0111ea

Re: [PATCH 11/26] serve: introduce git-serve

[Stefan Beller]

On Thu, Feb 1, 2018 at 12:37 PM, Randall S. Becker
 wrote:

>> I think that it would be too much of a change to up to 1MB lines at the
>> moment so I'm planning on leaving it right where it is :)
>
> In for a kilo, in for a tonne. Once we're way up there, it's not a problem
> or much of a difference. :)

What benefit does a larger buffer have?

I outlined the negatives above (large static buffer, issues with
progress meter).

And it seems to me that Brandon wants to keep this series as small as possible
w.r.t. bait for endless discussions and only deliver innovation, that solves the
immediate needs. Are there issues with too small buffers? (Can you link to
the performance measurements or an analysis?)

Good Morning !

[Anny]

Good Morning , 



I have a project i would like to bring to you and i want you to help me discuss 
it. please let me know if you will be available for this project. 



Thanks 

RE: [PATCH 11/26] serve: introduce git-serve

[Randall S. Becker]

On February 1, 2018 3:08 PM, Brandon Williams wrote:
> On 02/01, Randall S. Becker wrote:
> > On February 1, 2018 1:58 PM, Stefan Beller wrote:
> > > On Thu, Feb 1, 2018 at 10:48 AM, Jeff Hostetler
> > > 
> > > wrote:
> > > >
> > > >
> > > > On 1/2/2018 7:18 PM, Brandon Williams wrote:
> > > >>
> > > >> Introduce git-serve, the base server for protocol version 2.
> > > >>
> > > >> Protocol version 2 is intended to be a replacement for Git's
> > > >> current wire protocol.  The intention is that it will be a
> > > >> simpler, less wasteful protocol which can evolve over time.
> > > >>
> > > >> Protocol version 2 improves upon version 1 by eliminating the
> > > >> initial ref advertisement.  In its place a server will export a
> > > >> list of capabilities and commands which it supports in a
> > > >> capability advertisement.  A client can then request that a
> > > >> particular command be executed by providing a number of
> > > >> capabilities and command specific parameters.  At the completion
> > > >> of a command, a client can request that another command be
> > > >> executed or can terminate the connection by sending a flush packet.
> > > >>
> > > >> Signed-off-by: Brandon Williams 
> > > >> ---
> > > >>   .gitignore  |   1 +
> > > >>   Documentation/technical/protocol-v2.txt |  91 
> > > >>   Makefile|   2 +
> > > >>   builtin.h   |   1 +
> > > >>   builtin/serve.c |  30 
> > > >>   git.c   |   1 +
> > > >>   serve.c | 239
> > > >> 
> > > >>   serve.h |  15 ++
> > > >>   8 files changed, 380 insertions(+)
> > > >>   create mode 100644 Documentation/technical/protocol-v2.txt
> > > >>   create mode 100644 builtin/serve.c
> > > >>   create mode 100644 serve.c
> > > >>   create mode 100644 serve.h
> > > >>
> > > >> diff --git a/.gitignore b/.gitignore index 833ef3b0b..2d0450c26
> > > >> 100644
> > > >> --- a/.gitignore
> > > >> +++ b/.gitignore
> > > >> @@ -140,6 +140,7 @@
> > > >>   /git-rm
> > > >>   /git-send-email
> > > >>   /git-send-pack
> > > >> +/git-serve
> > > >>   /git-sh-i18n
> > > >>   /git-sh-i18n--envsubst
> > > >>   /git-sh-setup
> > > >> diff --git a/Documentation/technical/protocol-v2.txt
> > > >> b/Documentation/technical/protocol-v2.txt
> > > >> new file mode 100644
> > > >> index 0..b87ba3816
> > > >> --- /dev/null
> > > >> +++ b/Documentation/technical/protocol-v2.txt
> > > >> @@ -0,0 +1,91 @@
> > > >> + Git Wire Protocol, Version 2
> > > >> +==
> > > >> +
> > > >> +This document presents a specification for a version 2 of Git's
> > > >> +wire protocol.  Protocol v2 will improve upon v1 in the following
> ways:
> > > >> +
> > > >> +  * Instead of multiple service names, multiple commands will be
> > > >> +supported by a single service.
> > > >> +  * Easily extendable as capabilities are moved into their own
section
> > > >> +of the protocol, no longer being hidden behind a NUL byte and
> > > >> +limited by the size of a pkt-line (as there will be a single
> > > >> +capability per pkt-line).
> > > >> +  * Separate out other information hidden behind NUL bytes (e.g.
> agent
> > > >> +string as a capability and symrefs can be requested using
> > > >> + 'ls-refs')
> > > >> +  * Reference advertisement will be omitted unless explicitly
> > > >> + requested
> > > >> +  * ls-refs command to explicitly request some refs
> > > >> +
> > > >> + Detailed Design
> > > >> +=
> > > >> +
> > > >> +A client can request to speak protocol v2 by sending `version=2`
> > > >> +in the side-channel `GIT_PROTOCOL` in the initial request to the
> server.
> > > >> +
> > > >> +In protocol v2 communication is command oriented.  When first
> > > >> +contacting
> > > >> a
> > > >> +server a list of capabilities will advertised.  Some of these
> > > >> capabilities
> > > >> +will be commands which a client can request be executed.  Once a
> > > >> +command has completed, a client can reuse the connection and
> > > >> +request that other commands be executed.
> > > >> +
> > > >> + Special Packets
> > > >> +-
> > > >> +
> > > >> +In protocol v2 these special packets will have the following
> semantics:
> > > >> +
> > > >> +  * '' Flush Packet (flush-pkt) - indicates the end of a
> > > >> + message
> > > >> +  * '0001' Delimiter Packet (delim-pkt) - separates sections of
> > > >> + a message
> > > >
> > > >
> > > > Previously, a 0001 pkt-line meant that there was 1 byte of data
> > > > following, right?
> > >
> > > No, the length was including the length field, so 0005 would
> > > indicate that there is one byte following, (+4 bytes of "0005"
> > > included)
> > >
> > > > Does this change that and/or prevent 1 byte packets?  (Not sure 

Re: [PATCHv3 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Stefan Beller]

On Thu, Feb 1, 2018 at 11:29 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Thu, Jan 04 2018, Stefan Beller jotted:
>
>> Stefan Beller (4):
>>   color.h: document and modernize header
>>   builtin/blame: dim uninteresting metadata
>>   builtin/blame: add option to color metadata fields separately
>>   builtin/blame: highlight recently changed lines
>
> I like this feature in principle, but I can't get it to work. Building
> pu and:

Thanks for testing the feature!
Please use the flag `--color-lines`, `--color-fields` or `--heated-lines`
for experimentation.

In the future we may decide that one of them becomes the default
(which one?) and is triggered by the color.ui=always setting as well.

> ./git -c color.ui=always --exec-path=$PWD blame Makefile
>
> Shows no colors. Neither does:
>
> ./git -c color.ui=always --exec-path=$PWD -c 
> color.blame.highlightRecent="red,12 month ago,blue" blame Makefile
>
> And there's a bug, it segfaults on any custom value to the other config
> option:
>
> ./git -c color.ui=always --exec-path=$PWD -c color.blame.repeatedMeta=red 
> blame Makefile
>
> 0x004c312b in color_parse_mem (value=0x8f6520 "red", value_len=3, 
> dst=0x1 ) at color.c:272
> 272 OUT('\033');
>
> The repeated_meta_color variable is NULL when passed to
> color_parse_mem(). Didn't dig further.

Thanks for noticing.
Fix below (white space mangled)

--- i/builtin/blame.c
+++ w/builtin/blame.c
@@ -48,7 +48,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
-static char *repeated_meta_color;
+static char repeated_meta_color[COLOR_MAXLEN];

 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -1099,9 +1099,9 @@ int cmd_blame(int argc, const char **argv, const
char *prefix)

if (!(output_option & OUTPUT_PORCELAIN)) {
find_alignment(, _option);
-   if (!repeated_meta_color &&
+   if (!*repeated_meta_color &&
(output_option & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)))
-   repeated_meta_color = GIT_COLOR_DARK;
+   strcpy(repeated_meta_color, GIT_COLOR_DARK);
}

Re: [PATCH 11/26] serve: introduce git-serve

['Brandon Williams']

On 02/01, Randall S. Becker wrote:
> On February 1, 2018 1:58 PM, Stefan Beller wrote:
> > On Thu, Feb 1, 2018 at 10:48 AM, Jeff Hostetler 
> > wrote:
> > >
> > >
> > > On 1/2/2018 7:18 PM, Brandon Williams wrote:
> > >>
> > >> Introduce git-serve, the base server for protocol version 2.
> > >>
> > >> Protocol version 2 is intended to be a replacement for Git's current
> > >> wire protocol.  The intention is that it will be a simpler, less
> > >> wasteful protocol which can evolve over time.
> > >>
> > >> Protocol version 2 improves upon version 1 by eliminating the initial
> > >> ref advertisement.  In its place a server will export a list of
> > >> capabilities and commands which it supports in a capability
> > >> advertisement.  A client can then request that a particular command
> > >> be executed by providing a number of capabilities and command
> > >> specific parameters.  At the completion of a command, a client can
> > >> request that another command be executed or can terminate the
> > >> connection by sending a flush packet.
> > >>
> > >> Signed-off-by: Brandon Williams 
> > >> ---
> > >>   .gitignore  |   1 +
> > >>   Documentation/technical/protocol-v2.txt |  91 
> > >>   Makefile|   2 +
> > >>   builtin.h   |   1 +
> > >>   builtin/serve.c |  30 
> > >>   git.c   |   1 +
> > >>   serve.c | 239
> > >> 
> > >>   serve.h |  15 ++
> > >>   8 files changed, 380 insertions(+)
> > >>   create mode 100644 Documentation/technical/protocol-v2.txt
> > >>   create mode 100644 builtin/serve.c
> > >>   create mode 100644 serve.c
> > >>   create mode 100644 serve.h
> > >>
> > >> diff --git a/.gitignore b/.gitignore
> > >> index 833ef3b0b..2d0450c26 100644
> > >> --- a/.gitignore
> > >> +++ b/.gitignore
> > >> @@ -140,6 +140,7 @@
> > >>   /git-rm
> > >>   /git-send-email
> > >>   /git-send-pack
> > >> +/git-serve
> > >>   /git-sh-i18n
> > >>   /git-sh-i18n--envsubst
> > >>   /git-sh-setup
> > >> diff --git a/Documentation/technical/protocol-v2.txt
> > >> b/Documentation/technical/protocol-v2.txt
> > >> new file mode 100644
> > >> index 0..b87ba3816
> > >> --- /dev/null
> > >> +++ b/Documentation/technical/protocol-v2.txt
> > >> @@ -0,0 +1,91 @@
> > >> + Git Wire Protocol, Version 2
> > >> +==
> > >> +
> > >> +This document presents a specification for a version 2 of Git's wire
> > >> +protocol.  Protocol v2 will improve upon v1 in the following ways:
> > >> +
> > >> +  * Instead of multiple service names, multiple commands will be
> > >> +supported by a single service.
> > >> +  * Easily extendable as capabilities are moved into their own section
> > >> +of the protocol, no longer being hidden behind a NUL byte and
> > >> +limited by the size of a pkt-line (as there will be a single
> > >> +capability per pkt-line).
> > >> +  * Separate out other information hidden behind NUL bytes (e.g. agent
> > >> +string as a capability and symrefs can be requested using
> > >> + 'ls-refs')
> > >> +  * Reference advertisement will be omitted unless explicitly
> > >> + requested
> > >> +  * ls-refs command to explicitly request some refs
> > >> +
> > >> + Detailed Design
> > >> +=
> > >> +
> > >> +A client can request to speak protocol v2 by sending `version=2` in
> > >> +the side-channel `GIT_PROTOCOL` in the initial request to the server.
> > >> +
> > >> +In protocol v2 communication is command oriented.  When first
> > >> +contacting
> > >> a
> > >> +server a list of capabilities will advertised.  Some of these
> > >> capabilities
> > >> +will be commands which a client can request be executed.  Once a
> > >> +command has completed, a client can reuse the connection and request
> > >> +that other commands be executed.
> > >> +
> > >> + Special Packets
> > >> +-
> > >> +
> > >> +In protocol v2 these special packets will have the following semantics:
> > >> +
> > >> +  * '' Flush Packet (flush-pkt) - indicates the end of a message
> > >> +  * '0001' Delimiter Packet (delim-pkt) - separates sections of a
> > >> + message
> > >
> > >
> > > Previously, a 0001 pkt-line meant that there was 1 byte of data
> > > following, right?
> > 
> > No, the length was including the length field, so 0005 would indicate that
> > there is one byte following, (+4 bytes of "0005" included)
> > 
> > > Does this change that and/or prevent 1 byte packets?  (Not sure if it
> > > is likely, but the odd-tail of a packfile might get sent in a 0001
> > > line, right?)  Or is it that 0001 is only special during the V2
> > > negotiation stuff, but not during the packfile transmission?
> > 
> > 0001 is invalid in the current protocol v0.
> > 
> > >
> > > (I'm not 

Good Morning !

[Anny]

Good Morning , 



I have a project i would like to bring to you and i want you to help me discuss 
it. please let me know if you will be available for this project. 



Thanks 

Re: [PATCH 11/26] serve: introduce git-serve

[Brandon Williams]

On 02/01, Jeff Hostetler wrote:
> 
> 
> On 2/1/2018 1:57 PM, Stefan Beller wrote:
> > On Thu, Feb 1, 2018 at 10:48 AM, Jeff Hostetler  
> > wrote:
> > > 
> > > 
> > > On 1/2/2018 7:18 PM, Brandon Williams wrote:
> > > > 
> > > > Introduce git-serve, the base server for protocol version 2.
> [...]
> > > > + Special Packets
> > > > +-
> > > > +
> > > > +In protocol v2 these special packets will have the following semantics:
> > > > +
> > > > +  * '' Flush Packet (flush-pkt) - indicates the end of a message
> > > > +  * '0001' Delimiter Packet (delim-pkt) - separates sections of a 
> > > > message
> > > 
> > > 
> > > Previously, a 0001 pkt-line meant that there was 1 byte of data
> > > following, right?
> > 
> > No, the length was including the length field, so 0005 would indicate that
> > there is one byte following, (+4 bytes of "0005" included)
> 
> d'oh.  right.  thanks!
> 
> > > Should we also consider increasing the pkt-line limit to 5 hex-digits
> > > while we're at it ?   That would let us have 1MB buffers if that would
> > > help with large packfiles.
> > 
> > AFAICT there is a static allocation of one pkt-line (of maximum size),
> > such that the code can read in a full packet and then process it.
> > If we'd increase the packet size we'd need the static buffer to be 1MB,
> > which sounds good for my developer machine. But I suspect it may be
> > too much for people using git on embedded devices?
> 
> I got burned by that static buffer once upon a time when I wanted
> to have 2 streams going at the same time.  Hopefully, we can move
> that into the new reader structure at some point (if it isn't already).

Yeah the reader struct could easily be extended to take in the
buffer to read the data into.  Because I'm not trying to do any of that
atm I decided to have it default to using the static buffer, but it
would be as simple as changing the reader->buffer variable to use a
different buffer.

> 
> > 
> > pack files larger than 64k are put into multiple pkt-lines, which is
> > not a big deal, as the overhead of 4bytes per 64k is negligible.
> > (also there is progress information in the side channel, which
> > would come in as a special packet in between real packets,
> > such that every 64k transmitted you can update your progress
> > meter; Not sure I feel strongly on fewer progress updates)
> > 
> > >   Granted, we're throttled by the network,
> > > so it might not matter.  Would it be interesting to have a 5 digit
> > > prefix with parts of the high bits of first digit being flags ?
> > > Or is this too radical of a change?
> > 
> > What would the flags be for?
> > 
> > As an alternative we could put the channel number in one byte,
> > such that we can have a side channel not just while streaming the
> > pack but all the time. (Again, not sure if that buys a lot for us)
> > 
> 
> Delimiters like the 0001 and the side channel are a couple of
> ideas, but I was just thinking out loud.  And right, I'm not sure
> it gets us much right now.
> 
> Jeff

-- 
Brandon Williams

RE: [PATCH 11/26] serve: introduce git-serve

[Randall S. Becker]

On February 1, 2018 1:58 PM, Stefan Beller wrote:
> On Thu, Feb 1, 2018 at 10:48 AM, Jeff Hostetler 
> wrote:
> >
> >
> > On 1/2/2018 7:18 PM, Brandon Williams wrote:
> >>
> >> Introduce git-serve, the base server for protocol version 2.
> >>
> >> Protocol version 2 is intended to be a replacement for Git's current
> >> wire protocol.  The intention is that it will be a simpler, less
> >> wasteful protocol which can evolve over time.
> >>
> >> Protocol version 2 improves upon version 1 by eliminating the initial
> >> ref advertisement.  In its place a server will export a list of
> >> capabilities and commands which it supports in a capability
> >> advertisement.  A client can then request that a particular command
> >> be executed by providing a number of capabilities and command
> >> specific parameters.  At the completion of a command, a client can
> >> request that another command be executed or can terminate the
> >> connection by sending a flush packet.
> >>
> >> Signed-off-by: Brandon Williams 
> >> ---
> >>   .gitignore  |   1 +
> >>   Documentation/technical/protocol-v2.txt |  91 
> >>   Makefile|   2 +
> >>   builtin.h   |   1 +
> >>   builtin/serve.c |  30 
> >>   git.c   |   1 +
> >>   serve.c | 239
> >> 
> >>   serve.h |  15 ++
> >>   8 files changed, 380 insertions(+)
> >>   create mode 100644 Documentation/technical/protocol-v2.txt
> >>   create mode 100644 builtin/serve.c
> >>   create mode 100644 serve.c
> >>   create mode 100644 serve.h
> >>
> >> diff --git a/.gitignore b/.gitignore
> >> index 833ef3b0b..2d0450c26 100644
> >> --- a/.gitignore
> >> +++ b/.gitignore
> >> @@ -140,6 +140,7 @@
> >>   /git-rm
> >>   /git-send-email
> >>   /git-send-pack
> >> +/git-serve
> >>   /git-sh-i18n
> >>   /git-sh-i18n--envsubst
> >>   /git-sh-setup
> >> diff --git a/Documentation/technical/protocol-v2.txt
> >> b/Documentation/technical/protocol-v2.txt
> >> new file mode 100644
> >> index 0..b87ba3816
> >> --- /dev/null
> >> +++ b/Documentation/technical/protocol-v2.txt
> >> @@ -0,0 +1,91 @@
> >> + Git Wire Protocol, Version 2
> >> +==
> >> +
> >> +This document presents a specification for a version 2 of Git's wire
> >> +protocol.  Protocol v2 will improve upon v1 in the following ways:
> >> +
> >> +  * Instead of multiple service names, multiple commands will be
> >> +supported by a single service.
> >> +  * Easily extendable as capabilities are moved into their own section
> >> +of the protocol, no longer being hidden behind a NUL byte and
> >> +limited by the size of a pkt-line (as there will be a single
> >> +capability per pkt-line).
> >> +  * Separate out other information hidden behind NUL bytes (e.g. agent
> >> +string as a capability and symrefs can be requested using
> >> + 'ls-refs')
> >> +  * Reference advertisement will be omitted unless explicitly
> >> + requested
> >> +  * ls-refs command to explicitly request some refs
> >> +
> >> + Detailed Design
> >> +=
> >> +
> >> +A client can request to speak protocol v2 by sending `version=2` in
> >> +the side-channel `GIT_PROTOCOL` in the initial request to the server.
> >> +
> >> +In protocol v2 communication is command oriented.  When first
> >> +contacting
> >> a
> >> +server a list of capabilities will advertised.  Some of these
> >> capabilities
> >> +will be commands which a client can request be executed.  Once a
> >> +command has completed, a client can reuse the connection and request
> >> +that other commands be executed.
> >> +
> >> + Special Packets
> >> +-
> >> +
> >> +In protocol v2 these special packets will have the following semantics:
> >> +
> >> +  * '' Flush Packet (flush-pkt) - indicates the end of a message
> >> +  * '0001' Delimiter Packet (delim-pkt) - separates sections of a
> >> + message
> >
> >
> > Previously, a 0001 pkt-line meant that there was 1 byte of data
> > following, right?
> 
> No, the length was including the length field, so 0005 would indicate that
> there is one byte following, (+4 bytes of "0005" included)
> 
> > Does this change that and/or prevent 1 byte packets?  (Not sure if it
> > is likely, but the odd-tail of a packfile might get sent in a 0001
> > line, right?)  Or is it that 0001 is only special during the V2
> > negotiation stuff, but not during the packfile transmission?
> 
> 0001 is invalid in the current protocol v0.
> 
> >
> > (I'm not against having this delimiter -- I think it is useful, but
> > just curious if will cause problems elsewhere.)
> >
> > Should we also consider increasing the pkt-line limit to 5 hex-digits
> > while we're at it ?   That would let us have 1MB buffers if that would
> > help 

Re: [PATCH v2 00/27] protocol version 2

[Jeff Hostetler]

On 1/25/2018 6:58 PM, Brandon Williams wrote:

Changes in v2:
  * Added documentation for fetch
  * changes #defines for state variables to be enums
  * couple code changes to pkt-line functions and documentation
  * Added unit tests for the git-serve binary as well as for ls-refs

[...]


This looks really nice.  I'm eager to get this in so we can do some
additional commands to help make partial clone more efficient.

Thanks,
Jeff

Re: [PATCHv3 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 04 2018, Stefan Beller jotted:

> Stefan Beller (4):
>   color.h: document and modernize header
>   builtin/blame: dim uninteresting metadata
>   builtin/blame: add option to color metadata fields separately
>   builtin/blame: highlight recently changed lines

I like this feature in principle, but I can't get it to work. Building
pu and:

./git -c color.ui=always --exec-path=$PWD blame Makefile

Shows no colors. Neither does:

./git -c color.ui=always --exec-path=$PWD -c 
color.blame.highlightRecent="red,12 month ago,blue" blame Makefile

And there's a bug, it segfaults on any custom value to the other config
option:

./git -c color.ui=always --exec-path=$PWD -c color.blame.repeatedMeta=red 
blame Makefile

0x004c312b in color_parse_mem (value=0x8f6520 "red", value_len=3, 
dst=0x1 ) at color.c:272
272 OUT('\033');

The repeated_meta_color variable is NULL when passed to
color_parse_mem(). Didn't dig further.

Re: [PATCH 12/26] ls-refs: introduce ls-refs server command

[Jeff Hostetler]

On 1/2/2018 7:18 PM, Brandon Williams wrote:

Introduce the ls-refs server command.  In protocol v2, the ls-refs
command is used to request the ref advertisement from the server.  Since
it is a command which can be requested (as opposed to mandatory in v1),
a client can sent a number of parameters in its request to limit the ref
advertisement based on provided ref-patterns.

Signed-off-by: Brandon Williams 
---
  Documentation/technical/protocol-v2.txt | 26 +
  Makefile|  1 +
  ls-refs.c   | 97 +
  ls-refs.h   |  9 +++
  serve.c |  2 +
  5 files changed, 135 insertions(+)
  create mode 100644 ls-refs.c
  create mode 100644 ls-refs.h

diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
index b87ba3816..5f4d0e719 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -89,3 +89,29 @@ terminate the connection.
  Commands are the core actions that a client wants to perform (fetch, push,
  etc).  Each command will be provided with a list capabilities and
  arguments as requested by a client.
+
+ Ls-refs
+-
+
+Ls-refs is the command used to request a reference advertisement in v2.
+Unlike the current reference advertisement, ls-refs takes in parameters
+which can be used to limit the refs sent from the server.
+
+Ls-ref takes in the following parameters wraped in packet-lines:
+
+  symrefs: In addition to the object pointed by it, show the underlying
+  ref pointed by it when showing a symbolic ref.
+  peel: Show peeled tags.
+  ref-pattern : When specified, only references matching the
+given patterns are displayed.
+
+The output of ls-refs is as follows:
+
+output = *ref
+flush-pkt
+ref = PKT-LINE((tip | peeled) LF)
+tip = obj-id SP refname (SP symref-target)
+peeled = obj-id SP refname "^{}"
+
+symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
+shallow = PKT-LINE("shallow" SP obj-id LF)


Do you want to talk about ordering requirements on this?
I think packed-refs has one, but I'm not sure it matters here
where the client or server sorts it.

Are there any provisions for compressing the renames, like in the
reftable spec or in index-v4 ?

It doesn't need to be in the initial version.  Just asking.  We could
always add a "ls-refs-2" command that builds upon this.

Thanks,
Jeff

Re: [PATCH v2 01/41] parse-options: support --git-completion-helper

[Eric Sunshine]

On Thu, Feb 1, 2018 at 5:21 AM, Duy Nguyen  wrote:
> On Thu, Feb 1, 2018 at 4:54 PM, Eric Sunshine  wrote:
>> I don't see that as convincing argument for two classes of "no
>> complete". Since git-completion.bash already special-cases
>> rebase/am/cherry-pick for --continue|--abort|--skip, it is not far
>> fetched that that special-case treatment can be extended slightly to
>> also filter out those three options from the list returned by
>> --git-completion-helper.
>
> I agree that is possible, but it's a bit tricky to do the filtering
> right in bash (all options are sent back as one line instead of one
> per line, which is easier to process by command line tools).

Perhaps I'm missing something, but wouldn't filtering out those
options directly in Bash require only this?

% x='--foo --bar --snoo'
% echo ${x/--bar}
--foo --snoo

> On top of  that, I think we want git-completion.bash to be fast, the
> more commands we execute there, the unhappier Windows users are. It is
> too possible to do this kind of filtering just once, before caching.
> It does not fit well to how I designed __gitcomp_builtin so I need to
> sit back and see how to sort that out.

The filtering as illustrated above using builtin Bash functionality
seems unlikely to be a source of noticeable slow down on Windows.

And, if you're concerned about it not fitting the design of
__gitcomp_builtin, it's almost certainly cheap enough to do the
filtering each time its needed rather than worrying about caching the
filtered list. (It's hard to imagine anyone noticing a slow down of
three extra ${x/} substitutions.)

Re: [PATCH 11/26] serve: introduce git-serve

[Jeff Hostetler]

On 2/1/2018 1:57 PM, Stefan Beller wrote:

On Thu, Feb 1, 2018 at 10:48 AM, Jeff Hostetler  wrote:



On 1/2/2018 7:18 PM, Brandon Williams wrote:


Introduce git-serve, the base server for protocol version 2.

[...]

+ Special Packets
+-
+
+In protocol v2 these special packets will have the following semantics:
+
+  * '' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message



Previously, a 0001 pkt-line meant that there was 1 byte of data
following, right?


No, the length was including the length field, so 0005 would indicate that
there is one byte following, (+4 bytes of "0005" included)


d'oh.  right.  thanks!


Should we also consider increasing the pkt-line limit to 5 hex-digits
while we're at it ?   That would let us have 1MB buffers if that would
help with large packfiles.


AFAICT there is a static allocation of one pkt-line (of maximum size),
such that the code can read in a full packet and then process it.
If we'd increase the packet size we'd need the static buffer to be 1MB,
which sounds good for my developer machine. But I suspect it may be
too much for people using git on embedded devices?


I got burned by that static buffer once upon a time when I wanted
to have 2 streams going at the same time.  Hopefully, we can move
that into the new reader structure at some point (if it isn't already).



pack files larger than 64k are put into multiple pkt-lines, which is
not a big deal, as the overhead of 4bytes per 64k is negligible.
(also there is progress information in the side channel, which
would come in as a special packet in between real packets,
such that every 64k transmitted you can update your progress
meter; Not sure I feel strongly on fewer progress updates)


  Granted, we're throttled by the network,
so it might not matter.  Would it be interesting to have a 5 digit
prefix with parts of the high bits of first digit being flags ?
Or is this too radical of a change?


What would the flags be for?

As an alternative we could put the channel number in one byte,
such that we can have a side channel not just while streaming the
pack but all the time. (Again, not sure if that buys a lot for us)



Delimiters like the 0001 and the side channel are a couple of
ideas, but I was just thinking out loud.  And right, I'm not sure
it gets us much right now.

Jeff

Re: [PATCH 11/26] serve: introduce git-serve

[Stefan Beller]

On Thu, Feb 1, 2018 at 10:48 AM, Jeff Hostetler  wrote:
>
>
> On 1/2/2018 7:18 PM, Brandon Williams wrote:
>>
>> Introduce git-serve, the base server for protocol version 2.
>>
>> Protocol version 2 is intended to be a replacement for Git's current
>> wire protocol.  The intention is that it will be a simpler, less
>> wasteful protocol which can evolve over time.
>>
>> Protocol version 2 improves upon version 1 by eliminating the initial
>> ref advertisement.  In its place a server will export a list of
>> capabilities and commands which it supports in a capability
>> advertisement.  A client can then request that a particular command be
>> executed by providing a number of capabilities and command specific
>> parameters.  At the completion of a command, a client can request that
>> another command be executed or can terminate the connection by sending a
>> flush packet.
>>
>> Signed-off-by: Brandon Williams 
>> ---
>>   .gitignore  |   1 +
>>   Documentation/technical/protocol-v2.txt |  91 
>>   Makefile|   2 +
>>   builtin.h   |   1 +
>>   builtin/serve.c |  30 
>>   git.c   |   1 +
>>   serve.c | 239
>> 
>>   serve.h |  15 ++
>>   8 files changed, 380 insertions(+)
>>   create mode 100644 Documentation/technical/protocol-v2.txt
>>   create mode 100644 builtin/serve.c
>>   create mode 100644 serve.c
>>   create mode 100644 serve.h
>>
>> diff --git a/.gitignore b/.gitignore
>> index 833ef3b0b..2d0450c26 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -140,6 +140,7 @@
>>   /git-rm
>>   /git-send-email
>>   /git-send-pack
>> +/git-serve
>>   /git-sh-i18n
>>   /git-sh-i18n--envsubst
>>   /git-sh-setup
>> diff --git a/Documentation/technical/protocol-v2.txt
>> b/Documentation/technical/protocol-v2.txt
>> new file mode 100644
>> index 0..b87ba3816
>> --- /dev/null
>> +++ b/Documentation/technical/protocol-v2.txt
>> @@ -0,0 +1,91 @@
>> + Git Wire Protocol, Version 2
>> +==
>> +
>> +This document presents a specification for a version 2 of Git's wire
>> +protocol.  Protocol v2 will improve upon v1 in the following ways:
>> +
>> +  * Instead of multiple service names, multiple commands will be
>> +supported by a single service.
>> +  * Easily extendable as capabilities are moved into their own section
>> +of the protocol, no longer being hidden behind a NUL byte and
>> +limited by the size of a pkt-line (as there will be a single
>> +capability per pkt-line).
>> +  * Separate out other information hidden behind NUL bytes (e.g. agent
>> +string as a capability and symrefs can be requested using 'ls-refs')
>> +  * Reference advertisement will be omitted unless explicitly requested
>> +  * ls-refs command to explicitly request some refs
>> +
>> + Detailed Design
>> +=
>> +
>> +A client can request to speak protocol v2 by sending `version=2` in the
>> +side-channel `GIT_PROTOCOL` in the initial request to the server.
>> +
>> +In protocol v2 communication is command oriented.  When first contacting
>> a
>> +server a list of capabilities will advertised.  Some of these
>> capabilities
>> +will be commands which a client can request be executed.  Once a command
>> +has completed, a client can reuse the connection and request that other
>> +commands be executed.
>> +
>> + Special Packets
>> +-
>> +
>> +In protocol v2 these special packets will have the following semantics:
>> +
>> +  * '' Flush Packet (flush-pkt) - indicates the end of a message
>> +  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message
>
>
> Previously, a 0001 pkt-line meant that there was 1 byte of data
> following, right?

No, the length was including the length field, so 0005 would indicate that
there is one byte following, (+4 bytes of "0005" included)

> Does this change that and/or prevent 1 byte
> packets?  (Not sure if it is likely, but the odd-tail of a packfile
> might get sent in a 0001 line, right?)  Or is it that 0001 is only
> special during the V2 negotiation stuff, but not during the packfile
> transmission?

0001 is invalid in the current protocol v0.

>
> (I'm not against having this delimiter -- I think it is useful, but
> just curious if will cause problems elsewhere.)
>
> Should we also consider increasing the pkt-line limit to 5 hex-digits
> while we're at it ?   That would let us have 1MB buffers if that would
> help with large packfiles.

AFAICT there is a static allocation of one pkt-line (of maximum size),
such that the code can read in a full packet and then process it.
If we'd increase the packet size we'd need the static buffer to be 1MB,
which sounds good for my developer machine. But I suspect it may be
too much 

[PATCH] cocci: simplify check for trivial format strings

[René Scharfe]

353d84c537 (coccicheck: make transformation for strbuf_addf(sb, "...")
more precise) added a check to avoid transforming calls with format
strings which contain percent signs, as that would change the result.
It uses embedded Python code for that.  Simplify this rule by using the
regular expression matching operator instead.

Signed-off-by: Rene Scharfe 
---
Inspired by the Coccinelle package in Debian experimental, which lost
support for Python for some reason.  Tested only with that version
(1.0.6.deb-3) and Debian testing's 1.0.4.deb-3+b3.

 contrib/coccinelle/strbuf.cocci | 17 +
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
index 6fe8727421..e34eada1ad 100644
--- a/contrib/coccinelle/strbuf.cocci
+++ b/contrib/coccinelle/strbuf.cocci
@@ -1,21 +1,6 @@
 @ strbuf_addf_with_format_only @
 expression E;
-constant fmt;
-@@
-  strbuf_addf(E,
-(
-  fmt
-|
-  _(fmt)
-)
-  );
-
-@ script:python @
-fmt << strbuf_addf_with_format_only.fmt;
-@@
-cocci.include_match("%" not in fmt)
-
-@ extends strbuf_addf_with_format_only @
+constant fmt !~ "%";
 @@
 - strbuf_addf
 + strbuf_addstr
-- 
2.16.1

Re: [PATCH 11/26] serve: introduce git-serve

[Jeff Hostetler]

On 1/2/2018 7:18 PM, Brandon Williams wrote:

Introduce git-serve, the base server for protocol version 2.

Protocol version 2 is intended to be a replacement for Git's current
wire protocol.  The intention is that it will be a simpler, less
wasteful protocol which can evolve over time.

Protocol version 2 improves upon version 1 by eliminating the initial
ref advertisement.  In its place a server will export a list of
capabilities and commands which it supports in a capability
advertisement.  A client can then request that a particular command be
executed by providing a number of capabilities and command specific
parameters.  At the completion of a command, a client can request that
another command be executed or can terminate the connection by sending a
flush packet.

Signed-off-by: Brandon Williams 
---
  .gitignore  |   1 +
  Documentation/technical/protocol-v2.txt |  91 
  Makefile|   2 +
  builtin.h   |   1 +
  builtin/serve.c |  30 
  git.c   |   1 +
  serve.c | 239 
  serve.h |  15 ++
  8 files changed, 380 insertions(+)
  create mode 100644 Documentation/technical/protocol-v2.txt
  create mode 100644 builtin/serve.c
  create mode 100644 serve.c
  create mode 100644 serve.h

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..2d0450c26 100644
--- a/.gitignore
+++ b/.gitignore
@@ -140,6 +140,7 @@
  /git-rm
  /git-send-email
  /git-send-pack
+/git-serve
  /git-sh-i18n
  /git-sh-i18n--envsubst
  /git-sh-setup
diff --git a/Documentation/technical/protocol-v2.txt 
b/Documentation/technical/protocol-v2.txt
new file mode 100644
index 0..b87ba3816
--- /dev/null
+++ b/Documentation/technical/protocol-v2.txt
@@ -0,0 +1,91 @@
+ Git Wire Protocol, Version 2
+==
+
+This document presents a specification for a version 2 of Git's wire
+protocol.  Protocol v2 will improve upon v1 in the following ways:
+
+  * Instead of multiple service names, multiple commands will be
+supported by a single service.
+  * Easily extendable as capabilities are moved into their own section
+of the protocol, no longer being hidden behind a NUL byte and
+limited by the size of a pkt-line (as there will be a single
+capability per pkt-line).
+  * Separate out other information hidden behind NUL bytes (e.g. agent
+string as a capability and symrefs can be requested using 'ls-refs')
+  * Reference advertisement will be omitted unless explicitly requested
+  * ls-refs command to explicitly request some refs
+
+ Detailed Design
+=
+
+A client can request to speak protocol v2 by sending `version=2` in the
+side-channel `GIT_PROTOCOL` in the initial request to the server.
+
+In protocol v2 communication is command oriented.  When first contacting a
+server a list of capabilities will advertised.  Some of these capabilities
+will be commands which a client can request be executed.  Once a command
+has completed, a client can reuse the connection and request that other
+commands be executed.
+
+ Special Packets
+-
+
+In protocol v2 these special packets will have the following semantics:
+
+  * '' Flush Packet (flush-pkt) - indicates the end of a message
+  * '0001' Delimiter Packet (delim-pkt) - separates sections of a message


Previously, a 0001 pkt-line meant that there was 1 byte of data
following, right?  Does this change that and/or prevent 1 byte
packets?  (Not sure if it is likely, but the odd-tail of a packfile
might get sent in a 0001 line, right?)  Or is it that 0001 is only
special during the V2 negotiation stuff, but not during the packfile
transmission?

(I'm not against having this delimiter -- I think it is useful, but
just curious if will cause problems elsewhere.)

Should we also consider increasing the pkt-line limit to 5 hex-digits
while we're at it ?   That would let us have 1MB buffers if that would
help with large packfiles.  Granted, we're throttled by the network,
so it might not matter.  Would it be interesting to have a 5 digit
prefix with parts of the high bits of first digit being flags ?
Or is this too radical of a change?



+
+ Capability Advertisement
+--
+
+A server which decides to communicate (based on a request from a client)
+using protocol version 2, notifies the client by sending a version string
+in its initial response followed by an advertisement of its capabilities.
+Each capability is a key with an optional value.  Clients must ignore all
+unknown keys.  Semantics of unknown values are left to the definition of
+each key.  Some capabilities will describe commands which can be requested
+to be executed by the client.
+
+capability-advertisement = protocol-version
+  

Re: [PATCH v2 08/27] connect: discover protocol version outside of get_remote_heads

[Brandon Williams]

On 01/31, Derrick Stolee wrote:
> On 1/25/2018 6:58 PM, Brandon Williams wrote:
> > In order to prepare for the addition of protocol_v2 push the protocol
> > version discovery outside of 'get_remote_heads()'.  This will allow for
> > keeping the logic for processing the reference advertisement for
> > protocol_v1 and protocol_v0 separate from the logic for protocol_v2.
> > 
> > Signed-off-by: Brandon Williams 
> > ---
> >   builtin/fetch-pack.c | 16 +++-
> >   builtin/send-pack.c  | 17 +++--
> >   connect.c| 27 ++-
> >   connect.h|  3 +++
> >   remote-curl.c| 20 ++--
> >   remote.h |  5 +++--
> >   transport.c  | 24 +++-
> >   7 files changed, 83 insertions(+), 29 deletions(-)
> > 
> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index 366b9d13f..85d4faf76 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -4,6 +4,7 @@
> >   #include "remote.h"
> >   #include "connect.h"
> >   #include "sha1-array.h"
> > +#include "protocol.h"
> >   static const char fetch_pack_usage[] =
> >   "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
> > @@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> > char *prefix)
> > struct fetch_pack_args args;
> > struct oid_array shallow = OID_ARRAY_INIT;
> > struct string_list deepen_not = STRING_LIST_INIT_DUP;
> > +   struct packet_reader reader;
> > packet_trace_identity("fetch-pack");
> > @@ -193,7 +195,19 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> > char *prefix)
> > if (!conn)
> > return args.diag_url ? 0 : 1;
> > }
> > -   get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
> > +
> > +   packet_reader_init(, fd[0], NULL, 0,
> > +  PACKET_READ_CHOMP_NEWLINE |
> > +  PACKET_READ_GENTLE_ON_EOF);
> > +
> > +   switch (discover_version()) {
> > +   case protocol_v1:
> > +   case protocol_v0:
> > +   get_remote_heads(, , 0, NULL, );
> > +   break;
> > +   case protocol_unknown_version:
> > +   BUG("unknown protocol version");
> 
> Is this really a BUG in the client, or a bug/incompatibility in the server?
> 
> Perhaps I'm misunderstanding, but it looks like discover_version() will
> die() on an unknown version (the die() is in
> protocol.c:determine_protocol_version_client()). So maybe that's why this is
> a BUG()?
> 
> If there is something to change here, this BUG() appears three more times.

Yes, I have it labeled as a BUG because discover_version can't return an
unknown protocol version.  If the server actually returns an unknown
protocol version then it should be handled in
protocol.c:determine_protocol_version_client() as you mentioned.

> 
> > +   }
> > ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
> >  , pack_lockfile_ptr);
> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index fc4f0bb5f..83cb125a6 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -14,6 +14,7 @@
> >   #include "sha1-array.h"
> >   #include "gpg-interface.h"
> >   #include "gettext.h"
> > +#include "protocol.h"
> >   static const char * const send_pack_usage[] = {
> > N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
> > @@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const 
> > char *prefix)
> > int progress = -1;
> > int from_stdin = 0;
> > struct push_cas_option cas = {0};
> > +   struct packet_reader reader;
> > struct option options[] = {
> > OPT__VERBOSITY(),
> > @@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const 
> > char *prefix)
> > args.verbose ? CONNECT_VERBOSE : 0);
> > }
> > -   get_remote_heads(fd[0], NULL, 0, _refs, REF_NORMAL,
> > -_have, );
> > +   packet_reader_init(, fd[0], NULL, 0,
> > +  PACKET_READ_CHOMP_NEWLINE |
> > +  PACKET_READ_GENTLE_ON_EOF);
> > +
> > +   switch (discover_version()) {
> > +   case protocol_v1:
> > +   case protocol_v0:
> > +   get_remote_heads(, _refs, REF_NORMAL,
> > +_have, );
> > +   break;
> > +   case protocol_unknown_version:
> > +   BUG("unknown protocol version");
> > +   }
> > transport_verify_remote_names(nr_refspecs, refspecs);
> > diff --git a/connect.c b/connect.c
> > index 00e90075c..db3c9d24c 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
> >   "and the repository exists."));
> >   }
> > -static enum protocol_version discover_version(struct packet_reader *reader)
> > +enum protocol_version discover_version(struct packet_reader *reader)
> >   {
> > enum protocol_version version = protocol_unknown_version;

[PATCHv2] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Add a --edit option whichs allows modifying the messages provided by -m or -F,
the same way git commit --edit does.

Signed-off-by: Nicolas Morey-Chaisemartin 
---

Changes since v1:
- Fix usage string
- Use write_script to generate editor
- Rename editor to fakeeditor to match the other tests in the testsuite
- I'll post another series to fix the misleading messages in both commit.c and 
tag.c when launch_editor fails

 Documentation/git-tag.txt |  6 ++
 builtin/tag.c | 11 +--
 t/t7004-tag.sh| 30 ++
 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f984..b9e5a993bea0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
annotation lines.
Implies `-a` if none of `-a`, `-s`, or `-u `
is given.
 
+-e::
+--edit::
+   The message taken from file with `-F` and command line with
+   `-m` are usually used as the tag message unmodified.
+   This option lets you further edit the message taken from these sources.
+
 --cleanup=::
This option sets how the tag message is cleaned up.
The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
diff --git a/builtin/tag.c b/builtin/tag.c
index a7e6a5b0f234..ce5cac3dd23f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
struct object_id *resu
 
 struct create_tag_options {
unsigned int message_given:1;
+   unsigned int use_editor:1;
unsigned int sign;
enum {
CLEANUP_NONE,
@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
const char *tag,
tag,
git_committer_info(IDENT_STRICT));
 
-   if (!opt->message_given) {
+   if (!opt->message_given || opt->use_editor) {
int fd;
 
/* write the template message before editing: */
@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
const char *tag,
if (fd < 0)
die_errno(_("could not create file '%s'"), path);
 
-   if (!is_null_oid(prev)) {
+   if (opt->message_given) {
+   write_or_die(fd, buf->buf, buf->len);
+   strbuf_reset(buf);
+   } else if (!is_null_oid(prev)) {
write_tag_body(fd, prev);
} else {
struct strbuf buf = STRBUF_INIT;
@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+   int edit_flag = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, , N_("n"),
@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", , N_("message"),
 N_("tag message"), 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")),
OPT_STRING(0, "cleanup", _arg, N_("mode"),
N_("how to strip spaces and #comments from message")),
@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("tag '%s' already exists"), tag);
 
opt.message_given = msg.given || msgfile;
+   opt.use_editor = edit_flag;
 
if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a9af2de9960b..063996ddc05c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,21 @@ test_expect_success \
test_cmp expect actual
 '
 
+get_tag_header annotated-tag-edit $commit commit $time >expect
+echo "An edited message" >>expect
+test_expect_success 'set up editor' '
+   write_script fakeeditor <<-\EOF
+   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+   mv "$1-" "$1"
+   EOF
+'
+test_expect_success \
+   'creating an annotated tag with -m message --edit should succeed' '
+   EDITOR=./fakeeditor git tag -m "A message" --edit 
annotated-tag-edit &&
+   get_tag_msg annotated-tag-edit >actual &&
+   test_cmp expect actual
+'
+
 cat >msgfile >expect
+test_expect_success 'set up editor' '
+   write_script fakeeditor <<-\EOF
+   sed -e "s/Another message/Another edited 

Respond for details

[Mr. Allen]

Dear sir/ madam


Do you need a loan to pay off bills ? To pay off your mortgage quickly ? To set 
up a new business or to Re- finance your existing business ? I can help you 
secure a private loan at 3% interest rate please respond for more details



Thanks


Allen

[no subject]

[Charles Stanley]

Guess you are fine? I have been trying to reach you! 

Respond for details

[Mr. Allen]

Dear sir/ madam


Do you need a loan to pay off bills ? To pay off your mortgage quickly ? To set 
up a new business or to Re- finance your existing business ? I can help you 
secure a private loan at 3% interest rate please respond for more details



Thanks


Allen

Re: [PATCH] tag: add --edit option

[Ramsay Jones]

On 01/02/18 09:49, Nicolas Morey-Chaisemartin wrote:
> Add a --edit option whichs allows modifying the messages provided by -m or -F,
> the same way git commit --edit does.
> 
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>  Documentation/git-tag.txt |  6 ++
>  builtin/tag.c | 11 +--
>  t/t7004-tag.sh| 34 ++
>  3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 956fc019f984..b9e5a993bea0 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
> annotation lines.
>   Implies `-a` if none of `-a`, `-s`, or `-u `
>   is given.
>  
> +-e::
> +--edit::
> + The message taken from file with `-F` and command line with
> + `-m` are usually used as the tag message unmodified.
> + This option lets you further edit the message taken from these sources.
> +
>  --cleanup=::
>   This option sets how the tag message is cleaned up.
>   The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
> diff --git a/builtin/tag.c b/builtin/tag.c
> index a7e6a5b0f234..91c60829d5f9 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
> struct object_id *resu
>  
>  struct create_tag_options {
>   unsigned int message_given:1;
> + unsigned int use_editor:1;
>   unsigned int sign;
>   enum {
>   CLEANUP_NONE,
> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
> const char *tag,
>   tag,
>   git_committer_info(IDENT_STRICT));
>  
> - if (!opt->message_given) {
> + if (!opt->message_given || opt->use_editor) {
>   int fd;
>  
>   /* write the template message before editing: */
> @@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
> const char *tag,
>   if (fd < 0)
>   die_errno(_("could not create file '%s'"), path);
>  
> - if (!is_null_oid(prev)) {
> + if (opt->message_given) {
> + write_or_die(fd, buf->buf, buf->len);
> + strbuf_reset(buf);
> + } else if (!is_null_oid(prev)) {
>   write_tag_body(fd, prev);
>   } else {
>   struct strbuf buf = STRBUF_INIT;
> @@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   static struct ref_sorting *sorting = NULL, **sorting_tail = 
>   struct ref_format format = REF_FORMAT_INIT;
>   int icase = 0;
> + int edit_flag = 0;
>   struct option options[] = {
>   OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
>   { OPTION_INTEGER, 'n', NULL, , N_("n"),
> @@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   OPT_CALLBACK('m', "message", , N_("message"),
>N_("tag message"), parse_msg_arg),
>   OPT_FILENAME('F', "file", , N_("read message from 
> file")),
> + OPT_BOOL('e', "edit", _flag, N_("force edit of commit")),

s/commit/tag message/ ?

ATB,
Ramsay Jones

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:56, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 5:43 AM, Nicolas Morey-Chaisemartin
>  wrote:
>> Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>>> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
 A little below this change is where launch_editor() is actually
 invoked. If it fails for some reason, it prints:

 Please supply the message using either -m or -F option.

 which seems a bit counterintuitive if the user *did* specify one of
 those options along with --edit. I wonder if that message needs to be
 adjusted.

>>> Yes I'll fix this.
>> I just checked what commit.c does and it seems to behave as my patch:
>> if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>> fprintf(stderr,
>> _("Please supply the message using either -m or -F option.\n"));
>>
>> To be honest the message is not that clear either.
>> If I'm reading launch_editor right most (or all) its falire are du to a 
>> failure to launch the editor or the editor crashed/exited with an error.
>> In this case, I wouldn't advise the user to use -m or -F but to fix its 
>> editor.
> Indeed, I also looked at the implementation of launch_editor(), and my
> "wondering" about whether the message needed adjustment was just that.
> The message seems somewhat counterintuitive in this case, but I didn't
> necessarily have a better suggestion. A valid response, therefore,
> might be to punt on it and leave that change for the future, or
> perhaps take it on as a second patch which adjusts the message in both
> commands. I don't have strong feelings about it at this time.

It seems all the error paths from launch_editor have an error message.
A simple "Editor failure, cancelling {commit, tag}" would probably be a better 
error message.
I'll post another series for that.

t3404.6 breaks on master under GIT_FSMONITOR_TEST

[Ævar Arnfjörð Bjarmason]

The GIT_FSMONITOR_TEST variable allows you to roundtrip the fsmonitor
codpath in the whole test suite. On both Debian & CentOS this breaks for
me:

(cd t && GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all 
./t3404-rebase-interactive.sh -i)

Whereas this works:

(cd t && GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all GIT_SKIP_TESTS=t3404.6 
./t3404-rebase-interactive.sh -i)

The entirety of the rest of the test suite still passes with
GIT_FSMONITOR_TEST.

This has been failing ever since GIT_FSMONITOR_TEST was introduced in
883e248b8a ("fsmonitor: teach git to optionally utilize a file system
monitor to speed up detecting new or changed files.", 2017-09-22). Under
-v -x -i:

+ echo test_must_fail: command succeeded: env 
FAKE_LINES=exec_echo_foo_>file1 1 git rebase -i HEAD^
test_must_fail: command succeeded: env FAKE_LINES=exec_echo_foo_>file1 1 
git rebase -i HEAD^
+ return 1
error: last command exited with $?=1
not ok 6 - rebase -i with the exec command checks tree cleanness
#
#   git checkout master &&
#   set_fake_editor &&
#   test_must_fail env FAKE_LINES="exec_echo_foo_>file1 1" git 
rebase -i HEAD^ &&

Maybe once this is fixed running the test suite under GIT_FSMONITOR_TEST
would be a useful Travis target, but I don't know the current status of
adding new options to Travis.

[PATCH v3 2/2] diff: add --stat-with-summary

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

Certain information is currently shown with --summary, but when used
in combination with --stat it's a bit hard to read since info of the
same file is in two places (--stat and --summary).

On top of that, commits that add or remove files double the number of
display lines, which could be a lot if you add or remove a lot of
files.

--stat-with-summary embeds most of --summary back in --stat in the
little space between the file name part and the graph line, e.g. with
commit 0433d533f1:

   Documentation/merge-config.txt |  4 +
   builtin/merge.c|  2 +
   ...-pull-verify-signatures.sh (new +x) | 81 ++
   t/t7612-merge-verify-signatures.sh | 45 
   4 files changed, 132 insertions(+)

It helps both condensing information and saving some text
space. What's new in diffstat is:

- A new 0644 file is shown as (new)
- A new 0755 file is shown as (new +x)
- A new symlink is shown as (new +l)
- A deleted file is shown as (gone)
- A mode change adding executable bit is shown as (mode +x)
- A mode change removing it is shown as (mode -x)

Note that --stat-with-summary does not contain all the information
--summary provides. Rewrite percentage is not shown but it could be
added later, like R50% or C20%.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/diff-options.txt|  8 
 diff.c| 42 ++-
 diff.h|  1 +
 t/t4013-diff-various.sh   |  5 +++
 ...-pretty_--root_--stat-with-summary_initial | 12 ++
 ...etty_-R_--root_--stat-with-summary_initial | 12 ++
 ...diff-tree_--stat-with-summary_initial_mode |  4 ++
 ...f-tree_-R_--stat-with-summary_initial_mode |  4 ++
 8 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 
t/t4013/diff.diff-tree_--pretty_--root_--stat-with-summary_initial
 create mode 100644 
t/t4013/diff.diff-tree_--pretty_-R_--root_--stat-with-summary_initial
 create mode 100644 t/t4013/diff.diff-tree_--stat-with-summary_initial_mode
 create mode 100644 t/t4013/diff.diff-tree_-R_--stat-with-summary_initial_mode

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c330c01ff0..595e4cd548 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -128,6 +128,14 @@ have to use `--diff-algorithm=default` option.
 These parameters can also be set individually with `--stat-width=`,
 `--stat-name-width=` and `--stat-count=`.
 
+--stat-with-summary::
+   Output a condensed summary of extended header information such
+   as file creations or deletions ("new" or "gone", optionally "+l"
+   if it's a symlink) and mode changes ("+x" or "-x" for adding
+   or removing executable bit respectively) in diffstat. The
+   information is put betwen the filename part and the graph
+   part. Implies `--stat`.
+
 --numstat::
Similar to `--stat`, but shows number of added and
deleted lines in decimal notation and pathname without
diff --git a/diff.c b/diff.c
index 9d874a670f..6bf9867388 100644
--- a/diff.c
+++ b/diff.c
@@ -2129,6 +2129,7 @@ struct diffstat_t {
char *from_name;
char *name;
char *print_name;
+   const char *comments;
unsigned is_unmerged:1;
unsigned is_binary:1;
unsigned is_renamed:1;
@@ -2205,6 +2206,9 @@ static void fill_print_name(struct diffstat_file *file)
else
quote_c_style(file->name, , NULL, 0);
 
+   if (file->comments)
+   strbuf_addf(, " (%s)", file->comments);
+
file->print_name = strbuf_detach(, NULL);
 }
 
@@ -3239,6 +3243,32 @@ static void builtin_diff(const char *name_a,
return;
 }
 
+static char *get_compact_summary(const struct diff_filepair *p, int is_renamed)
+{
+   if (!is_renamed) {
+   if (p->status == DIFF_STATUS_ADDED) {
+   if (S_ISLNK(p->two->mode))
+   return "new +l";
+   else if ((p->two->mode & 0777) == 0755)
+   return "new +x";
+   else
+   return "new";
+   } else if (p->status == DIFF_STATUS_DELETED)
+   return "gone";
+   }
+   if (S_ISLNK(p->one->mode) && !S_ISLNK(p->two->mode))
+   return "mode -l";
+   else if (!S_ISLNK(p->one->mode) && S_ISLNK(p->two->mode))
+   return "mode +l";
+   else if ((p->one->mode & 0777) == 0644 &&
+(p->two->mode & 0777) == 0755)
+   return "mode +x";
+   else if ((p->one->mode & 0777) == 0755 &&
+(p->two->mode & 0777) == 0644)
+   return "mode -x";
+   return NULL;
+}
+
 static void builtin_diffstat(const char *name_a, const char *name_b,
 struct 

[PATCH v3 1/2] diff.c: refactor pprint_rename() to use strbuf

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

Instead of passing char* around, let function handle strbuf
directly. All callers already use strbuf internally.

This helps kill the "not free" exception in free_diffstat_info(). I
don't think this code is so critical that we need to avoid some free()
calls.

The other benefit comes in the next patch, where we append something
in pname before returning from fill_print_name(). With strbuf, it's
very simple. With "char *" we may have to resort to explicit
reallocation and stuff.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 diff.c | 59 ++
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/diff.c b/diff.c
index 0a9a0cdf18..9d874a670f 100644
--- a/diff.c
+++ b/diff.c
@@ -2045,11 +2045,10 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
}
 }
 
-static char *pprint_rename(const char *a, const char *b)
+static void pprint_rename(struct strbuf *name, const char *a, const char *b)
 {
const char *old = a;
const char *new = b;
-   struct strbuf name = STRBUF_INIT;
int pfx_length, sfx_length;
int pfx_adjust_for_slash;
int len_a = strlen(a);
@@ -2059,10 +2058,10 @@ static char *pprint_rename(const char *a, const char *b)
int qlen_b = quote_c_style(b, NULL, NULL, 0);
 
if (qlen_a || qlen_b) {
-   quote_c_style(a, , NULL, 0);
-   strbuf_addstr(, " => ");
-   quote_c_style(b, , NULL, 0);
-   return strbuf_detach(, NULL);
+   quote_c_style(a, name, NULL, 0);
+   strbuf_addstr(name, " => ");
+   quote_c_style(b, name, NULL, 0);
+   return;
}
 
/* Find common prefix */
@@ -2109,19 +2108,18 @@ static char *pprint_rename(const char *a, const char *b)
if (b_midlen < 0)
b_midlen = 0;
 
-   strbuf_grow(, pfx_length + a_midlen + b_midlen + sfx_length + 7);
+   strbuf_grow(name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
if (pfx_length + sfx_length) {
-   strbuf_add(, a, pfx_length);
-   strbuf_addch(, '{');
+   strbuf_add(name, a, pfx_length);
+   strbuf_addch(name, '{');
}
-   strbuf_add(, a + pfx_length, a_midlen);
-   strbuf_addstr(, " => ");
-   strbuf_add(, b + pfx_length, b_midlen);
+   strbuf_add(name, a + pfx_length, a_midlen);
+   strbuf_addstr(name, " => ");
+   strbuf_add(name, b + pfx_length, b_midlen);
if (pfx_length + sfx_length) {
-   strbuf_addch(, '}');
-   strbuf_add(, a + len_a - sfx_length, sfx_length);
+   strbuf_addch(name, '}');
+   strbuf_add(name, a + len_a - sfx_length, sfx_length);
}
-   return strbuf_detach(, NULL);
 }
 
 struct diffstat_t {
@@ -2197,23 +2195,17 @@ static void show_graph(struct strbuf *out, char ch, int 
cnt,
 
 static void fill_print_name(struct diffstat_file *file)
 {
-   char *pname;
+   struct strbuf pname = STRBUF_INIT;
 
if (file->print_name)
return;
 
-   if (!file->is_renamed) {
-   struct strbuf buf = STRBUF_INIT;
-   if (quote_c_style(file->name, , NULL, 0)) {
-   pname = strbuf_detach(, NULL);
-   } else {
-   pname = file->name;
-   strbuf_release();
-   }
-   } else {
-   pname = pprint_rename(file->from_name, file->name);
-   }
-   file->print_name = pname;
+   if (file->is_renamed)
+   pprint_rename(, file->from_name, file->name);
+   else
+   quote_c_style(file->name, , NULL, 0);
+
+   file->print_name = strbuf_detach(, NULL);
 }
 
 static void print_stat_summary_inserts_deletes(struct diff_options *options,
@@ -2797,8 +2789,7 @@ static void free_diffstat_info(struct diffstat_t 
*diffstat)
int i;
for (i = 0; i < diffstat->nr; i++) {
struct diffstat_file *f = diffstat->files[i];
-   if (f->name != f->print_name)
-   free(f->print_name);
+   free(f->print_name);
free(f->name);
free(f->from_name);
free(f);
@@ -5241,10 +5232,12 @@ static void show_rename_copy(struct diff_options *opt, 
const char *renamecopy,
struct diff_filepair *p)
 {
struct strbuf sb = STRBUF_INIT;
-   char *names = pprint_rename(p->one->path, p->two->path);
+   struct strbuf names = STRBUF_INIT;
+
+   pprint_rename(, p->one->path, p->two->path);
strbuf_addf(, " %s %s (%d%%)\n",
-   renamecopy, names, similarity_index(p));
-   free(names);
+   renamecopy, names.buf, similarity_index(p));
+   strbuf_release();
emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
 sb.buf, sb.len, 0);
  

[PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

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

Changes since v2 [1]:

- goes back to my original version (yay!) where the extra info
  is appended after the path name. More is described in 2/2
- --compact-summary is now renamed --stat-with-summary and implies
  --stat
- 1/2 is just a cleanup patch to make it easier to add 2/2

[1] https://public-inbox.org/git/20180118100546.32251-1-pclo...@gmail.com/

Nguyễn Thái Ngọc Duy (2):
  diff.c: refactor pprint_rename() to use strbuf
  diff: add --stat-with-summary

 Documentation/diff-options.txt |   8 ++
 diff.c | 101 ++---
 diff.h |   1 +
 t/t4013-diff-various.sh|   5 +
 ...pretty_--root_--stat-with-summary_initial (new) |  12 +++
 ...tty_-R_--root_--stat-with-summary_initial (new) |  12 +++
 ...iff-tree_--stat-with-summary_initial_mode (new) |   4 +
 ...-tree_-R_--stat-with-summary_initial_mode (new) |   4 +
 8 files changed, 113 insertions(+), 34 deletions(-)

-- 
2.16.1.75.ga05eb4

[PATCH v3 1/2] format-patch: keep cover-letter diffstat wrapped in 72 columns

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

We already wrap shortlog around 72 columns in cover letters. Do the same
for diffstat (also in cover letters).

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

diff --git a/builtin/log.c b/builtin/log.c
index 46b4ca13e5..96af897403 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -29,6 +29,8 @@
 #include "gpg-interface.h"
 #include "progress.h"
 
+#define MAIL_DEFAULT_WRAP 72
+
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
 
@@ -1044,7 +1046,7 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
 
shortlog_init();
log.wrap_lines = 1;
-   log.wrap = 72;
+   log.wrap = MAIL_DEFAULT_WRAP;
log.in1 = 2;
log.in2 = 4;
log.file = rev->diffopt.file;
@@ -1061,6 +1063,7 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
 
memcpy(, >diffopt, sizeof(opts));
opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
+   opts.stat_width = MAIL_DEFAULT_WRAP;
 
diff_setup_done();
 
-- 
2.16.1.205.g271f633410

[PATCH v3 2/2] format-patch: reduce patch diffstat width to 72

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

Patches generated by format-patch are meant to be exchanged as emails,
most of the time. And since it's generally agreed that text in mails
should be wrapped around 70 columns or so, make sure these diffstat
follow the convention (especially when used with --cover-letter since we
already defaults to wrapping 72 columns). The default can still be
overriden with command line options.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/log.c  |  2 ++
 t/t4052-stat-output.sh | 46 --
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 96af897403..94ee177d56 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1617,6 +1617,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
(!rev.diffopt.output_format ||
 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | 
DIFF_FORMAT_SUMMARY;
+   if (!rev.diffopt.stat_width)
+   rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
 
/* Always generate a patch */
rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 9f563db20a..6e2cf933f7 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -19,17 +19,33 @@ test_expect_success 'preparation' '
git commit -m message "$name"
 '
 
+cat >expect72 <<-'EOF'
+ ...a | 1 +
+EOF
+test_expect_success "format-patch: small change with long name gives more 
space to the name" '
+   git format-patch -1 --stdout >output &&
+   grep " | " output >actual &&
+   test_cmp expect72 actual
+'
+
 while read cmd args
 do
-   cat >expect <<-'EOF'
+   cat >expect80 <<-'EOF'
 
...a | 1 +
EOF
test_expect_success "$cmd: small change with long name gives more space 
to the name" '
git $cmd $args >output &&
grep " | " output >actual &&
-   test_cmp expect actual
+   test_cmp expect80 actual
'
+done <<\EOF
+diff HEAD^ HEAD --stat
+show --stat
+log -1 --stat
+EOF
 
+while read cmd args
+do
cat >expect <<-'EOF'
 ...a | 1 +
EOF
@@ -79,11 +95,11 @@ test_expect_success 'preparation for big change tests' '
git commit -m message abcd
 '
 
-cat >expect80 <<'EOF'
- abcd | 1000 ++
+cat >expect72 <<'EOF'
+ abcd | 1000 ++
 EOF
-cat >expect80-graph <<'EOF'
-|  abcd | 1000 
++
+cat >expect72-graph <<'EOF'
+|  abcd | 1000 ++
 EOF
 cat >expect200 <<'EOF'
  abcd | 1000 
++
@@ -107,7 +123,7 @@ do
test_cmp "$expect-graph" actual
'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
 respects expect200 show --stat
 respects expect200 log -1 --stat
@@ -135,7 +151,7 @@ do
test_cmp "$expect-graph" actual
'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
 respects expect40 show --stat
 respects expect40 log -1 --stat
@@ -163,7 +179,7 @@ do
test_cmp "$expect-graph" actual
'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
 respects expect40 show --stat
 respects expect40 log -1 --stat
@@ -250,11 +266,11 @@ show --stat
 log -1 --stat
 EOF
 
-cat >expect80 <<'EOF'
- ...aaa | 1000 
+cat >expect72 <<'EOF'
+ ...aa | 1000 +
 EOF
-cat >expect80-graph <<'EOF'
-|  ...aaa | 1000 

+cat >expect72-graph <<'EOF'
+|  ...aa | 1000 +
 EOF
 cat >expect200 <<'EOF'
  aaa | 1000 
+++
@@ -278,7 +294,7 @@ do
test_cmp "$expect-graph" actual
'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
 

[PATCH v3 0/2] wrap format-patch diffstats around 72 columns

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

v3 fixes tests in 2/2 that I overlooked (but Jeff didn't). Interdiff:

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 1e62333b46..6e2cf933f7 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -19,17 +19,33 @@ test_expect_success 'preparation' '
git commit -m message "$name"
 '
 
+cat >expect72 <<-'EOF'
+ ...a | 1 +
+EOF
+test_expect_success "format-patch: small change with long name gives more 
space to the name" '
+   git format-patch -1 --stdout >output &&
+   grep " | " output >actual &&
+   test_cmp expect72 actual
+'
+
 while read cmd args
 do
-   cat >expect <<-'EOF'
+   cat >expect80 <<-'EOF'
 
...a | 1 +
EOF
test_expect_success "$cmd: small change with long name gives more space 
to the name" '
git $cmd $args >output &&
grep " | " output >actual &&
-   test_cmp expect actual
+   test_cmp expect80 actual
'
+done <<\EOF
+diff HEAD^ HEAD --stat
+show --stat
+log -1 --stat
+EOF
 
+while read cmd args
+do
cat >expect <<-'EOF'
 ...a | 1 +
EOF
@@ -60,7 +76,7 @@ do
test_cmp expect actual
'
 done <<\EOF
-format-patch --stat=80 -1 --stdout
+format-patch -1 --stdout
 diff HEAD^ HEAD --stat
 show --stat
 log -1 --stat


Nguyễn Thái Ngọc Duy (2):
  format-patch: keep cover-letter diffstat wrapped in 72 columns
  format-patch: reduce patch diffstat width to 72

 builtin/log.c  |  7 ++-
 t/t4052-stat-output.sh | 46 --
 2 files changed, 37 insertions(+), 16 deletions(-)

-- 
2.16.1.205.g271f633410

Re: What's cooking in git.git (Jan 2018, #04; Wed, 31)

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 01 2018, Junio C. Hamano jotted:

> * nd/fix-untracked-cache-invalidation (2018-01-24) 5 commits
>  - dir.c: stop ignoring opendir() error in open_cached_dir()
>  - update-index doc: note a fixed bug in the untracked cache
>  - dir.c: fix missing dir invalidation in untracked code
>  - dir.c: avoid stat() in valid_cached_dir()
>  - status: add a failing test showing a core.untrackedCache bug
>
>  Some bugs around "untracked cache" feature have been fixed.
>
>  Will merge to 'next'.

The "update-index doc: note a fixed bug in the untracked cache" needs to
be amended so it doesn't say "Before 2.16, ":


https://github.com/gitster/git/commit/b9fc38c9f90b8bf2c9147ad536813b66aa45220d#diff-01fe1588047a177245bfaf82336cdeaeR467

I'm not sure whether you're planning this for 2.16.2, or whether it'll
be in 2.17.0, but the patch should be amended to mention either one of
those.

I can submit a replacement patch, but figured this was trivial enough
(and you know more about the release plan) that you'd like to amend this
locally.

Re: [PATCH v2 3/3] rebase: introduce and use pseudo-ref ORIG_COMMIT

[Phillip Wood]

On 31/01/18 09:30, Nguyễn Thái Ngọc Duy wrote:
> 
> The new command `git rebase --show-current-patch` is useful for seeing
> the commit related to the current rebase state. Some however may find
> the "git show" command behind it too limiting. You may want to
> increase context lines, do a diff that ignores whitespaces...
> 
> For these advanced use cases, the user can execute any command they
> want with the new pseudo ref ORIG_COMMIT.
> 
> This also helps show where the stopped commit is from, which is hard
> to see from the previous patch which implements --show-current-patch.
> 
> Helped-by: Tim Landscheidt 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/git-rebase.txt   | 3 ++-
>  builtin/am.c   | 4 
>  contrib/completion/git-completion.bash | 2 +-
>  git-rebase--interactive.sh | 5 -
>  git-rebase--merge.sh   | 4 +++-
>  git-rebase.sh  | 1 +
>  sequencer.c| 3 +++
>  t/t3400-rebase.sh  | 3 ++-
>  t/t3404-rebase-interactive.sh  | 5 -
>  9 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 7ef9577472..6da9296bf8 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -252,7 +252,8 @@ leave out at most one of A and B, in which case it 
> defaults to HEAD.
>  
>  --show-current-patch::
>   Show the current patch in an interactive rebase or when rebase
> - is stopped because of conflicts.
> + is stopped because of conflicts. This is the equivalent of
> + `git show ORIG_COMMIT`.
>  
>  -m::
>  --merge::
> diff --git a/builtin/am.c b/builtin/am.c
> index caec50cba9..bf9b356340 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1011,6 +1011,7 @@ static void am_setup(struct am_state *state, enum 
> patch_format patch_format,
>  
>   if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
>   die_errno(_("failed to create directory '%s'"), state->dir);
> + delete_ref(NULL, "ORIG_COMMIT", NULL, REF_NO_DEREF);
>  
>   if (split_mail(state, patch_format, paths, keep_cr) < 0) {
>   am_destroy(state);
> @@ -1110,6 +,7 @@ static void am_next(struct am_state *state)
>  
>   oidclr(>orig_commit);
>   unlink(am_path(state, "original-commit"));
> + delete_ref(NULL, "ORIG_COMMIT", NULL, REF_NO_DEREF);
>  
>   if (!get_oid("HEAD", ))
>   write_state_text(state, "abort-safety", oid_to_hex());
> @@ -1441,6 +1443,8 @@ static int parse_mail_rebase(struct am_state *state, 
> const char *mail)
>  
>   oidcpy(>orig_commit, _oid);
>   write_state_text(state, "original-commit", oid_to_hex(_oid));
> + update_ref("am", "ORIG_COMMIT", _oid,
> +NULL, 0, UPDATE_REFS_DIE_ON_ERR);
>  
>   return 0;
>  }
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 2bd30d68cf..deea688e0e 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -439,7 +439,7 @@ __git_refs ()
>   track=""
>   ;;
>   *)
> - for i in HEAD FETCH_HEAD ORIG_HEAD MERGE_HEAD; do
> + for i in HEAD FETCH_HEAD ORIG_HEAD MERGE_HEAD 
> ORIG_COMMIT; do
>   case "$i" in
>   $match*)
>   if [ -e "$dir/$i" ]; then
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0c0f8abbf9..ef72bd5871 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -199,12 +199,14 @@ make_patch () {
>  
>  die_with_patch () {
>   echo "$1" > "$state_dir"/stopped-sha
> + git update-ref ORIG_COMMIT "$1"
>   make_patch "$1"
>   die "$2"
>  }
>  
>  exit_with_patch () {
>   echo "$1" > "$state_dir"/stopped-sha
> + git update-ref ORIG_COMMIT "$1"
>   make_patch $1
>   git rev-parse --verify HEAD > "$amend"
>   gpg_sign_opt_quoted=${gpg_sign_opt:+$(git rev-parse --sq-quote 
> "$gpg_sign_opt")}
> @@ -841,7 +843,7 @@ To continue rebase after editing, run:
>   exit
>   ;;
>  show-current-patch)
> - exec git show "$(cat "$state_dir/stopped-sha")" --
> + exec git show ORIG_COMMIT --
>   ;;
>  esac
>  
> @@ -858,6 +860,7 @@ fi
>  
>  orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
>  mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary 
> \$state_dir")"
> +rm -f "$(git rev-parse --git-path ORIG_COMMIT)"
>  
>  : > "$state_dir"/interactive || die "$(gettext "Could not mark as 
> interactive")"
>  write_basic_state
> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> index 0a96dfae37..70966c32c3 100644
> --- a/git-rebase--merge.sh
> +++ 

Re: What's cooking in git.git (Jan 2018, #04; Wed, 31)

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 01 2018, Junio C. Hamano jotted:

> * ab/wildmatch-tests (2018-01-30) 10 commits
>  - wildmatch test: mark test as EXPENSIVE_ON_WINDOWS
>  - test-lib: add an EXPENSIVE_ON_WINDOWS prerequisite
>  - wildmatch test: create & test files on disk in addition to in-memory
>  - wildmatch test: perform all tests under all wildmatch() modes
>  - wildmatch test: use test_must_fail, not ! for test-wildmatch
>  - wildmatch test: remove dead fnmatch() test code
>  - wildmatch test: use a paranoia pattern from nul_match()
>  - wildmatch test: don't try to vertically align our output
>  - wildmatch test: use more standard shell style
>  - wildmatch test: indent with tabs, not spaces
>
>  More tests for wildmatch functions.
>
>  Expecting an update.
>  cf. <87vaga9mgf@evledraar.gmail.com>

The 2018-01-30 series is the update mentioned in
87vaga9mgf@evledraar.gmail.com. You probably noticed this / just
didn't adjust the note since you queued in in pu already, but just in
case: the known issues in it have been resolved, but hopefully Johannes
Schindelin can test it on Windows & report.

> * nd/fix-untracked-cache-invalidation (2018-01-24) 5 commits
>  - dir.c: stop ignoring opendir() error in open_cached_dir()
>  - update-index doc: note a fixed bug in the untracked cache
>  - dir.c: fix missing dir invalidation in untracked code
>  - dir.c: avoid stat() in valid_cached_dir()
>  - status: add a failing test showing a core.untrackedCache bug
>
>  Some bugs around "untracked cache" feature have been fixed.
>
>  Will merge to 'next'.

It must be Murphy's law or something, but even though the bug has been
there for years I just had some internal users again run into the bug
this fixes, so I'm building an internal v2.16.1 + custom patches (this
included).

> * ab/sha1dc-build (2017-12-12) 4 commits
>  . Makefile: use the sha1collisiondetection submodule by default
>  - sha1dc_git.h: re-arrange an ifdef chain for a subsequent change
>  - Makefile: under "make dist", include the sha1collisiondetection submodule
>  - Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto
>
>  Push the submodule version of collision-detecting SHA-1 hash
>  implementation a bit harder on builders.
>
>  The earlier two may make sense, but leaning toward rejecting the last step.
>  cf. 

This has been lingering for a long time. I think it makes sense just to
merge the first 3 down and then we can discuss 4/4 in another
submission. [12]/4 solve real bugs we have now, 3/4 is harmless to merge
down (and makes 4/4 smaller when we get around to discussing it again),
it's just 4/4 that's been stalling this.

Do you want to peel of 4/4 and just keep 1-3 should I submit another
version without 4/4?

Re: [PATCH] tag: add --edit option

[Eric Sunshine]

On Thu, Feb 1, 2018 at 5:43 AM, Nicolas Morey-Chaisemartin
 wrote:
> Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
>>> A little below this change is where launch_editor() is actually
>>> invoked. If it fails for some reason, it prints:
>>>
>>> Please supply the message using either -m or -F option.
>>>
>>> which seems a bit counterintuitive if the user *did* specify one of
>>> those options along with --edit. I wonder if that message needs to be
>>> adjusted.
>>>
>> Yes I'll fix this.
> I just checked what commit.c does and it seems to behave as my patch:
> if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
> fprintf(stderr,
> _("Please supply the message using either -m or -F option.\n"));
>
> To be honest the message is not that clear either.
> If I'm reading launch_editor right most (or all) its falire are du to a 
> failure to launch the editor or the editor crashed/exited with an error.
> In this case, I wouldn't advise the user to use -m or -F but to fix its 
> editor.

Indeed, I also looked at the implementation of launch_editor(), and my
"wondering" about whether the message needed adjustment was just that.
The message seems somewhat counterintuitive in this case, but I didn't
necessarily have a better suggestion. A valid response, therefore,
might be to punt on it and leave that change for the future, or
perhaps take it on as a second patch which adjusts the message in both
commands. I don't have strong feelings about it at this time.

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>
> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
>> On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
>>  wrote:
>>> Add a --edit option whichs allows modifying the messages provided by -m or 
>>> -F,
>>> the same way git commit --edit does.
>>>
>>> Signed-off-by: Nicolas Morey-Chaisemartin 
>>> ---
>>> diff --git a/builtin/tag.c b/builtin/tag.c
>>> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
>>> const char *tag,
>>> -   if (!opt->message_given) {
>>> +   if (!opt->message_given || opt->use_editor) {
>>>
>>> -   if (!is_null_oid(prev)) {
>>> +   if (opt->message_given) {
>>> +   write_or_die(fd, buf->buf, buf->len);
>>> +   strbuf_reset(buf);
>>> +   } else if (!is_null_oid(prev)) {
>>> write_tag_body(fd, prev);
>>> } else {
>> A little below this change is where launch_editor() is actually
>> invoked. If it fails for some reason, it prints:
>>
>> Please supply the message using either -m or -F option.
>>
>> which seems a bit counterintuitive if the user *did* specify one of
>> those options along with --edit. I wonder if that message needs to be
>> adjusted.
>>
> Yes I'll fix this.
I just checked what commit.c does and it seems to behave as my patch:
        if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
            fprintf(stderr,
            _("Please supply the message using either -m or -F option.\n"));
            exit(1);
        }


To be honest the message is not that clear either.
If I'm reading launch_editor right most (or all) its falire are du to a failure 
to launch the editor or the editor crashed/exited with an error.
In this case, I wouldn't advise the user to use -m or -F but to fix its editor.

Nicolas

Re: Some rough edges of core.fsmonitor

[Duy Nguyen]

On Tue, Jan 30, 2018 at 6:16 AM, Ben Peart  wrote:
>
>
> On 1/29/2018 4:40 AM, Duy Nguyen wrote:
>>
>> On Sat, Jan 27, 2018 at 12:43:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> b) with fsmonitor
>>>
>>>  $ time GIT_TRACE_PERFORMANCE=1 ~/g/git/git-status
>>>  12:34:23.833625 read-cache.c:1890   performance: 0.049485685 s:
>>> read cache .git/index
>>
>>
>> This is sort of off topic but may be interesting for big repo guys. It
>> looks like read cache's time is partially dominated by malloc().
>>
>
> That is correct.  We have tried a few different ways to address this. First
> was my patch series [1] that would parallelize all of the read cache code.
>
> We quickly found that malloc() was the biggest culprit and by speeding that
> up, we got most of the wins.  At Peff's recommendation [2], we looked into
> using tcmalloc but found that 1) it has bugs on Windows and 2) it isn't
> being actively maintained so it didn't seem those bugs would ever get fixed.
>
> We are currently working on a patch that will use a refactored version of
> the mem_pool in fast-import.c to do block allocations of the cache entries
> which is giving us about a 22% improvement in "git status" times.  One

My apologies if this has been discussed in the second half of 2017
which I have no idea what happened.

I just wonder if it's possible to design a "file format" that is
basically a memory dump of lots of struct cache_entry? This "file"
will stay in a shared memory somewhere and never get written down to
disk. Since it's very close to the data structure we have in core, the
most we need to do after mmap'ing it (and keeping it mmap'd until the
end) is adjust some pointers. These entries are of course read-only.
When you modify/create new entries, they are created new, using the
old malloc(). We just need to make sure not free the read-only
cache_entry entries and munmap() the whole thing when we
discard_index().

This opens up another option to deal with the large UNTR and TREE
extensions in a similar way. These will be your next headache after
you have reduced parse time for main entries.

> challenge has been ensuring that cache entries are not passed from one
> index/mem_pool to another which could cause access after free bugs.

We kind of have something close to that, but not entirely the same.
When split index is used, the same cache_entry can appear in two
index_state structs. Of course you can free only one of them (and you
can only do so when you know both index_state are gone). I see some
code cleanup opportunity :)

>
> [1]
> https://public-inbox.org/git/20171109141737.47976-1-benpe...@microsoft.com/
> [2]
> https://public-inbox.org/git/20171120153846.v5b7ho42yzrzn...@sigill.intra.peff.net/
-- 
Duy

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
>  wrote:
>> Add a --edit option whichs allows modifying the messages provided by -m or 
>> -F,
>> the same way git commit --edit does.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
>> const char *tag,
>> -   if (!opt->message_given) {
>> +   if (!opt->message_given || opt->use_editor) {
>>
>> -   if (!is_null_oid(prev)) {
>> +   if (opt->message_given) {
>> +   write_or_die(fd, buf->buf, buf->len);
>> +   strbuf_reset(buf);
>> +   } else if (!is_null_oid(prev)) {
>> write_tag_body(fd, prev);
>> } else {
> A little below this change is where launch_editor() is actually
> invoked. If it fails for some reason, it prints:
>
> Please supply the message using either -m or -F option.
>
> which seems a bit counterintuitive if the user *did* specify one of
> those options along with --edit. I wonder if that message needs to be
> adjusted.
>
Yes I'll fix this.

>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -452,6 +452,23 @@ test_expect_success \
>> +get_tag_header annotated-tag-edit $commit commit $time >expect
>> +echo "An edited message" >>expect
>> +test_expect_success 'set up editor' '
>> +   cat >editor <<-\EOF &&
>> +   #!/bin/sh
>> +   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
>> +   mv "$1-" "$1"
>> +   EOF
>> +   chmod 755 editor
> If you use write_script() to create the fake editor, then it supplies
> the #!/bin/sh line for you and does the 'chmod', so you only need to
> supply the actual script payload. Also, other "editors" in this test
> file are named "fakeeditor", so perhaps follow suit.
>
> write_script fakeeditor <<-\EOF
> sed -e "s/A message/An edited message/g" <"$1" >"$1-"
> mv "$1-" "$1"
> EOF
>
I dumbly copied the test from commit --edit as it was my reference.
I'll fix the names and switch to write_script.

Thanks

Nicolas

Re: [PATCH v2 01/41] parse-options: support --git-completion-helper

[Duy Nguyen]

On Thu, Feb 1, 2018 at 4:54 PM, Eric Sunshine  wrote:
> On Wed, Jan 31, 2018 at 7:05 PM, Duy Nguyen  wrote:
>> On Thu, Feb 1, 2018 at 4:04 AM, Eric Sunshine  
>> wrote:
>>> On Wed, Jan 31, 2018 at 6:05 AM, Nguyễn Thái Ngọc Duy  
>>> wrote:
 Dangerous/Unpopular
 options could be hidden with the new "NOCOMPLETE" flag.
>>>
>>> I wonder if this option should be named DANGEROUS rather than
>>> NOCOMPLETE to better reflect its intention.
>>
>> It's not only for dangerous options (I forgot to mention this in the
>> commit message, I will in v3). The --continue|--abort|--skip should
>> only show up when you are in a middle of rebase/am/cherry-pick.
>> git-completion.bash handles this case separately and only put them in
>> the completion list  when appropriate. --git-completion-helper must
>> not include these or the trick done by git-completion.bash becomes
>> useless.
>>
>> Interesting. So we now have two classes of "no complete". One can't be
>> configurable (--continue|--abort|--skip) and one can. I'll use two
>> separate flags for these, though I'm not adding the configuration
>> option right now.
>
> I don't see that as convincing argument for two classes of "no
> complete". Since git-completion.bash already special-cases
> rebase/am/cherry-pick for --continue|--abort|--skip, it is not far
> fetched that that special-case treatment can be extended slightly to
> also filter out those three options from the list returned by
> --git-completion-helper.

I agree that is possible, but it's a bit tricky to do the filtering
right in bash (all options are sent back as one line instead of one
per line, which is easier to process by command line tools).

On top of  that, I think we want git-completion.bash to be fast, the
more commands we execute there, the unhappier Windows users are. It is
too possible to do this kind of filtering just once, before caching.
It does not fit well to how I designed __gitcomp_builtin so I need to
sit back and see how to sort that out.

Long term though, I think we would want more and more completion logic
built in. One of those things I have in mind is to let
--git-completion-helper (or some other new option) to provide
completion for possible option values (e.g. true/false, values for
"git merge --strategy=", "git status --untrack-files="). That may
also include completion of --continue|--abort|.. in C code, something
like this roughly

# separate command blocks because we still need to cache them in shell
if [ -f .git/rebase-apply ]; then
__gitcomp $(git $cmd --git-completion-helper=in-progress)
else
__gitcomp $(git $cmd --git-completion-helper)
fi

which means eventually we have to flag these options differently.

Having said all that, these are the things I imagine we _might_ do. I
have not really thought it through and nor do I have a clear plan
forward at this point, so they may end up being just rubbish thoughts.

> So, if that special case is handled entirely by the completion script,
> then that leaves only the "dangerous" options, which requires only a
> single flag.
-- 
Duy

Re: [PATCH] tag: add --edit option

[Eric Sunshine]

On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
 wrote:
> Add a --edit option whichs allows modifying the messages provided by -m or -F,
> the same way git commit --edit does.
>
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
> const char *tag,
> -   if (!opt->message_given) {
> +   if (!opt->message_given || opt->use_editor) {
>
> -   if (!is_null_oid(prev)) {
> +   if (opt->message_given) {
> +   write_or_die(fd, buf->buf, buf->len);
> +   strbuf_reset(buf);
> +   } else if (!is_null_oid(prev)) {
> write_tag_body(fd, prev);
> } else {

A little below this change is where launch_editor() is actually
invoked. If it fails for some reason, it prints:

Please supply the message using either -m or -F option.

which seems a bit counterintuitive if the user *did* specify one of
those options along with --edit. I wonder if that message needs to be
adjusted.

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -452,6 +452,23 @@ test_expect_success \
> +get_tag_header annotated-tag-edit $commit commit $time >expect
> +echo "An edited message" >>expect
> +test_expect_success 'set up editor' '
> +   cat >editor <<-\EOF &&
> +   #!/bin/sh
> +   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
> +   mv "$1-" "$1"
> +   EOF
> +   chmod 755 editor

If you use write_script() to create the fake editor, then it supplies
the #!/bin/sh line for you and does the 'chmod', so you only need to
supply the actual script payload. Also, other "editors" in this test
file are named "fakeeditor", so perhaps follow suit.

write_script fakeeditor <<-\EOF
sed -e "s/A message/An edited message/g" <"$1" >"$1-"
mv "$1-" "$1"
EOF

[PATCH v2 3/3] perf/aggregate: sort JSON fields in output

[Christian Couder]

It is much easier to diff the output against a previous
one when the fields are sorted.

Helped-by: Philip Oakley 
Signed-off-by: Christian Couder 
---
 t/perf/aggregate.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The only change compared to v1 is a typo fix suggested by Philip in
the commti message.

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index a609292491..749d6689f9 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -253,7 +253,7 @@ sub print_codespeed_results {
}
}
 
-   print to_json(\@data, {utf8 => 1, pretty => 1}), "\n";
+   print to_json(\@data, {utf8 => 1, pretty => 1, canonical => 1}), "\n";
 }
 
 binmode STDOUT, ":utf8" or die "PANIC on binmode: $!";
-- 
2.16.0.rc2.45.g09a1bbd803

[PATCH v2 2/3] perf/aggregate: add --reponame option

[Christian Couder]

This makes it easier to use the aggregate script
on the command line when one wants to get the
"environment" fields set in the codespeed output.

Previously setting GIT_REPO_NAME was needed
for this purpose.

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 t/perf/aggregate.perl | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

The only change compared to v1 is a logical change suggested by Eric
in the 'if ... elsif ... else ...' sequence that sets $environment.

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index bbf0f30898..a609292491 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -37,7 +37,7 @@ sub format_times {
 }
 
 my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
-$codespeed, $subsection);
+$codespeed, $subsection, $reponame);
 while (scalar @ARGV) {
my $arg = $ARGV[0];
my $dir;
@@ -55,6 +55,15 @@ while (scalar @ARGV) {
}
next;
}
+   if ($arg eq "--reponame") {
+   shift @ARGV;
+   $reponame = $ARGV[0];
+   shift @ARGV;
+   if (! $reponame) {
+   die "empty reponame";
+   }
+   next;
+   }
last if -f $arg or $arg eq "--";
if (! -d $arg) {
my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
@@ -210,7 +219,9 @@ sub print_codespeed_results {
}
 
my $environment;
-   if (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} ne "") 
{
+   if ($reponame) {
+   $environment = $reponame;
+   } elsif (exists $ENV{GIT_PERF_REPO_NAME} and $ENV{GIT_PERF_REPO_NAME} 
ne "") {
$environment = $ENV{GIT_PERF_REPO_NAME};
} elsif (exists $ENV{GIT_TEST_INSTALLED} and $ENV{GIT_TEST_INSTALLED} 
ne "") {
$environment = $ENV{GIT_TEST_INSTALLED};
-- 
2.16.0.rc2.45.g09a1bbd803

[PATCH v2 1/3] perf/aggregate: add --subsection option

[Christian Couder]

This makes it easier to use the aggregate script
on the command line, to get results from
subsections.

Previously setting GIT_PERF_SUBSECTION was needed
for this purpose.

Signed-off-by: Christian Couder 
---
 t/perf/aggregate.perl | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

This small patch series contains a few small Codespeed related
usability improvements of the perf/aggregate.perl script on top the
cc/codespeed patch series that was recently merged into master.

Changes compared to v1 are described in each patch if any.

The discussion thread about v1 is there:  

https://public-inbox.org/git/20180128111843.2690-1-chrisc...@tuxfamily.org/T/#u

diff --git a/t/perf/aggregate.perl b/t/perf/aggregate.perl
index 5c439f6bc2..bbf0f30898 100755
--- a/t/perf/aggregate.perl
+++ b/t/perf/aggregate.perl
@@ -36,7 +36,8 @@ sub format_times {
return $out;
 }
 
-my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests, $codespeed);
+my (@dirs, %dirnames, %dirabbrevs, %prefixes, @tests,
+$codespeed, $subsection);
 while (scalar @ARGV) {
my $arg = $ARGV[0];
my $dir;
@@ -45,6 +46,15 @@ while (scalar @ARGV) {
shift @ARGV;
next;
}
+   if ($arg eq "--subsection") {
+   shift @ARGV;
+   $subsection = $ARGV[0];
+   shift @ARGV;
+   if (! $subsection) {
+   die "empty subsection";
+   }
+   next;
+   }
last if -f $arg or $arg eq "--";
if (! -d $arg) {
my $rev = Git::command_oneline(qw(rev-parse --verify), $arg);
@@ -76,10 +86,15 @@ if (not @tests) {
 }
 
 my $resultsdir = "test-results";
-my $results_section = "";
-if (exists $ENV{GIT_PERF_SUBSECTION} and $ENV{GIT_PERF_SUBSECTION} ne "") {
-   $resultsdir .= "/" . $ENV{GIT_PERF_SUBSECTION};
-   $results_section = $ENV{GIT_PERF_SUBSECTION};
+
+if (! $subsection and
+exists $ENV{GIT_PERF_SUBSECTION} and
+$ENV{GIT_PERF_SUBSECTION} ne "") {
+   $subsection = $ENV{GIT_PERF_SUBSECTION};
+}
+
+if ($subsection) {
+   $resultsdir .= "/" . $subsection;
 }
 
 my @subtests;
@@ -183,15 +198,15 @@ sub print_default_results {
 }
 
 sub print_codespeed_results {
-   my ($results_section) = @_;
+   my ($subsection) = @_;
 
my $project = "Git";
 
my $executable = `uname -s -m`;
chomp $executable;
 
-   if ($results_section ne "") {
-   $executable .= ", " . $results_section;
+   if ($subsection) {
+   $executable .= ", " . $subsection;
}
 
my $environment;
@@ -233,7 +248,7 @@ sub print_codespeed_results {
 binmode STDOUT, ":utf8" or die "PANIC on binmode: $!";
 
 if ($codespeed) {
-   print_codespeed_results($results_section);
+   print_codespeed_results($subsection);
 } else {
print_default_results();
 }
-- 
2.16.0.rc2.45.g09a1bbd803

Hello My Dear Friend,

[Mrs.Zainab Ahmed]

I have a business proposal in the tune of $10.2 Million USD for you to handle 
with me. I have opportunity to transfer this abandon fund to your bank account 
in your country which belongs to our client.

I am inviting you in this transaction where this money can be shared between us 
at ratio of 60/40% and help the needy around us don’t be afraid of anything I 
am with you I will instruct you what you will do to maintain this fund.

Please kindly contact me with your information's if you are interested in this 
transaction for more details.

Your Name:..
Your Bank Name:.
Your Account Number:...
Your Telephone Number:
Your Country And Address:
Your Age And Sex:...

Thanks
Mrs.Zainab Ahmed,

Re: [PATCH v2 3/3] rebase: introduce and use pseudo-ref ORIG_COMMIT

[Duy Nguyen]

On Thu, Feb 1, 2018 at 6:18 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> The new command `git rebase --show-current-patch` is useful for seeing
>> the commit related to the current rebase state. Some however may find
>> the "git show" command behind it too limiting. You may want to
>> increase context lines, do a diff that ignores whitespaces...
>>
>> For these advanced use cases, the user can execute any command they
>> want with the new pseudo ref ORIG_COMMIT.
>>
>> This also helps show where the stopped commit is from, which is hard
>> to see from the previous patch which implements --show-current-patch.
>>
>> Helped-by: Tim Landscheidt 
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>
> Hmph, how is this new file conceptually different from existing ones
> like CHERRY_PICK_HEAD?

Conceptually the same, except that CHERRY_PICK_HEAD can't be reused
because it's specifically tied to git-cherry-pick (there's even code
that delete this ref if cherry-pick is run as part of rebase, and
git-status uses this ref to see if a cherry-pick is in progress).
There's also REVERT_HEAD in sequencer.c, same purpose but for
git-revert. Perhaps I should rename this new ref to REBASE_HEAD to
follow the same naming?
-- 
Duy

Re: [PATCH] gitignore.txt: elaborate shell glob syntax

[Duy Nguyen]

On Wed, Jan 31, 2018 at 03:22:46PM -0800, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy   writes:

Argh.. stray patches strike again. It's not supposed to be in this
thread but in

https://public-inbox.org/git/%3C20180130101351.GA761@ash%3E/

> 
> > `.gitignore` file).
> >  
> > - - Otherwise, Git treats the pattern as a shell glob suitable
> > -   for consumption by fnmatch(3) with the FNM_PATHNAME flag:
> > -   wildcards in the pattern will not match a / in the pathname.
> > -   For example, "Documentation/{asterisk}.html" matches
> > -   "Documentation/git.html" but not "Documentation/ppc/ppc.html"
> > -   or "tools/perf/Documentation/perf.html".
> > + - Otherwise, Git treats the pattern as a shell glob: '{asterisk}'
> > +   matches anything except '/', '?' matches any one character except
> > +   '/' and '[]' matches one character in a selected range. See
> > +   fnmatch(3) and the FNM_PATHNAME flag for a more accurate
> > +   description.
> 
> Where the original did not quote single letters at all, this uses a
> pair of single quotes.  Did you make sure it renders well in HTML
> and manpage already or should I do that for you before applying?

I didn't. I thought I didn't add any weird symbols. I was wrong. These
are now wrapped as "`stuff`" to be displayed the same way as in nearby
paragraphs. Verified both man and HTML pages are rendered well.

> I think what you wrote is accurate enough already, and those who
> want to go to fnmatch(3) would do so not for accuracy but for
> authority ;-) Perhaps s/accurate/detailed/?

Well there are rooms for guessing, for example "matches anything" does
not tell you straight that it can match multiple characters. Anyway
fixed too.

-- 8< --
Subject: [PATCH v2] gitignore.txt: elaborate shell glob syntax

`fnmatch(3)` is a great mention if the intended audience is
programmers. For normal users it's probably better to spell out what
a shell glob is.

This paragraph is updated to roughly tell (or remind) what the main
wildcards are supposed to do. All the details are still hidden away
behind the `fnmatch(3)` wall because bringing the whole specification
here may be too much.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/gitignore.txt | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 63260f0056..ff5d7f9ed6 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -102,12 +102,11 @@ PATTERN FORMAT
(relative to the toplevel of the work tree if not from a
`.gitignore` file).
 
- - Otherwise, Git treats the pattern as a shell glob suitable
-   for consumption by fnmatch(3) with the FNM_PATHNAME flag:
-   wildcards in the pattern will not match a / in the pathname.
-   For example, "Documentation/{asterisk}.html" matches
-   "Documentation/git.html" but not "Documentation/ppc/ppc.html"
-   or "tools/perf/Documentation/perf.html".
+ - Otherwise, Git treats the pattern as a shell glob: "`*`" matches
+   anything except "`/`", "`?`" matches any one character except "`/`"
+   and "`[]`" matches one character in a selected range. See
+   fnmatch(3) and the FNM_PATHNAME flag for a more detailed
+   description.
 
  - A leading slash matches the beginning of the pathname.
For example, "/{asterisk}.c" matches "cat-file.c" but not
-- 
2.16.1.205.g271f633410

-- 8< --

Re: [PATCH v2 01/41] parse-options: support --git-completion-helper

[Eric Sunshine]

On Wed, Jan 31, 2018 at 7:05 PM, Duy Nguyen  wrote:
> On Thu, Feb 1, 2018 at 4:04 AM, Eric Sunshine  wrote:
>> On Wed, Jan 31, 2018 at 6:05 AM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> Dangerous/Unpopular
>>> options could be hidden with the new "NOCOMPLETE" flag.
>>
>> I wonder if this option should be named DANGEROUS rather than
>> NOCOMPLETE to better reflect its intention.
>
> It's not only for dangerous options (I forgot to mention this in the
> commit message, I will in v3). The --continue|--abort|--skip should
> only show up when you are in a middle of rebase/am/cherry-pick.
> git-completion.bash handles this case separately and only put them in
> the completion list  when appropriate. --git-completion-helper must
> not include these or the trick done by git-completion.bash becomes
> useless.
>
> Interesting. So we now have two classes of "no complete". One can't be
> configurable (--continue|--abort|--skip) and one can. I'll use two
> separate flags for these, though I'm not adding the configuration
> option right now.

I don't see that as convincing argument for two classes of "no
complete". Since git-completion.bash already special-cases
rebase/am/cherry-pick for --continue|--abort|--skip, it is not far
fetched that that special-case treatment can be extended slightly to
also filter out those three options from the list returned by
--git-completion-helper.

So, if that special case is handled entirely by the completion script,
then that leaves only the "dangerous" options, which requires only a
single flag.

[PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Add a --edit option whichs allows modifying the messages provided by -m or -F,
the same way git commit --edit does.

Signed-off-by: Nicolas Morey-Chaisemartin 
---
 Documentation/git-tag.txt |  6 ++
 builtin/tag.c | 11 +--
 t/t7004-tag.sh| 34 ++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f984..b9e5a993bea0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
annotation lines.
Implies `-a` if none of `-a`, `-s`, or `-u `
is given.
 
+-e::
+--edit::
+   The message taken from file with `-F` and command line with
+   `-m` are usually used as the tag message unmodified.
+   This option lets you further edit the message taken from these sources.
+
 --cleanup=::
This option sets how the tag message is cleaned up.
The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
diff --git a/builtin/tag.c b/builtin/tag.c
index a7e6a5b0f234..91c60829d5f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
struct object_id *resu
 
 struct create_tag_options {
unsigned int message_given:1;
+   unsigned int use_editor:1;
unsigned int sign;
enum {
CLEANUP_NONE,
@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
const char *tag,
tag,
git_committer_info(IDENT_STRICT));
 
-   if (!opt->message_given) {
+   if (!opt->message_given || opt->use_editor) {
int fd;
 
/* write the template message before editing: */
@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
const char *tag,
if (fd < 0)
die_errno(_("could not create file '%s'"), path);
 
-   if (!is_null_oid(prev)) {
+   if (opt->message_given) {
+   write_or_die(fd, buf->buf, buf->len);
+   strbuf_reset(buf);
+   } else if (!is_null_oid(prev)) {
write_tag_body(fd, prev);
} else {
struct strbuf buf = STRBUF_INIT;
@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+   int edit_flag = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, , N_("n"),
@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", , N_("message"),
 N_("tag message"), parse_msg_arg),
OPT_FILENAME('F', "file", , N_("read message from 
file")),
+   OPT_BOOL('e', "edit", _flag, N_("force edit of commit")),
OPT_BOOL('s', "sign", , N_("annotated and GPG-signed 
tag")),
OPT_STRING(0, "cleanup", _arg, N_("mode"),
N_("how to strip spaces and #comments from message")),
@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("tag '%s' already exists"), tag);
 
opt.message_given = msg.given || msgfile;
+   opt.use_editor = edit_flag;
 
if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a9af2de9960b..60e3a53f297f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,23 @@ test_expect_success \
test_cmp expect actual
 '
 
+get_tag_header annotated-tag-edit $commit commit $time >expect
+echo "An edited message" >>expect
+test_expect_success 'set up editor' '
+   cat >editor <<-\EOF &&
+   #!/bin/sh
+   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+   mv "$1-" "$1"
+   EOF
+   chmod 755 editor
+'
+test_expect_success \
+   'creating an annotated tag with -m message --edit should succeed' '
+   EDITOR=./editor git tag -m "A message" --edit annotated-tag-edit &&
+   get_tag_msg annotated-tag-edit >actual &&
+   test_cmp expect actual
+'
+
 cat >msgfile >expect
+test_expect_success 'set up editor' '
+   cat >editor <<-\EOF &&
+   #!/bin/sh
+   sed -e "s/Another message/Another edited message/g" <"$1" >"$1-"
+   mv "$1-" "$1"
+   EOF
+   chmod 755 editor
+'
+test_expect_success \
+   'creating an annotated tag with -F messagefile --edit should succeed' '
+   EDITOR=./editor git tag -F msgfile