2.19.2 wont launch

2018-11-22 Thread Paul Gureghian

I installed 2.19.2 on windows 7 , 32 bit and it wont launch.



Re: How to propagate critical fixs from master to develope branch.

2018-11-22 Thread Andrew Ardill
Hi GB,

On Fri, 23 Nov 2018 at 04:23, Mgr Georg Black  wrote:
>
> Hello everyone.I red git manual but I can't figure out how to propagate 
> critical change from master branch to long live develop branch. I red chapter 
> about rebasing that I think could solve it but at the end of this chapter is 
> written that it's not recommended for pubic repositories. I don't know how to 
> do it without merging develop branch to master.
> I'll appreciate list of orders very much. :-)
> Thanks for any info or link.
> GB

A lot of what to do comes down to how you and your team work, and your
ongoing maintenance needs are.

The two methods I've seen used are to either cherry-pick the critical
change on top of whichever branches need it, or to build the change
from the oldest branch point that has the error, and then merging
those changes up to any maintained branches.

The cherry-pick method is quick and dirty, and doesn't require much
messing about.

The 'hotfix branch' method requires a bit more work to set up, but can
make it easier to see where the change is coming from and where it has
been applied. This fits in with the 'git flow' development methodology
(but doesn't require it).

A pretty good discussion of these ideas can be found at
https://stackoverflow.com/questions/7175869/managing-hotfixes-when-develop-branch-is-very-different-from-master

Regards,

Andrew Ardill


Re: [PATCH] t5562: skip if NO_CURL is enabled

2018-11-22 Thread Max Kirillov
On Thu, Nov 22, 2018 at 11:17:22AM -0500, Jeff King wrote:
> The script I use is at:
> 
>   https://github.com/peff/git/blob/meta/stress
> 
> which you invoke like "/path/to/stress t5562" from the top-level of a
> git.git checkout.  It basically just runs a loop of twice as many
> simultaneous invocations of the test script as you have CPUs, and waits
> for one to fail. The load created by all of the runs tends to flush out
> timing effects after a while.
> 
> It fails for me on t5562 within 30 seconds or so (but note that in this
> particular case it sometimes takes a while to produce the final output
> because invoke-with-content-length misses the expected SIGCLD and sleeps
> the full 60 seconds).

I have observed it caught failure at the very first run.
However I could not fail t again. I tried running up to 20
instances, with 1 or 2 active cores (that's all I have
here), also edited the test to include only push_plain case,
and repeat it several times, to avoid running irrelevant
cases, the failure never happened again.

The first failure was a bit unusual, in the ouput actually
all tests were marked as passed, but it still failed
somehow. Unfortunately, I did not save the output.

I submitted the perl patch

-- 
Max


[PATCH] t5562: fix perl path

2018-11-22 Thread Max Kirillov
From: Jeff King 

Some systems do not have perl installed to /usr/bin. Use the variable
from the build settiings, and call perl directly than via shebang.

Signed-off-by: Max Kirillov 
---
Submitting. Could you sign-off? Also removed shebang from the script as it is 
not needed
 t/t5562-http-backend-content-length.sh | 1 +
 t/t5562/invoke-with-content-length.pl  | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)
 mode change 100755 => 100644 t/t5562/invoke-with-content-length.pl

diff --git a/t/t5562-http-backend-content-length.sh 
b/t/t5562-http-backend-content-length.sh
index b24d8b05a4..90d890d02f 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -31,6 +31,7 @@ test_http_env() {
PATH_TRANSLATED="$PWD/.git/git-$handler_type-pack" \
GIT_HTTP_EXPORT_ALL=TRUE \
REQUEST_METHOD=POST \
+   "$PERL_PATH" \
"$TEST_DIRECTORY"/t5562/invoke-with-content-length.pl \
"$request_body" git http-backend >act.out 2>act.err
 }
diff --git a/t/t5562/invoke-with-content-length.pl 
b/t/t5562/invoke-with-content-length.pl
old mode 100755
new mode 100644
index 6c2aae7692..0943474af2
--- a/t/t5562/invoke-with-content-length.pl
+++ b/t/t5562/invoke-with-content-length.pl
@@ -1,4 +1,3 @@
-#!/usr/bin/perl
 use 5.008;
 use strict;
 use warnings;
-- 
2.19.0.1202.g68e1e8f04e



[PATCH v11 14/22] stash: convert show to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
Add stash show to the helper and delete the show_stash, have_stash,
assert_stash_like, is_stash_like and parse_flags_and_rev functions
from the shell script now that they are no longer needed.

In shell version, although `git stash show` accepts `--index` and
`--quiet` options, it ignores them. In C, both options are passed
further to `git diff`.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c |  87 ++
 git-stash.sh| 132 +---
 2 files changed, 88 insertions(+), 131 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index d66a4589a5..36651f745a 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -10,9 +10,12 @@
 #include "run-command.h"
 #include "dir.h"
 #include "rerere.h"
+#include "revision.h"
+#include "log-tree.h"
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
+   N_("git stash--helper show [] []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
@@ -25,6 +28,11 @@ static const char * const git_stash_helper_list_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_show_usage[] = {
+   N_("git stash--helper show [] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -645,6 +653,83 @@ static int list_stash(int argc, const char **argv, const 
char *prefix)
return run_command();
 }
 
+static int show_stat = 1;
+static int show_patch;
+
+static int git_stash_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "stash.showstat")) {
+   show_stat = git_config_bool(var, value);
+   return 0;
+   }
+   if (!strcmp(var, "stash.showpatch")) {
+   show_patch = git_config_bool(var, value);
+   return 0;
+   }
+   return git_default_config(var, value, cb);
+}
+
+static int show_stash(int argc, const char **argv, const char *prefix)
+{
+   int i;
+   int opts = 0;
+   int ret = 0;
+   struct stash_info info;
+   struct rev_info rev;
+   struct argv_array stash_args = ARGV_ARRAY_INIT;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   init_diff_ui_defaults();
+   git_config(git_diff_ui_config, NULL);
+   init_revisions(, prefix);
+
+   for (i = 1; i < argc; i++) {
+   if (argv[i][0] != '-')
+   argv_array_push(_args, argv[i]);
+   else
+   opts++;
+   }
+
+   ret = get_stash_info(, stash_args.argc, stash_args.argv);
+   argv_array_clear(_args);
+   if (ret)
+   return -1;
+
+   /*
+* The config settings are applied only if there are not passed
+* any options.
+*/
+   if (!opts) {
+   git_config(git_stash_config, NULL);
+   if (show_stat)
+   rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT;
+
+   if (show_patch)
+   rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+
+   if (!show_stat && !show_patch) {
+   free_stash_info();
+   return 0;
+   }
+   }
+
+   argc = setup_revisions(argc, argv, , NULL);
+   if (argc > 1) {
+   free_stash_info();
+   usage_with_options(git_stash_helper_show_usage, options);
+   }
+
+   rev.diffopt.flags.recursive = 1;
+   setup_diff_pager();
+   diff_tree_oid(_commit, _commit, "", );
+   log_tree_diff_flush();
+
+   free_stash_info();
+   return diff_result_code(, 0);
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -677,6 +762,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!branch_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "list"))
return !!list_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "show"))
+   return !!show_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 6052441aa2..0d05cbc1e5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -378,35 +378,6 @@ save_stash () {
fi
 }
 
-have_stash () {
-   git rev-parse --verify --quiet $ref_stash >/dev/null
-}
-
-show_stash () {
-   ALLOW_UNKNOWN_FLAGS=t
-   assert_stash_like "$@"
-
-   if test -z "$FLAGS"
-   then
-   if test "$(git config --bool stash.showStat || echo true)" = 
"true"
-   then
-   FLAGS=--stat
-  

[PATCH v11 20/22] stash: convert `stash--helper.c` into `stash.c`

2018-11-22 Thread Paul-Sebastian Ungureanu
The old shell script `git-stash.sh`  was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.

This commit also removes the word `helper` since now stash is
called directly and not by a shell script.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 .gitignore   |   1 -
 Makefile |   3 +-
 builtin.h|   2 +-
 builtin/{stash--helper.c => stash.c} | 157 +++
 git-stash.sh | 153 --
 git.c|   2 +-
 6 files changed, 92 insertions(+), 226 deletions(-)
 rename builtin/{stash--helper.c => stash.c} (91%)
 delete mode 100755 git-stash.sh

diff --git a/.gitignore b/.gitignore
index 6ecab90ab2..0d77ea5894 100644
--- a/.gitignore
+++ b/.gitignore
@@ -162,7 +162,6 @@
 /git-show-ref
 /git-stage
 /git-stash
-/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index aa83545e94..450936fcaf 100644
--- a/Makefile
+++ b/Makefile
@@ -619,7 +619,6 @@ SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-legacy-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
@@ -1115,7 +1114,7 @@ BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-index.o
 BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/stash--helper.o
+BUILTIN_OBJS += builtin/stash.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index ff4460aff7..b78ab6e30b 100644
--- a/builtin.h
+++ b/builtin.h
@@ -225,7 +225,7 @@ extern int cmd_show(int argc, const char **argv, const char 
*prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_show_index(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
-extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash.c
similarity index 91%
rename from builtin/stash--helper.c
rename to builtin/stash.c
index 47a0ab6669..c76a1936d5 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash.c
@@ -16,75 +16,70 @@
 
 #define INCLUDE_ALL_FILES 2
 
-static const char * const git_stash_helper_usage[] = {
-   N_("git stash--helper list []"),
-   N_("git stash--helper show [] []"),
-   N_("git stash--helper drop [-q|--quiet] []"),
-   N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
-   N_("git stash--helper branch  []"),
-   N_("git stash--helper clear"),
-   N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+static const char * const git_stash_usage[] = {
+   N_("git stash list []"),
+   N_("git stash show [] []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash clear"),
+   N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
   "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
   "  [--] [...]]"),
-   N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+   N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
   "  [-u|--include-untracked] [-a|--all] []"),
NULL
 };
 
-static const char * const git_stash_helper_list_usage[] = {
-   N_("git stash--helper list []"),
+static const char * const git_stash_list_usage[] = {
+   N_("git stash list []"),
NULL
 };
 
-static const char * const git_stash_helper_show_usage[] = {
-   N_("git stash--helper show [] []"),
+static const char * const git_stash_show_usage[] = {
+   N_("git stash show [] []"),
NULL
 };
 
-static const char * const git_stash_helper_drop_usage[] = {
-   N_("git stash--helper drop [-q|--quiet] []"),
+static const char * const git_stash_drop_usage[] = {
+   N_("git stash drop [-q|--quiet] []"),
NULL
 };
 
-static const char * const git_stash_helper_pop_usage[] = {
-   N_("git stash--helper pop [--index] 

[PATCH v11 15/22] stash: convert store to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
Add stash store to the helper and delete the store_stash function
from the shell script.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 62 +
 git-stash.sh| 43 ++--
 2 files changed, 64 insertions(+), 41 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 36651f745a..5dc6c068d7 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -58,6 +58,11 @@ static const char * const git_stash_helper_clear_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_store_usage[] = {
+   N_("git stash--helper store [-m|--message ] [-q|--quiet] 
"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -730,6 +735,61 @@ static int show_stash(int argc, const char **argv, const 
char *prefix)
return diff_result_code(, 0);
 }
 
+static int do_store_stash(const struct object_id *w_commit, const char 
*stash_msg,
+ int quiet)
+{
+   if (!stash_msg)
+   stash_msg = "Created via \"git stash store\".";
+
+   if (update_ref(stash_msg, ref_stash, w_commit, NULL,
+  REF_FORCE_CREATE_REFLOG,
+  quiet ? UPDATE_REFS_QUIET_ON_ERR :
+  UPDATE_REFS_MSG_ON_ERR)) {
+   if (!quiet) {
+   fprintf_ln(stderr, _("Cannot update %s with %s"),
+  ref_stash, oid_to_hex(w_commit));
+   }
+   return -1;
+   }
+
+   return 0;
+}
+
+static int store_stash(int argc, const char **argv, const char *prefix)
+{
+   int quiet = 0;
+   const char *stash_msg = NULL;
+   struct object_id obj;
+   struct object_context dummy;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet")),
+   OPT_STRING('m', "message", _msg, "message",
+  N_("stash message")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_store_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+   if (argc != 1) {
+   if (!quiet)
+   fprintf_ln(stderr, _("\"git stash store\" requires one "
+" argument"));
+   return -1;
+   }
+
+   if (get_oid_with_context(argv[0], quiet ? GET_OID_QUIETLY : 0, ,
+)) {
+   if (!quiet)
+   fprintf_ln(stderr, _("Cannot update %s with %s"),
+ref_stash, argv[0]);
+   return -1;
+   }
+
+   return do_store_stash(, stash_msg, quiet);
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -764,6 +824,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!list_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "show"))
return !!show_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "store"))
+   return !!store_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0d05cbc1e5..5739c51527 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -191,45 +191,6 @@ create_stash () {
die "$(gettext "Cannot record working tree state")"
 }
 
-store_stash () {
-   while test $# != 0
-   do
-   case "$1" in
-   -m|--message)
-   shift
-   stash_msg="$1"
-   ;;
-   -m*)
-   stash_msg=${1#-m}
-   ;;
-   --message=*)
-   stash_msg=${1#--message=}
-   ;;
-   -q|--quiet)
-   quiet=t
-   ;;
-   *)
-   break
-   ;;
-   esac
-   shift
-   done
-   test $# = 1 ||
-   die "$(eval_gettext "\"$dashless store\" requires one  
argument")"
-
-   w_commit="$1"
-   if test -z "$stash_msg"
-   then
-   stash_msg="Created via \"git stash store\"."
-   fi
-
-   git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
-   ret=$?
-   test $ret != 0 && test -z "$quiet" &&
-   die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
-   return $ret
-}
-
 push_stash () {
keep_index=
patch_mode=
@@ -308,7 +269,7 @@ push_stash () {
clear_stash || die "$(gettext "Cannot initialize stash")"
 
create_stash -m "$stash_msg" -u "$untracked" -- 

[PATCH v11 16/22] stash: convert create to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
Add stash create to the helper.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 451 +++-
 git-stash.sh|   2 +-
 2 files changed, 451 insertions(+), 2 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 5dc6c068d7..cd769d87b3 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,9 @@
 #include "rerere.h"
 #include "revision.h"
 #include "log-tree.h"
+#include "diffcore.h"
+
+#define INCLUDE_ALL_FILES 2
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
@@ -63,6 +66,11 @@ static const char * const git_stash_helper_store_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_create_usage[] = {
+   N_("git stash--helper create []"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -288,6 +296,24 @@ static int reset_head(void)
return run_command();
 }
 
+static void add_diff_to_buf(struct diff_queue_struct *q,
+   struct diff_options *options,
+   void *data)
+{
+   int i;
+
+   for (i = 0; i < q->nr; i++) {
+   strbuf_addstr(data, q->queue[i]->one->path);
+
+   /*
+* The reason we add "0" at the end of this strbuf
+* is because we will pass the output further to
+* "git update-index -z ...".
+*/
+   strbuf_addch(data, '\0');
+   }
+}
+
 static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
 {
struct child_process cp = CHILD_PROCESS_INIT;
@@ -790,6 +816,427 @@ static int store_stash(int argc, const char **argv, const 
char *prefix)
return do_store_stash(, stash_msg, quiet);
 }
 
+static void add_pathspecs(struct argv_array *args,
+ struct pathspec ps) {
+   int i;
+
+   for (i = 0; i < ps.nr; i++)
+   argv_array_push(args, ps.items[i].match);
+}
+
+/*
+ * `untracked_files` will be filled with the names of untracked files.
+ * The return value is:
+ *
+ * = 0 if there are not any untracked files
+ * > 0 if there are untracked files
+ */
+static int get_untracked_files(struct pathspec ps, int include_untracked,
+  struct strbuf *untracked_files)
+{
+   int i;
+   int max_len;
+   int found = 0;
+   char *seen;
+   struct dir_struct dir;
+
+   memset(, 0, sizeof(dir));
+   if (include_untracked != INCLUDE_ALL_FILES)
+   setup_standard_excludes();
+
+   seen = xcalloc(ps.nr, 1);
+
+   max_len = fill_directory(, the_repository->index, );
+   for (i = 0; i < dir.nr; i++) {
+   struct dir_entry *ent = dir.entries[i];
+   if (dir_path_match(_index, ent, , max_len, seen)) {
+   found++;
+   strbuf_addstr(untracked_files, ent->name);
+   /* NUL-terminate: will be fed to update-index -z */
+   strbuf_addch(untracked_files, 0);
+   }
+   free(ent);
+   }
+
+   free(seen);
+   free(dir.entries);
+   free(dir.ignored);
+   clear_directory();
+   return found;
+}
+
+/*
+ * The return value of `check_changes()` can be:
+ *
+ * < 0 if there was an error
+ * = 0 if there are no changes.
+ * > 0 if there are changes.
+ */
+static int check_changes(struct pathspec ps, int include_untracked)
+{
+   int result;
+   struct rev_info rev;
+   struct object_id dummy;
+   struct strbuf out = STRBUF_INIT;
+
+   /* No initial commit. */
+   if (get_oid("HEAD", ))
+   return -1;
+
+   if (read_cache() < 0)
+   return -1;
+
+   init_revisions(, NULL);
+   rev.prune_data = ps;
+
+   rev.diffopt.flags.quick = 1;
+   rev.diffopt.flags.ignore_submodules = 1;
+   rev.abbrev = 0;
+
+   add_head_to_pending();
+   diff_setup_done();
+
+   result = run_diff_index(, 1);
+   if (diff_result_code(, result))
+   return 1;
+
+   object_array_clear();
+   result = run_diff_files(, 0);
+   if (diff_result_code(, result))
+   return 1;
+
+   if (include_untracked && get_untracked_files(ps, include_untracked,
+)) {
+   strbuf_release();
+   return 1;
+   }
+
+   strbuf_release();
+   return 0;
+}
+
+static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
+   struct strbuf files)
+{
+   int ret = 0;
+   struct strbuf untracked_msg = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
+   struct child_process cp_upd_index = CHILD_PROCESS_INIT;
+   struct child_process cp_write_tree = CHILD_PROCESS_INIT;
+
+   cp_upd_index.git_cmd = 1;
+ 

[PATCH v11 01/22] sha1-name.c: add `get_oidf()` which acts like `get_oid()`

2018-11-22 Thread Paul-Sebastian Ungureanu
Compared to `get_oid()`, `get_oidf()` has as parameters
a pointer to `object_id`, a printf format string and
additional arguments. This will help simplify the code
in subsequent commits.

Original-idea-by: Johannes Schindelin 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 cache.h |  1 +
 sha1-name.c | 19 +++
 2 files changed, 20 insertions(+)

diff --git a/cache.h b/cache.h
index 8b1ee42ae9..6f1a549489 100644
--- a/cache.h
+++ b/cache.h
@@ -1334,6 +1334,7 @@ struct object_context {
GET_OID_BLOB)
 
 extern int get_oid(const char *str, struct object_id *oid);
+extern int get_oidf(struct object_id *oid, const char *fmt, ...);
 extern int get_oid_commit(const char *str, struct object_id *oid);
 extern int get_oid_committish(const char *str, struct object_id *oid);
 extern int get_oid_tree(const char *str, struct object_id *oid);
diff --git a/sha1-name.c b/sha1-name.c
index faa60f69e3..cf0e8a3f85 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1542,6 +1542,25 @@ int get_oid(const char *name, struct object_id *oid)
return get_oid_with_context(name, 0, oid, );
 }
 
+/*
+ * This returns a non-zero value if the string (built using printf
+ * format and the given arguments) is not a valid object.
+ */
+int get_oidf(struct object_id *oid, const char *fmt, ...)
+{
+   va_list ap;
+   int ret;
+   struct strbuf sb = STRBUF_INIT;
+
+   va_start(ap, fmt);
+   strbuf_vaddf(, fmt, ap);
+   va_end(ap);
+
+   ret = get_oid(sb.buf, oid);
+   strbuf_release();
+
+   return ret;
+}
 
 /*
  * Many callers know that the user meant to name a commit-ish by
-- 
2.19.1.878.g0482332a22



[PATCH v11 02/22] strbuf.c: add `strbuf_join_argv()`

2018-11-22 Thread Paul-Sebastian Ungureanu
Implement `strbuf_join_argv()` to join arguments
into a strbuf.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 strbuf.c | 15 +++
 strbuf.h |  7 +++
 2 files changed, 22 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index f6a6cf78b9..82e90f1dfe 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -268,6 +268,21 @@ void strbuf_addbuf(struct strbuf *sb, const struct strbuf 
*sb2)
strbuf_setlen(sb, sb->len + sb2->len);
 }
 
+const char *strbuf_join_argv(struct strbuf *buf,
+int argc, const char **argv, char delim)
+{
+   if (!argc)
+   return buf->buf;
+
+   strbuf_addstr(buf, *argv);
+   while (--argc) {
+   strbuf_addch(buf, delim);
+   strbuf_addstr(buf, *(++argv));
+   }
+
+   return buf->buf;
+}
+
 void strbuf_addchars(struct strbuf *sb, int c, size_t n)
 {
strbuf_grow(sb, n);
diff --git a/strbuf.h b/strbuf.h
index fc40873b65..be02150df3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -288,6 +288,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const 
char *s)
  */
 void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);
 
+/**
+ * Join the arguments into a buffer. `delim` is put between every
+ * two arguments.
+ */
+const char *strbuf_join_argv(struct strbuf *buf, int argc,
+const char **argv, char delim);
+
 /**
  * This function can be used to expand a format string containing
  * placeholders. To that end, it parses the string and calls the specified
-- 
2.19.1.878.g0482332a22



[PATCH v11 10/22] stash: convert drop and clear to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
From: Joel Teichroeb 

Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 117 
 git-stash.sh|   4 +-
 2 files changed, 119 insertions(+), 2 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 997b1c0ecf..07b8ec5bcb 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,7 +12,14 @@
 #include "rerere.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
NULL
 };
 
@@ -21,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+   N_("git stash--helper clear"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -137,6 +149,32 @@ static int get_stash_info(struct stash_info *info, int 
argc, const char **argv)
return !(ret == 0 || ret == 1);
 }
 
+static int do_clear_stash(void)
+{
+   struct object_id obj;
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_clear_usage,
+PARSE_OPT_STOP_AT_NON_OPTION);
+
+   if (argc)
+   return error(_("git stash clear with parameters is "
+  "unimplemented"));
+
+   return do_clear_stash();
+}
+
 static int reset_tree(struct object_id *i_tree, int update, int reset)
 {
int nr_trees = 1;
@@ -424,6 +462,81 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info, int 
quiet)
+{
+   int ret;
+   struct child_process cp_reflog = CHILD_PROCESS_INIT;
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   /*
+* reflog does not provide a simple function for deleting refs. One will
+* need to be added to avoid implementing too much reflog code here
+*/
+
+   cp_reflog.git_cmd = 1;
+   argv_array_pushl(_reflog.args, "reflog", "delete", "--updateref",
+"--rewrite", NULL);
+   argv_array_push(_reflog.args, info->revision.buf);
+   ret = run_command(_reflog);
+   if (!ret) {
+   if (!quiet)
+   printf_ln(_("Dropped %s (%s)"), info->revision.buf,
+ oid_to_hex(>w_commit));
+   } else {
+   return error(_("%s: Could not drop stash entry"),
+info->revision.buf);
+   }
+
+   /*
+* This could easily be replaced by get_oid, but currently it will throw
+* a fatal error when a reflog is empty, which we can not recover from.
+*/
+   cp.git_cmd = 1;
+   /* Even though --quiet is specified, rev-parse still outputs the hash */
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   /* do_clear_stash if we just dropped the last stash entry */
+   if (ret)
+   do_clear_stash();
+
+   return 0;
+}
+
+static void assert_stash_ref(struct stash_info *info)
+{
+   if (!info->is_stash_ref) {
+   free_stash_info(info);
+   error(_("'%s' is not a stash reference"), info->revision.buf);
+   exit(128);
+   }
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+   int ret;
+   int quiet = 0;
+   struct stash_info info;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_drop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   assert_stash_ref();
+
+   ret = do_drop_stash(prefix, , quiet);
+   

[PATCH v11 21/22] stash: optimize `get_untracked_files()` and `check_changes()`

2018-11-22 Thread Paul-Sebastian Ungureanu
This commits introduces a optimization by avoiding calling the
same functions again. For example, `git stash push -u`
would call at some points the following functions:

 * `check_changes()` (inside `do_push_stash()`)
 * `do_create_stash()`, which calls: `check_changes()` and
`get_untracked_files()`

Note that `check_changes()` also calls `get_untracked_files()`.
So, `check_changes()` is called 2 times and `get_untracked_files()`
3 times.

The old function `check_changes()` now consists of two functions:
`get_untracked_files()` and `check_changes_tracked_files()`.

These are the call chains for `push` and `create`:

 * `push_stash()` -> `do_push_stash()` -> `do_create_stash()`

 * `create_stash()` -> `do_create_stash()`

To prevent calling the same functions over and over again,
`check_changes()` inside `do_create_stash()` is now placed
in the caller functions (`create_stash()` and `do_push_stash()`).
This way `check_changes()` and `get_untracked files()` are called
only one time.

https://public-inbox.org/git/20180818223329.gj11...@hank.intra.tgummerer.com/

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash.c | 50 -
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index c76a1936d5..5ad0f443ca 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -879,18 +879,18 @@ static int get_untracked_files(struct pathspec ps, int 
include_untracked,
 }
 
 /*
- * The return value of `check_changes()` can be:
+ * The return value of `check_changes_tracked_files()` can be:
  *
  * < 0 if there was an error
  * = 0 if there are no changes.
  * > 0 if there are changes.
  */
-static int check_changes(struct pathspec ps, int include_untracked)
+
+static int check_changes_tracked_files(struct pathspec ps)
 {
int result;
struct rev_info rev;
struct object_id dummy;
-   struct strbuf out = STRBUF_INIT;
 
/* No initial commit. */
if (get_oid("HEAD", ))
@@ -918,14 +918,26 @@ static int check_changes(struct pathspec ps, int 
include_untracked)
if (diff_result_code(, result))
return 1;
 
+   return 0;
+}
+
+/*
+ * The function will fill `untracked_files` with the names of untracked files
+ * It will return 1 if there were any changes and 0 if there were not.
+ */
+
+static int check_changes(struct pathspec ps, int include_untracked,
+struct strbuf *untracked_files)
+{
+   int ret = 0;
+   if (check_changes_tracked_files(ps))
+   ret = 1;
+
if (include_untracked && get_untracked_files(ps, include_untracked,
-)) {
-   strbuf_release();
-   return 1;
-   }
+untracked_files))
+   ret = 1;
 
-   strbuf_release();
-   return 0;
+   return ret;
 }
 
 static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
@@ -1134,7 +1146,7 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
head_commit = lookup_commit(the_repository, >b_commit);
}
 
-   if (!check_changes(ps, include_untracked)) {
+   if (!check_changes(ps, include_untracked, _files)) {
ret = 1;
goto done;
}
@@ -1159,8 +1171,7 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
goto done;
}
 
-   if (include_untracked && get_untracked_files(ps, include_untracked,
-_files)) {
+   if (include_untracked) {
if (save_untracked_files(info, , untracked_files)) {
if (!quiet)
fprintf_ln(stderr, _("Cannot save "
@@ -1235,18 +1246,14 @@ static int create_stash(int argc, const char **argv, 
const char *prefix)
strbuf_join_argv(_msg_buf, argc - 1, ++argv, ' ');
 
memset(, 0, sizeof(ps));
-   ret = do_create_stash(ps, _msg_buf, 0, 0, , NULL, 0);
+   if (!check_changes_tracked_files(ps))
+   return 0;
 
-   if (!ret)
+   if (!(ret = do_create_stash(ps, _msg_buf, 0, 0, , NULL, 0)))
printf_ln("%s", oid_to_hex(_commit));
 
strbuf_release(_msg_buf);
-
-   /*
-* ret can be 1 if there were no changes. In this case, we should
-* not error out.
-*/
-   return ret < 0;
+   return ret;
 }
 
 static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
@@ -1256,6 +1263,7 @@ static int do_push_stash(struct pathspec ps, const char 
*stash_msg, int quiet,
struct stash_info info;
struct strbuf patch = STRBUF_INIT;
struct strbuf stash_msg_buf = STRBUF_INIT;
+   struct strbuf untracked_files = STRBUF_INIT;
 
if (patch_mode && keep_index == -1)
keep_index = 1;
@@ 

[PATCH v11 12/22] stash: convert pop to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
From: Joel Teichroeb 

Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref functions from the shell script now that they
are no longer needed.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 39 +-
 git-stash.sh| 47 ++---
 2 files changed, 40 insertions(+), 46 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 68b65165e4..d7ff78784b 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,7 +13,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
-   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
@@ -24,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_pop_usage[] = {
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -543,6 +548,36 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+   int ret;
+   int index = 0;
+   int quiet = 0;
+   struct stash_info info;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_BOOL(0, "index", ,
+N_("attempt to recreate the index")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_pop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   assert_stash_ref();
+   if ((ret = do_apply_stash(prefix, , index, quiet)))
+   printf_ln(_("The stash entry is kept in case "
+   "you need it again."));
+   else
+   ret = do_drop_stash(prefix, , quiet);
+
+   free_stash_info();
+   return ret;
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
int ret;
@@ -607,6 +642,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "pop"))
+   return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
 
diff --git a/git-stash.sh b/git-stash.sh
index 29d9f44255..8f2640fe90 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
 }
 
-is_stash_ref() {
-   is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
-   is_stash_ref "$@" || {
-   args="$*"
-   die "$(eval_gettext "'\$args' is not a stash reference")"
-   }
-}
-
-apply_stash () {
-   cd "$START_DIR"
-   git stash--helper apply "$@"
-   res=$?
-   cd_to_toplevel
-   return $res
-}
-
-pop_stash() {
-   assert_stash_ref "$@"
-
-   if apply_stash "$@"
-   then
-   drop_stash "$@"
-   else
-   status=$?
-   say "$(gettext "The stash entry is kept in case you need it 
again.")"
-   exit $status
-   fi
-}
-
-drop_stash () {
-   assert_stash_ref "$@"
-
-   git reflog delete --updateref --rewrite "${REV}" &&
-   say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-   die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
-   # clear_stash if we just dropped the last stash entry
-   git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
-   clear_stash
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -655,7 +611,8 @@ drop)
;;
 pop)
shift
-   pop_stash "$@"
+   cd "$START_DIR"
+   git stash--helper pop "$@"
;;
 branch)
shift
-- 
2.19.1.878.g0482332a22



[PATCH v11 18/22] stash: make push -q quiet

2018-11-22 Thread Paul-Sebastian Ungureanu
There is a change in behaviour with this commit. When there was
no initial commit, the shell version of stash would still display
a message. This commit makes `push` to not display any message if
`--quiet` or `-q` is specified. Add tests for `--quiet`.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 56 ++---
 t/t3903-stash.sh| 23 +
 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 8683c662fc..0dd5dbade6 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -971,7 +971,7 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg,
 }
 
 static int stash_patch(struct stash_info *info, struct pathspec ps,
-  struct strbuf *out_patch)
+  struct strbuf *out_patch, int quiet)
 {
int ret = 0;
struct strbuf out = STRBUF_INIT;
@@ -1024,7 +1024,8 @@ static int stash_patch(struct stash_info *info, struct 
pathspec ps,
}
 
if (!out_patch->len) {
-   fprintf_ln(stderr, _("No changes selected"));
+   if (!quiet)
+   fprintf_ln(stderr, _("No changes selected"));
ret = 1;
}
 
@@ -1102,7 +1103,8 @@ static int stash_working_tree(struct stash_info *info, 
struct pathspec ps)
 
 static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
   int include_untracked, int patch_mode,
-  struct stash_info *info, struct strbuf *patch)
+  struct stash_info *info, struct strbuf *patch,
+  int quiet)
 {
int ret = 0;
int flags = 0;
@@ -1120,7 +1122,9 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
refresh_cache(REFRESH_QUIET);
 
if (get_oid("HEAD", >b_commit)) {
-   fprintf_ln(stderr, _("You do not have the initial commit yet"));
+   if (!quiet)
+   fprintf_ln(stderr, _("You do not have "
+"the initial commit yet"));
ret = -1;
goto done;
} else {
@@ -1145,7 +1149,9 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
if (write_cache_as_tree(>i_tree, 0, NULL) ||
commit_tree(commit_tree_label.buf, commit_tree_label.len,
>i_tree, parents, >i_commit, NULL, NULL)) {
-   fprintf_ln(stderr, _("Cannot save the current index state"));
+   if (!quiet)
+   fprintf_ln(stderr, _("Cannot save the current "
+"index state"));
ret = -1;
goto done;
}
@@ -1153,26 +1159,29 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
if (include_untracked && get_untracked_files(ps, include_untracked,
 _files)) {
if (save_untracked_files(info, , untracked_files)) {
-   fprintf_ln(stderr, _("Cannot save "
-"the untracked files"));
+   if (!quiet)
+   fprintf_ln(stderr, _("Cannot save "
+"the untracked files"));
ret = -1;
goto done;
}
untracked_commit_option = 1;
}
if (patch_mode) {
-   ret = stash_patch(info, ps, patch);
+   ret = stash_patch(info, ps, patch, quiet);
if (ret < 0) {
-   fprintf_ln(stderr, _("Cannot save the current "
-"worktree state"));
+   if (!quiet)
+   fprintf_ln(stderr, _("Cannot save the current "
+"worktree state"));
goto done;
} else if (ret > 0) {
goto done;
}
} else {
if (stash_working_tree(info, ps)) {
-   fprintf_ln(stderr, _("Cannot save the current "
-"worktree state"));
+   if (!quiet)
+   fprintf_ln(stderr, _("Cannot save the current "
+"worktree state"));
ret = -1;
goto done;
}
@@ -1198,7 +1207,9 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
 
if (commit_tree(stash_msg_buf->buf, stash_msg_buf->len, >w_tree,
parents, >w_commit, NULL, NULL)) {
-   

[PATCH v11 11/22] stash: convert branch to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
From: Joel Teichroeb 

Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

Checkout does not currently provide a function for checking out
a branch as cmd_checkout does a large amount of sanity checks
first that we require here.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 46 +
 git-stash.sh| 17 ++-
 2 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 07b8ec5bcb..68b65165e4 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -14,6 +14,7 @@
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
 };
@@ -28,6 +29,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+   N_("git stash--helper branch  []"),
+   NULL
+};
+
 static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -537,6 +543,44 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   int ret;
+   const char *branch = NULL;
+   struct stash_info info;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_branch_usage, 0);
+
+   if (!argc) {
+   fprintf_ln(stderr, _("No branch name specified"));
+   return -1;
+   }
+
+   branch = argv[0];
+
+   if (get_stash_info(, argc - 1, argv + 1))
+   return -1;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, oid_to_hex(_commit));
+   ret = run_command();
+   if (!ret)
+   ret = do_apply_stash(prefix, , 1, 0);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, , 0);
+
+   free_stash_info();
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -563,6 +607,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   return !!branch_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index a99d5dc9e5..29d9f44255 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
 }
 
-apply_to_branch () {
-   test -n "$1" || die "$(gettext "No branch name specified")"
-   branch=$1
-   shift 1
-
-   set -- --index "$@"
-   assert_stash_like "$@"
-
-   git checkout -b $branch $REV^ &&
-   apply_stash "$@" && {
-   test -z "$IS_STASH_REF" || drop_stash "$@"
-   }
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -673,7 +659,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.19.1.878.g0482332a22



[PATCH v11 22/22] stash: replace all `write-tree` child processes with API calls

2018-11-22 Thread Paul-Sebastian Ungureanu
This commit replaces spawning `git write-tree` with API calls.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash.c | 41 -
 1 file changed, 12 insertions(+), 29 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 5ad0f443ca..029e209176 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -945,9 +945,8 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg,
 {
int ret = 0;
struct strbuf untracked_msg = STRBUF_INIT;
-   struct strbuf out = STRBUF_INIT;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
-   struct child_process cp_write_tree = CHILD_PROCESS_INIT;
+   struct index_state istate = { NULL };
 
cp_upd_index.git_cmd = 1;
argv_array_pushl(_upd_index.args, "update-index", "-z", "--add",
@@ -962,15 +961,11 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg,
goto done;
}
 
-   cp_write_tree.git_cmd = 1;
-   argv_array_push(_write_tree.args, "write-tree");
-   argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
-   if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+   if (write_index_as_tree(>u_tree, , stash_index_path.buf, 0,
+   NULL)) {
ret = -1;
goto done;
}
-   get_oid_hex(out.buf, >u_tree);
 
if (commit_tree(untracked_msg.buf, untracked_msg.len,
>u_tree, NULL, >u_commit, NULL, NULL)) {
@@ -979,8 +974,8 @@ static int save_untracked_files(struct stash_info *info, 
struct strbuf *msg,
}
 
 done:
+   discard_index();
strbuf_release(_msg);
-   strbuf_release();
remove_path(stash_index_path.buf);
return ret;
 }
@@ -989,11 +984,10 @@ static int stash_patch(struct stash_info *info, struct 
pathspec ps,
   struct strbuf *out_patch, int quiet)
 {
int ret = 0;
-   struct strbuf out = STRBUF_INIT;
struct child_process cp_read_tree = CHILD_PROCESS_INIT;
struct child_process cp_add_i = CHILD_PROCESS_INIT;
-   struct child_process cp_write_tree = CHILD_PROCESS_INIT;
struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
+   struct index_state istate = { NULL };
 
remove_path(stash_index_path.buf);
 
@@ -1019,17 +1013,12 @@ static int stash_patch(struct stash_info *info, struct 
pathspec ps,
}
 
/* State of the working tree. */
-   cp_write_tree.git_cmd = 1;
-   argv_array_push(_write_tree.args, "write-tree");
-   argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
-   if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+   if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+   NULL)) {
ret = -1;
goto done;
}
 
-   get_oid_hex(out.buf, >w_tree);
-
cp_diff_tree.git_cmd = 1;
argv_array_pushl(_diff_tree.args, "diff-tree", "-p", "HEAD",
 oid_to_hex(>w_tree), "--", NULL);
@@ -1045,7 +1034,7 @@ static int stash_patch(struct stash_info *info, struct 
pathspec ps,
}
 
 done:
-   strbuf_release();
+   discard_index();
remove_path(stash_index_path.buf);
return ret;
 }
@@ -1055,9 +1044,8 @@ static int stash_working_tree(struct stash_info *info, 
struct pathspec ps)
int ret = 0;
struct rev_info rev;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
-   struct child_process cp_write_tree = CHILD_PROCESS_INIT;
-   struct strbuf out = STRBUF_INIT;
struct strbuf diff_output = STRBUF_INIT;
+   struct index_state istate = { NULL };
 
set_alternate_index_output(stash_index_path.buf);
if (reset_tree(>i_tree, 0, 0)) {
@@ -1096,20 +1084,15 @@ static int stash_working_tree(struct stash_info *info, 
struct pathspec ps)
goto done;
}
 
-   cp_write_tree.git_cmd = 1;
-   argv_array_push(_write_tree.args, "write-tree");
-   argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
-   if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+   if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+   NULL)) {
ret = -1;
goto done;
}
 
-   get_oid_hex(out.buf, >w_tree);
-
 done:
+   discard_index();
UNLEAK(rev);
-   strbuf_release();
object_array_clear();
strbuf_release(_output);
remove_path(stash_index_path.buf);
-- 
2.19.1.878.g0482332a22



[PATCH v11 17/22] stash: convert push to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
Add stash push to the helper.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 245 +++-
 git-stash.sh|   6 +-
 2 files changed, 245 insertions(+), 6 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index cd769d87b3..8683c662fc 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -23,6 +23,9 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
+   N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
+  "  [--] [...]]"),
NULL
 };
 
@@ -71,6 +74,13 @@ static const char * const git_stash_helper_create_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_push_usage[] = {
+   N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
+  "  [--] [...]]"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -1092,7 +1102,7 @@ static int stash_working_tree(struct stash_info *info, 
struct pathspec ps)
 
 static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,
   int include_untracked, int patch_mode,
-  struct stash_info *info)
+  struct stash_info *info, struct strbuf *patch)
 {
int ret = 0;
int flags = 0;
@@ -1105,7 +1115,6 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
struct strbuf msg = STRBUF_INIT;
struct strbuf commit_tree_label = STRBUF_INIT;
struct strbuf untracked_files = STRBUF_INIT;
-   struct strbuf patch = STRBUF_INIT;
 
read_cache_preload(NULL);
refresh_cache(REFRESH_QUIET);
@@ -1152,7 +1161,7 @@ static int do_create_stash(struct pathspec ps, struct 
strbuf *stash_msg_buf,
untracked_commit_option = 1;
}
if (patch_mode) {
-   ret = stash_patch(info, ps, );
+   ret = stash_patch(info, ps, patch);
if (ret < 0) {
fprintf_ln(stderr, _("Cannot save the current "
 "worktree state"));
@@ -1223,7 +1232,8 @@ static int create_stash(int argc, const char **argv, 
const char *prefix)
 
memset(, 0, sizeof(ps));
strbuf_addstr(_msg_buf, stash_msg);
-   ret = do_create_stash(ps, _msg_buf, include_untracked, 0, );
+   ret = do_create_stash(ps, _msg_buf, include_untracked, 0, ,
+ NULL);
 
if (!ret)
printf_ln("%s", oid_to_hex(_commit));
@@ -1237,6 +1247,231 @@ static int create_stash(int argc, const char **argv, 
const char *prefix)
return ret < 0;
 }
 
+static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,
+int keep_index, int patch_mode, int include_untracked)
+{
+   int ret = 0;
+   struct stash_info info;
+   struct strbuf patch = STRBUF_INIT;
+   struct strbuf stash_msg_buf = STRBUF_INIT;
+
+   if (patch_mode && keep_index == -1)
+   keep_index = 1;
+
+   if (patch_mode && include_untracked) {
+   fprintf_ln(stderr, _("Can't use --patch and --include-untracked"
+" or --all at the same time"));
+   ret = -1;
+   goto done;
+   }
+
+   read_cache_preload(NULL);
+   if (!include_untracked && ps.nr) {
+   int i;
+   char *ps_matched = xcalloc(ps.nr, 1);
+
+   for (i = 0; i < active_nr; i++)
+   ce_path_match(_index, active_cache[i], ,
+ ps_matched);
+
+   if (report_path_error(ps_matched, , NULL)) {
+   fprintf_ln(stderr, _("Did you forget to 'git add'?"));
+   ret = -1;
+   free(ps_matched);
+   goto done;
+   }
+   free(ps_matched);
+   }
+
+   if (refresh_cache(REFRESH_QUIET)) {
+   ret = -1;
+   goto done;
+   }
+
+   if (!check_changes(ps, include_untracked)) {
+   if (!quiet)
+   printf_ln(_("No local changes to save"));
+   goto done;
+   }
+
+   if (!reflog_exists(ref_stash) && do_clear_stash()) {
+   ret = -1;
+   fprintf_ln(stderr, _("Cannot initialize stash"));
+   goto done;
+   }
+
+   if (stash_msg)
+   strbuf_addstr(_msg_buf, stash_msg);
+   if (do_create_stash(ps, 

[PATCH v11 09/22] stash: convert apply to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
From: Joel Teichroeb 

Add a builtin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
lets conversion get started without the other commands being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 452 
 git-stash.sh|  78 +--
 git.c   |   1 +
 6 files changed, 463 insertions(+), 71 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 0d77ea5894..6ecab90ab2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -162,6 +162,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index bbfbb4292d..aa83545e94 100644
--- a/Makefile
+++ b/Makefile
@@ -1115,6 +1115,7 @@ BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-index.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 6538932e99..ff4460aff7 100644
--- a/builtin.h
+++ b/builtin.h
@@ -225,6 +225,7 @@ extern int cmd_show(int argc, const char **argv, const char 
*prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_show_index(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 00..997b1c0ecf
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,452 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+#include "rerere.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static struct strbuf stash_index_path = STRBUF_INIT;
+
+/*
+ * w_commit is set to the commit containing the working tree
+ * b_commit is set to the base commit
+ * i_commit is set to the commit containing the index tree
+ * u_commit is set to the commit containing the untracked files tree
+ * w_tree is set to the working tree
+ * b_tree is set to the base tree
+ * i_tree is set to the index tree
+ * u_tree is set to the untracked files tree
+ */
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   struct strbuf revision;
+   int is_stash_ref;
+   int has_u;
+};
+
+static void free_stash_info(struct stash_info *info)
+{
+   strbuf_release(>revision);
+}
+
+static void assert_stash_like(struct stash_info *info, const char *revision)
+{
+   if (get_oidf(>b_commit, "%s^1", revision) ||
+   get_oidf(>w_tree, "%s:", revision) ||
+   get_oidf(>b_tree, "%s^1:", revision) ||
+   get_oidf(>i_tree, "%s^2:", revision))
+   die(_("'%s' is not a stash-like commit"), revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+   int ret;
+   char *end_of_rev;
+   char *expanded_ref;
+   const char *revision;
+   const char *commit = NULL;
+   struct object_id dummy;
+   struct strbuf symbolic = STRBUF_INIT;
+
+   if (argc > 1) {
+   int i;
+   struct strbuf refs_msg = STRBUF_INIT;
+
+   for (i = 0; i < argc; i++)
+   strbuf_addf(_msg, " '%s'", argv[i]);
+
+   fprintf_ln(stderr, _("Too many revisions specified:%s"),
+ 

[PATCH v11 13/22] stash: convert list to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
Add stash list to the helper and delete the list_stash function
from the shell script.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c | 31 +++
 git-stash.sh|  7 +--
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index d7ff78784b..d66a4589a5 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 #include "rerere.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper list []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] 
[]"),
N_("git stash--helper branch  []"),
@@ -19,6 +20,11 @@ static const char * const git_stash_helper_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_list_usage[] = {
+   N_("git stash--helper list []"),
+   NULL
+};
+
 static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -616,6 +622,29 @@ static int branch_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int list_stash(int argc, const char **argv, const char *prefix)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_list_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+   if (!ref_exists(ref_stash))
+   return 0;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "log", "--format=%gd: %gs", "-g",
+"--first-parent", "-m", NULL);
+   argv_array_pushv(, argv);
+   argv_array_push(, ref_stash);
+   argv_array_push(, "--");
+   return run_command();
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -646,6 +675,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "list"))
+   return !!list_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 8f2640fe90..6052441aa2 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -382,11 +382,6 @@ have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
 }
 
-list_stash () {
-   have_stash || return 0
-   git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
-}
-
 show_stash () {
ALLOW_UNKNOWN_FLAGS=t
assert_stash_like "$@"
@@ -574,7 +569,7 @@ test -n "$seen_non_option" || set "push" "$@"
 case "$1" in
 list)
shift
-   list_stash "$@"
+   git stash--helper list "$@"
;;
 show)
shift
-- 
2.19.1.878.g0482332a22



[PATCH v11 19/22] stash: convert save to builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
Add stash save to the helper and delete functions which are no
longer needed (`show_help()`, `save_stash()`, `push_stash()`,
`create_stash()`, `clear_stash()`, `untracked_files()` and
`no_changes()`).

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/stash--helper.c |  50 +++
 git-stash.sh| 311 +---
 2 files changed, 52 insertions(+), 309 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 0dd5dbade6..47a0ab6669 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -26,6 +26,8 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
   "  [-u|--include-untracked] [-a|--all] [-m|--message 
]\n"
   "  [--] [...]]"),
+   N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] []"),
NULL
 };
 
@@ -81,6 +83,12 @@ static const char * const git_stash_helper_push_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_save_usage[] = {
+   N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index] 
[-q|--quiet]\n"
+  "  [-u|--include-untracked] [-a|--all] []"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -1488,6 +1496,46 @@ static int push_stash(int argc, const char **argv, const 
char *prefix)
 include_untracked);
 }
 
+static int save_stash(int argc, const char **argv, const char *prefix)
+{
+   int keep_index = -1;
+   int patch_mode = 0;
+   int include_untracked = 0;
+   int quiet = 0;
+   int ret = 0;
+   const char *stash_msg = NULL;
+   struct pathspec ps;
+   struct strbuf stash_msg_buf = STRBUF_INIT;
+   struct option options[] = {
+   OPT_BOOL('k', "keep-index", _index,
+N_("keep index")),
+   OPT_BOOL('p', "patch", _mode,
+N_("stash in patch mode")),
+   OPT__QUIET(, N_("quiet mode")),
+   OPT_BOOL('u', "include-untracked", _untracked,
+N_("include untracked files in stash")),
+   OPT_SET_INT('a', "all", _untracked,
+   N_("include ignore files"), 2),
+   OPT_STRING('m', "message", _msg, "message",
+  N_("stash message")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_save_usage,
+PARSE_OPT_KEEP_DASHDASH);
+
+   if (argc)
+   stash_msg = strbuf_join_argv(_msg_buf, argc, argv, ' ');
+
+   memset(, 0, sizeof(ps));
+   ret = do_push_stash(ps, stash_msg, quiet, keep_index,
+   patch_mode, include_untracked);
+
+   strbuf_release(_msg_buf);
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
pid_t pid = getpid();
@@ -1528,6 +1576,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
return !!create_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "push"))
return !!push_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "save"))
+   return !!save_stash(argc, argv, prefix);
 
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
  git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index c3146f62ab..695f1feba3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -36,314 +36,6 @@ else
reset_color=
 fi
 
-no_changes () {
-   git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
-   git diff-files --quiet --ignore-submodules -- "$@" &&
-   (test -z "$untracked" || test -z "$(untracked_files "$@")")
-}
-
-untracked_files () {
-   if test "$1" = "-z"
-   then
-   shift
-   z=-z
-   else
-   z=
-   fi
-   excl_opt=--exclude-standard
-   test "$untracked" = "all" && excl_opt=
-   git ls-files -o $z $excl_opt -- "$@"
-}
-
-clear_stash () {
-   if test $# != 0
-   then
-   die "$(gettext "git stash clear with parameters is 
unimplemented")"
-   fi
-   if current=$(git rev-parse --verify --quiet $ref_stash)
-   then
-   git update-ref -d $ref_stash $current
-   fi
-}
-
-create_stash () {
-   stash_msg=
-   untracked=
-   while test $# != 0
-   do
-   case "$1" in
-   -m|--message)
-   shift
-   stash_msg=${1?"BUG: create_stash () -m requires an 
argument"}
-   ;;
-   -m*)
-   stash_msg=${1#-m}

[PATCH v11 06/22] stash: rename test cases to be more descriptive

2018-11-22 Thread Paul-Sebastian Ungureanu
Rename some test cases' labels to be more descriptive and under 80
characters per line.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 t/t3903-stash.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 098a387a82..8b09a3d6cc 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -604,7 +604,7 @@ test_expect_success 'stash show -p - no stashes on stack, 
stash-like argument' '
test_cmp expected actual
 '
 
-test_expect_success 'stash drop - fail early if specified stash is not a stash 
reference' '
+test_expect_success 'drop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -618,7 +618,7 @@ test_expect_success 'stash drop - fail early if specified 
stash is not a stash r
git reset --hard HEAD
 '
 
-test_expect_success 'stash pop - fail early if specified stash is not a stash 
reference' '
+test_expect_success 'pop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -682,7 +682,7 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
git stash drop
 '
 
-test_expect_success 'stash branch should not drop the stash if the branch 
exists' '
+test_expect_success 'branch: do not drop the stash if the branch exists' '
git stash clear &&
echo foo >file &&
git add file &&
@@ -693,7 +693,7 @@ test_expect_success 'stash branch should not drop the stash 
if the branch exists
git rev-parse stash@{0} --
 '
 
-test_expect_success 'stash branch should not drop the stash if the apply 
fails' '
+test_expect_success 'branch: should not drop the stash if the apply fails' '
git stash clear &&
git reset HEAD~1 --hard &&
echo foo >file &&
@@ -707,7 +707,7 @@ test_expect_success 'stash branch should not drop the stash 
if the apply fails'
git rev-parse stash@{0} --
 '
 
-test_expect_success 'stash apply shows status same as git status (relative to 
current directory)' '
+test_expect_success 'apply: show same status as git status (relative to ./)' '
git stash clear &&
echo 1 >subdir/subfile1 &&
echo 2 >subdir/subfile2 &&
@@ -1048,7 +1048,7 @@ test_expect_success 'stash push -p with pathspec shows no 
changes only once' '
test_i18ncmp expect actual
 '
 
-test_expect_success 'stash push with pathspec shows no changes when there are 
none' '
+test_expect_success 'push : show no changes when there are none' '
>foo &&
git add foo &&
git commit -m "tmp" &&
@@ -1058,7 +1058,7 @@ test_expect_success 'stash push with pathspec shows no 
changes when there are no
test_i18ncmp expect actual
 '
 
-test_expect_success 'stash push with pathspec not in the repository errors 
out' '
+test_expect_success 'push:  not in the repository errors out' '
>untracked &&
test_must_fail git stash push untracked &&
test_path_is_file untracked
-- 
2.19.1.878.g0482332a22



[PATCH v11 05/22] t3903: modernize style

2018-11-22 Thread Paul-Sebastian Ungureanu
Remove whitespaces after redirection operators and wrap
long lines.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 t/t3903-stash.sh | 120 ---
 1 file changed, 61 insertions(+), 59 deletions(-)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 4f8aa56021..098a387a82 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -8,22 +8,22 @@ test_description='Test git stash'
 . ./test-lib.sh
 
 test_expect_success 'stash some dirty working directory' '
-   echo 1 > file &&
+   echo 1 >file &&
git add file &&
echo unrelated >other-file &&
git add other-file &&
test_tick &&
git commit -m initial &&
-   echo 2 > file &&
+   echo 2 >file &&
git add file &&
-   echo 3 > file &&
+   echo 3 >file &&
test_tick &&
git stash &&
git diff-files --quiet &&
git diff-index --cached --quiet HEAD
 '
 
-cat > expect << EOF
+cat >expect < output &&
+   git diff stash^2..stash >output &&
test_cmp expect output
 '
 
@@ -74,7 +74,7 @@ test_expect_success 'apply stashed changes' '
 
 test_expect_success 'apply stashed changes (including index)' '
git reset --hard HEAD^ &&
-   echo 6 > other-file &&
+   echo 6 >other-file &&
git add other-file &&
test_tick &&
git commit -m other-file &&
@@ -99,12 +99,12 @@ test_expect_success 'stash drop complains of extra options' 
'
 
 test_expect_success 'drop top stash' '
git reset --hard &&
-   git stash list > stashlist1 &&
-   echo 7 > file &&
+   git stash list >expected &&
+   echo 7 >file &&
git stash &&
git stash drop &&
-   git stash list > stashlist2 &&
-   test_cmp stashlist1 stashlist2 &&
+   git stash list >actual &&
+   test_cmp expected actual &&
git stash apply &&
test 3 = $(cat file) &&
test 1 = $(git show :file) &&
@@ -113,9 +113,9 @@ test_expect_success 'drop top stash' '
 
 test_expect_success 'drop middle stash' '
git reset --hard &&
-   echo 8 > file &&
+   echo 8 >file &&
git stash &&
-   echo 9 > file &&
+   echo 9 >file &&
git stash &&
git stash drop stash@{1} &&
test 2 = $(git stash list | wc -l) &&
@@ -160,7 +160,7 @@ test_expect_success 'stash pop' '
test 0 = $(git stash list | wc -l)
 '
 
-cat > expect << EOF
+cat >expect < expect1 << EOF
+cat >expect1 < expect2 << EOF
+cat >expect2 < file &&
+   echo foo >file &&
git commit file -m first &&
-   echo bar > file &&
-   echo bar2 > file2 &&
+   echo bar >file &&
+   echo bar2 >file2 &&
git add file2 &&
git stash &&
-   echo baz > file &&
+   echo baz >file &&
git commit file -m second &&
git stash branch stashbranch &&
test refs/heads/stashbranch = $(git symbolic-ref HEAD) &&
test $(git rev-parse HEAD) = $(git rev-parse master^) &&
-   git diff --cached > output &&
+   git diff --cached >output &&
test_cmp expect output &&
-   git diff > output &&
+   git diff >output &&
test_cmp expect1 output &&
git add file &&
git commit -m alternate\ second &&
-   git diff master..stashbranch > output &&
+   git diff master..stashbranch >output &&
test_cmp output expect2 &&
test 0 = $(git stash list | wc -l)
 '
 
 test_expect_success 'apply -q is quiet' '
-   echo foo > file &&
+   echo foo >file &&
git stash &&
-   git stash apply -q > output.out 2>&1 &&
+   git stash apply -q >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'save -q is quiet' '
-   git stash save --quiet > output.out 2>&1 &&
+   git stash save --quiet >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'pop -q is quiet' '
-   git stash pop -q > output.out 2>&1 &&
+   git stash pop -q >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'pop -q --index works and is quiet' '
-   echo foo > file &&
+   echo foo >file &&
git add file &&
git stash save --quiet &&
-   git stash pop -q --index > output.out 2>&1 &&
+   git stash pop -q --index >output.out 2>&1 &&
test foo = "$(git show :file)" &&
test_must_be_empty output.out
 '
 
 test_expect_success 'drop -q is quiet' '
git stash &&
-   git stash drop -q > output.out 2>&1 &&
+   git stash drop -q >output.out 2>&1 &&
test_must_be_empty output.out
 '
 
 test_expect_success 'stash -k' '
-   echo bar3 > file &&
-   echo bar4 > file2 &&
+   echo bar3 >file &&
+   echo bar4 >file2 &&
git add file2 &&
git stash -k &&
test bar,bar4 = $(cat file),$(cat file2)
 '
 
 test_expect_success 'stash --no-keep-index' '
-   echo bar33 > file &&
-   echo bar44 > file2 &&
+   echo bar33 

[PATCH v11 08/22] stash: mention options in `show` synopsis

2018-11-22 Thread Paul-Sebastian Ungureanu
Mention in the documentation, that `show` accepts any
option known to `git diff`.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 Documentation/git-stash.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 7ef8c47911..e31ea7d303 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 
 [verse]
 'git stash' list []
-'git stash' show []
+'git stash' show [] []
 'git stash' drop [-q|--quiet] []
 'git stash' ( pop | apply ) [--index] [-q|--quiet] []
 'git stash' branch  []
@@ -106,7 +106,7 @@ stash@{1}: On master: 9cc0589... Add git-stash
 The command takes options applicable to the 'git log'
 command to control what is shown and how. See linkgit:git-log[1].
 
-show []::
+show [] []::
 
Show the changes recorded in the stash entry as a diff between the
stashed contents and the commit back when the stash entry was first
-- 
2.19.1.878.g0482332a22



[PATCH v11 07/22] stash: add tests for `git stash show` config

2018-11-22 Thread Paul-Sebastian Ungureanu
This commit introduces tests for `git stash show`
config. It tests all the cases where `stash.showStat`
and `stash.showPatch` are unset or set to true / false.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 t/t3907-stash-show-config.sh | 83 
 1 file changed, 83 insertions(+)
 create mode 100755 t/t3907-stash-show-config.sh

diff --git a/t/t3907-stash-show-config.sh b/t/t3907-stash-show-config.sh
new file mode 100755
index 00..10914bba7b
--- /dev/null
+++ b/t/t3907-stash-show-config.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+test_description='Test git stash show configuration.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   test_commit file
+'
+
+# takes three parameters:
+# 1. the stash.showStat value (or "")
+# 2. the stash.showPatch value (or "")
+# 3. the diff options of the expected output (or nothing for no output)
+test_stat_and_patch () {
+   if test "" = "$1"
+   then
+   test_unconfig stash.showStat
+   else
+   test_config stash.showStat "$1"
+   fi &&
+
+   if test "" = "$2"
+   then
+   test_unconfig stash.showPatch
+   else
+   test_config stash.showPatch "$2"
+   fi &&
+
+   shift 2 &&
+   echo 2 >file.t &&
+   if test $# != 0
+   then
+   git diff "$@" >expect
+   fi &&
+   git stash &&
+   git stash show >actual &&
+
+   if test $# = 0
+   then
+   test_must_be_empty actual
+   else
+   test_cmp expect actual
+   fi
+}
+
+test_expect_success 'showStat unset showPatch unset' '
+   test_stat_and_patch "" "" --stat
+'
+
+test_expect_success 'showStat unset showPatch false' '
+   test_stat_and_patch "" false --stat
+'
+
+test_expect_success 'showStat unset showPatch true' '
+   test_stat_and_patch "" true --stat -p
+'
+
+test_expect_success 'showStat false showPatch unset' '
+   test_stat_and_patch false ""
+'
+
+test_expect_success 'showStat false showPatch false' '
+   test_stat_and_patch false false
+'
+
+test_expect_success 'showStat false showPatch true' '
+   test_stat_and_patch false true -p
+'
+
+test_expect_success 'showStat true showPatch unset' '
+   test_stat_and_patch true "" --stat
+'
+
+test_expect_success 'showStat true showPatch false' '
+   test_stat_and_patch true false --stat
+'
+
+test_expect_success 'showStat true showPatch true' '
+   test_stat_and_patch true true --stat -p
+'
+
+test_done
-- 
2.19.1.878.g0482332a22



[PATCH v11 03/22] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`

2018-11-22 Thread Paul-Sebastian Ungureanu
Implement `strbuf_insertf()` and `strbuf_vinsertf()` to
insert data using a printf format string.

Original-idea-by: Johannes Schindelin 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 strbuf.c | 36 
 strbuf.h |  9 +
 2 files changed, 45 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index 82e90f1dfe..bfbbdadbf3 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -249,6 +249,42 @@ void strbuf_insert(struct strbuf *sb, size_t pos, const 
void *data, size_t len)
strbuf_splice(sb, pos, 0, data, len);
 }
 
+void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list 
ap)
+{
+   int len, len2;
+   char save;
+   va_list cp;
+
+   if (pos > sb->len)
+   die("`pos' is too far after the end of the buffer");
+   va_copy(cp, ap);
+   len = vsnprintf(sb->buf + sb->len, 0, fmt, cp);
+   va_end(cp);
+   if (len < 0)
+   BUG("your vsnprintf is broken (returned %d)", len);
+   if (!len)
+   return; /* nothing to do */
+   if (unsigned_add_overflows(sb->len, len))
+   die("you want to use way too much memory");
+   strbuf_grow(sb, len);
+   memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
+   /* vsnprintf() will append a NUL, overwriting one of our characters */
+   save = sb->buf[pos + len];
+   len2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);
+   sb->buf[pos + len] = save;
+   if (len2 != len)
+   BUG("your vsnprintf is broken (returns inconsistent lengths)");
+   strbuf_setlen(sb, sb->len + len);
+}
+
+void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...)
+{
+   va_list ap;
+   va_start(ap, fmt);
+   strbuf_vinsertf(sb, pos, fmt, ap);
+   va_end(ap);
+}
+
 void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
 {
strbuf_splice(sb, pos, len, "", 0);
diff --git a/strbuf.h b/strbuf.h
index be02150df3..8f8fe01e68 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -244,6 +244,15 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);
  */
 void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);
 
+/**
+ * Insert data to the given position of the buffer giving a printf format
+ * string. The contents will be shifted, not overwritten.
+ */
+void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
+va_list ap);
+
+void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
+
 /**
  * Remove given amount of data from a given position of the buffer.
  */
-- 
2.19.1.878.g0482332a22



[PATCH v11 00/22] Convert "git stash" to C builtin

2018-11-22 Thread Paul-Sebastian Ungureanu
Hello,

This is the 11th iteration of C git stash. Here are some of the changes,
based on Thomas's and dscho's suggestions (from mailing list / pull request
#495):

- improved memory management. Now, the callers of `do_create_stash()`
are responsible of freeing the parameter they pass in. Moreover, the
stash message is now a pointer to a buffer (in the previous iteration
it was a pointer to a string). This should make it more clear who is
responsible of freeing the memory.

- added `strbuf_insertf()` which inserts a format string at a given
position in the buffer.

- some minor changes (changed "!oidcmp" to "oideq")

- fixed merge conflicts

Best regards,
Paul

Joel Teichroeb (5):
  stash: improve option parsing test coverage
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

Paul-Sebastian Ungureanu (17):
  sha1-name.c: add `get_oidf()` which acts like `get_oid()`
  strbuf.c: add `strbuf_join_argv()`
  strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
  t3903: modernize style
  stash: rename test cases to be more descriptive
  stash: add tests for `git stash show` config
  stash: mention options in `show` synopsis
  stash: convert list to builtin
  stash: convert show to builtin
  stash: convert store to builtin
  stash: convert create to builtin
  stash: convert push to builtin
  stash: make push -q quiet
  stash: convert save to builtin
  stash: convert `stash--helper.c` into `stash.c`
  stash: optimize `get_untracked_files()` and `check_changes()`
  stash: replace all `write-tree` child processes with API calls

 Documentation/git-stash.txt  |4 +-
 Makefile |2 +-
 builtin.h|1 +
 builtin/stash.c  | 1596 ++
 cache.h  |1 +
 git-stash.sh |  752 
 git.c|1 +
 sha1-name.c  |   19 +
 strbuf.c |   51 ++
 strbuf.h |   16 +
 t/t3903-stash.sh |  192 ++--
 t/t3907-stash-show-config.sh |   83 ++
 12 files changed, 1897 insertions(+), 821 deletions(-)
 create mode 100644 builtin/stash.c
 delete mode 100755 git-stash.sh
 create mode 100755 t/t3907-stash-show-config.sh

-- 
2.19.1.878.g0482332a22



[PATCH v11 04/22] stash: improve option parsing test coverage

2018-11-22 Thread Paul-Sebastian Ungureanu
From: Joel Teichroeb 

In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.

Signed-off-by: Joel Teichroeb 
Signed-off-by: Paul-Sebastian Ungureanu 
---
 t/t3903-stash.sh | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index cd216655b9..4f8aa56021 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,36 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
 '
 
+test_expect_success 'giving too many ref arguments does not modify files' '
+   git stash clear &&
+   test_when_finished "git reset --hard HEAD" &&
+   echo foo >file2 &&
+   git stash &&
+   echo bar >file2 &&
+   git stash &&
+   test-tool chmtime =123456789 file2 &&
+   for type in apply pop "branch stash-branch"
+   do
+   test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many revisions" err &&
+   test 123456789 = $(test-tool chmtime -g file2) || return 1
+   done
+'
+
+test_expect_success 'drop: too many arguments errors out (does nothing)' '
+   git stash list >expect &&
+   test_must_fail git stash drop stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many revisions" err &&
+   git stash list >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'show: too many arguments errors out (does nothing)' '
+   test_must_fail git stash show stash@{0} stash@{1} 2>err 1>out &&
+   test_i18ngrep "Too many revisions" err &&
+   test_must_be_empty out
+'
+
 test_expect_success 'stash create - no changes' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
@@ -479,6 +509,11 @@ test_expect_success 'stash branch - stashes on stack, 
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
 '
 
+test_expect_success 'stash branch complains with no arguments' '
+   test_must_fail git stash branch 2>err &&
+   test_i18ngrep "No branch name specified" err
+'
+
 test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.19.1.878.g0482332a22



[PATCH 1/2] format-patch: add a more exhaustive --range-diff test

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Change the narrow test added in 31e2617a5f ("format-patch: add
--range-diff option to embed diff in cover letter", 2018-07-22) to
test the full output. This test would have spotted a regression in the
output if it wasn't beating around the bush and tested the full
output, let's do that.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t3206-range-diff.sh | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e497c1358f..0235c038be 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -249,11 +249,28 @@ for prev in topic master..topic
 do
test_expect_success "format-patch --range-diff=$prev" '
git format-patch --stdout --cover-letter --range-diff=$prev \
-   master..unmodified >actual &&
-   grep "= 1: .* s/5/A" actual &&
-   grep "= 2: .* s/4/A" actual &&
-   grep "= 3: .* s/11/B" actual &&
-   grep "= 4: .* s/12/B" actual
+   master..unmodified >actual.raw &&
+   sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
+   :1:  4de457d = 1:  35b9b25 s/5/A/
+   : a => b | 0
+   : 1 file changed, 0 insertions(+), 0 deletions(-)
+   ::
+   :2:  fccce22 = 2:  de345ab s/4/A/
+   : a => b | 0
+   : 1 file changed, 0 insertions(+), 0 deletions(-)
+   ::
+   :3:  147e64e = 3:  9af6654 s/11/B/
+   : a => b | 0
+   : 1 file changed, 0 insertions(+), 0 deletions(-)
+   ::
+   :4:  a63e992 = 4:  2901f77 s/12/B/
+   : a => b | 0
+   : 1 file changed, 0 insertions(+), 0 deletions(-)
+   ::
+   :-- :
+   EOF
+   sed -ne "/^1:/,/^--/p" actual &&
+   test_cmp expect actual
'
 done
 
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH 2/2] format-patch: don't include --stat with --range-diff output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Fix a regression introduced in my a48e12ef7a ("range-diff: make diff
option behavior (e.g. --stat) consistent", 2018-11-13). Since the
format-patch setup code implicitly sets --stat --summary by default,
we started emitting the --stat output in the cover letter's
range-diff.

As noted in df569c3f31 ("range-diff doc: add a section about output
stability", 2018-11-09) the --stat output is currently rather useless,
and just adds noise.

Perhaps we should detect if --stat or --summary were implicitly passed
to format-patch, and then pass them along, but I think fixing it this
way is fine. If our --stat output ever becomes useful in range-diff we
can revisit this.

There's still cases like e.g. --numstat triggering rather useless
range-diff output, but I think it's OK to just handle the default
case. Users are unlikely to produce a formatted patch with the likes
of --numstat, or indeed any other custom diff option except -U or
maybe -W. If they need weirder combinations of options they can always
manually produce the range-diff.

This whole situation comes about because we're assuming that when the
user passes along e.g. -U10 that they want that some 10-line context
for the range-diff as for the patches themselves. As noted in [1] I
think it's worth re-visiting this and making -U10 just apply to the
patches, and e.g. --range-diff-U10 to the range-diff. But that's left
as a topic for another series less close to a rc2.

1. https://public-inbox.org/git/87d0ri7gbs@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/log.c |  7 ++-
 t/t3206-range-diff.sh | 12 
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 0fe6f9ba1e..7cd2db0be9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1094,9 +1094,13 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
}
 
if (rev->rdiff1) {
+   struct diff_options opts;
+   memcpy(, >diffopt, sizeof(opts));
+   opts.output_format &= ~(DIFF_FORMAT_DIFFSTAT | 
DIFF_FORMAT_SUMMARY);
+
fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
show_range_diff(rev->rdiff1, rev->rdiff2,
-   rev->creation_factor, 1, >diffopt);
+   rev->creation_factor, 1, );
}
 }
 
@@ -1697,6 +1701,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
if (!use_patch_format &&
(!rev.diffopt.output_format ||
 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
+   /* Needs to be mirrored in show_range_diff() invocation */
rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | 
DIFF_FORMAT_SUMMARY;
if (!rev.diffopt.stat_width)
rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 0235c038be..90def330bd 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -252,21 +252,9 @@ do
master..unmodified >actual.raw &&
sed -e "s|^:||" -e "s|:$||" >expect <<-\EOF &&
:1:  4de457d = 1:  35b9b25 s/5/A/
-   : a => b | 0
-   : 1 file changed, 0 insertions(+), 0 deletions(-)
-   ::
:2:  fccce22 = 2:  de345ab s/4/A/
-   : a => b | 0
-   : 1 file changed, 0 insertions(+), 0 deletions(-)
-   ::
:3:  147e64e = 3:  9af6654 s/11/B/
-   : a => b | 0
-   : 1 file changed, 0 insertions(+), 0 deletions(-)
-   ::
:4:  a63e992 = 4:  2901f77 s/12/B/
-   : a => b | 0
-   : 1 file changed, 0 insertions(+), 0 deletions(-)
-   ::
:-- :
EOF
sed -ne "/^1:/,/^--/p" actual &&
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH 0/2] format-patch: pre-2.20 range-diff regression fix

2018-11-22 Thread Ævar Arnfjörð Bjarmason
[Dropping LKML & git-packagers from CC]

On Thu, Nov 22 2018, Eric Sunshine wrote:

> On Thu, Nov 22, 2018 at 10:58 AM Ævar Arnfjörð Bjarmason
>  wrote:
>> There's a regression related to this that I wanted to send a headsup
>> for, but don't have time to fix today. Now range-diff in format-patch
>> includes --stat output. See e.g. my
>> https://public-inbox.org/git/20181122132823.9883-1-ava...@gmail.com/
>
> Umf. Unfortunate fallout from [1].
>
>> diff --git a/builtin/log.c b/builtin/log.c
>> @@ -1094,9 +1094,12 @@ static void make_cover_letter(struct rev_info *rev, 
>> int use_stdout,
>> if (rev->rdiff1) {
>> +   const int oldfmt = rev->diffopt.output_format;
>> fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
>> +   rev->diffopt.output_format &= ~(DIFF_FORMAT_DIFFSTAT | 
>> DIFF_FORMAT_SUMMARY);
>> show_range_diff(rev->rdiff1, rev->rdiff2,
>> rev->creation_factor, 1, >diffopt);
>> +   rev->diffopt.output_format = oldfmt;
>> }
>>  }
>
> A few questions/observations:
>
> Does this same "fix" need to be applied to the --interdiff case just
> above this --range-diff block?
>
> Does the same "fix" need to be applied to the --interdiff and
> --range-diff cases in log-tree.c:show_log()?

No, that seems to do the right thing, but perhaps tests are lacking
there too. I haven't looked.

> Aside from fixing the broken --no-patches option[2], a goal of the
> series was to some day make --stat do something useful. Doesn't this
> "fix" go against that goal?

The goal was to fix the regression introduced in 73a834e9e2
("range-diff: relieve callers of low-level configuration burden",
2018-07-22). One aspect of having fixed that is we might in the future
do stuff with --stat.

> The way this change needs to be spread around at various locations is
> making it feel like a BandAid "fix" rather than a proper solution. It
> seems like it should be fixed at a different level, though I'm not
> sure yet if that level is higher (where the options get set) or lower
> (where they actually affect the operation).
>
> Until we figure out the answers to these questions, I wonder if a more
> sensible short-term solution would be to back out [1] and just keep
> [2], which fixed the --no-patches regression.

I think that patch leaves range-diff itself in a good state without
any bugs, and it would be a mistake to revert it.

But this interaction with format-patch --range-diff is another
matter. As noted in 2/2 I think in practice this series sweeps the
current bugs under the rug.

But as also noted there I think re-using what we get from
setup_revisions() in format-patch for the range-diff was a mistake,
and is always going to lead to some confusion. It should be split up
so we can supply those diff options independently.

> [...]
> [1]: https://public-inbox.org/git/20181113185558.23438-4-ava...@gmail.com/
> [2]: https://public-inbox.org/git/20181113185558.23438-3-ava...@gmail.com/

Ævar Arnfjörð Bjarmason (2):
  format-patch: add a more exhaustive --range-diff test
  format-patch: don't include --stat with --range-diff output

 builtin/log.c |  7 ++-
 t/t3206-range-diff.sh | 15 ++-
 2 files changed, 16 insertions(+), 6 deletions(-)

-- 
2.20.0.rc0.387.gc7a69e6b6c



CAN YOU HANDLE THIS?

2018-11-22 Thread Aya Ashraf




--
Greetings,

I wish to seek your assistance to legally move my late husband's estate 
to your country for investment and charity purposes.If interested,i 
will give you more details.Thanks.

Mrs.Aya Ashraf
--



[PATCH v1 1/1] t5601-99: Enable colliding file detection for MINGW

2018-11-22 Thread Carlo Marcelo Arenas Belón
Which FS was this tested on?, is Git LFS I keep hearing about also considered
a "filesystem" for git?

Could you also test with the following applied on top?

Carlo
-- >8 --
Subject: [PATCH] entry: remove windows fallback to inode checking

this test is really FS specific, so is better to avoid any compiled
assumptions about the platform and let the user drive the fallback
through core.checkStat instead

Signed-off-by: Carlo Marcelo Arenas Belón 
---
 entry.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/entry.c b/entry.c
index 0a3c451f5f..5ae74856e6 100644
--- a/entry.c
+++ b/entry.c
@@ -404,10 +404,6 @@ static void mark_colliding_entries(const struct checkout 
*state,
 {
int i, trust_ino = check_stat;
 
-#if defined(GIT_WINDOWS_NATIVE) || defined(__CYGWIN__)
-   trust_ino = 0;
-#endif
-
ce->ce_flags |= CE_MATCHED;
 
for (i = 0; i < state->istate->cache_nr; i++) {
-- 
2.20.0.rc1



Re: [PATCH v3 00/10] commit-graph write: progress output improvements

2018-11-22 Thread Eric Sunshine
On Thu, Nov 22, 2018 at 8:28 AM Ævar Arnfjörð Bjarmason
 wrote:
> Range-diff:
> By the way, is there any way to
> Pass the equivalent of "git range-diff origin/master topic-2 topic-3"
> to git-format-patch?

git-range-diff documentations says that the three-argument form:

git range-diff   

is equivalent to passing two ranges:

git range-diff .. ..

git-format-patch synopsis shows:

git format-patch --range-diff= 

where  is the range of commits to format, and 
can be a range specifying the previous version, so:

git format-patch --range-diff=.. ..

should do what you ask.

However, since the two versions in your example both derive from
origin/master, you should be able to get by with the simpler:

git format-patch --range-diff= ..

which, if you were running git-range-diff manually, would be the equivalent of:

git range-diff ...

for which the range-diff machinery figures out the common base
(origin/master) automatically.


Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

2018-11-22 Thread Ævar Arnfjörð Bjarmason


On Thu, Nov 22 2018, Jeff King wrote:

> On Wed, Nov 21, 2018 at 11:48:14AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Wed, Nov 21 2018, Junio C Hamano wrote:
>>
>> > * jk/loose-object-cache (2018-11-13) 9 commits
>> >   (merged to 'next' on 2018-11-18 at 276691a21b)
>> >  + fetch-pack: drop custom loose object cache
>> >  + sha1-file: use loose object cache for quick existence check
>> >  + object-store: provide helpers for loose_objects_cache
>> >  + sha1-file: use an object_directory for the main object dir
>> >  + handle alternates paths the same as the main object dir
>> >  + sha1_file_name(): overwrite buffer instead of appending
>> >  + rename "alternate_object_database" to "object_directory"
>> >  + submodule--helper: prefer strip_suffix() to ends_with()
>> >  + fsck: do not reuse child_process structs
>> >
>> >  Code clean-up with optimization for the codepath that checks
>> >  (non-)existence of loose objects.
>> >
>> >  Will cook in 'next'.
>>
>> I think as noted in
>> https://public-inbox.org/git/e5148b8c-9a3a-5d2e-ac8c-3e536c0f2...@web.de/
>> that we should hold off the [89]/9 of this series due to the performance
>> regressions this introduces in some cases (while fixing other cases).
>>
>> I hadn't had time to follow up on that, and figured it could wait until
>> post-2.20 for a re-roll.
>
> Yeah, my intent had been to circle back around to this, but I just
> hadn't gotten to it. I'm still pondering a config option or similar,
> though I remain unconvinced that the cases in which you've showed it
> being slow are actually realistic or worth worrying about

FWIW those "used to be 2ms are now 20-40ms" pushes on ext4 are
representative of the actual prod setup I'm mainly targeting. Now, I
don't run on ext4 this patch helps there, but it seems plausible that it
matters to someone who's counting on that performance.

Buh yeah, it's certainly obscure. I don't blame you if you don't want to
hack on it, and not ejecting this out before 2.20 isn't going to break
anything for me. But do you mind if I make it configurable as part of my
post-2.20 "disable collisions?"

>  (and certainly having an obscure config option is not enough to help
> most people). If we could have it kick in heuristically, that would be
> better.

Aside from this specific scenario. I'd really prefer if we avoid having
heuristic performance optimizations at all costs.

Database servers tend to do that sort of thing with their query planner,
and it results in cases where your entire I/O profile changes overnight
because you're now on the wrong side of some if/else heuristic about
whather to use some index or not.

> However, note that the cache-load for finding abbreviations _must_ have
> the complete list. And has been loading it for some time. So if you run
> "git-fetch", for example, you've already been running this code for
> months (and using the cache in more places is now a free speedup).

This is reminding me that I need to get around to re-submitting my
core.validateAbbrev series, which addresses this part of the problem:
https://public-inbox.org/git/20180608224136.20220-21-ava...@gmail.com/

> At the very least, we'd want this patch on top, too. I also think René's
> suggestion use access() is worth pursuing (though to some degree is
> orthogonal to the cache).

I haven't had time to test that, and wasn't prioritizing it since I
figured this was post-2.20. My hunch is it doesn't matter much if at all
on NFS. The roundtrip time is what matters, whether that roundtrip is
fstat() or access() probably not.

> -- >8 --
> Subject: [PATCH] odb_load_loose_cache: fix strbuf leak
>
> Commit 66f04152be (object-store: provide helpers for
> loose_objects_cache, 2018-11-12) moved the cache-loading code from
> find_short_object_filename(), but forgot the line that releases the path
> strbuf.
>
> Reported-by: René Scharfe 
> Signed-off-by: Jeff King 
> ---
>  sha1-file.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sha1-file.c b/sha1-file.c
> index 5894e48ea4..5a272f70de 100644
> --- a/sha1-file.c
> +++ b/sha1-file.c
> @@ -2169,6 +2169,7 @@ void odb_load_loose_cache(struct object_directory *odb, 
> int subdir_nr)
>   NULL, NULL,
>   >loose_objects_cache);
>   odb->loose_objects_subdir_seen[subdir_nr] = 1;
> + strbuf_release();
>  }
>
>  static int check_stream_sha1(git_zstream *stream,


[PATCH v1 1/1] t5601-99: Enable colliding file detection for MINGW

2018-11-22 Thread tboegi
From: Torsten Bögershausen 

Commit b878579ae7 (clone: report duplicate entries on case-insensitive
filesystems - 2018-08-17) adds a warning to user when cloning a repo
with case-sensitive file names on a case-insensitive file system.

This test has never been enabled for MINGW.
It had been working since day 1, but I forget to report that to the
author.
Enable it after a re-test.

Signed-off-by: Torsten Bögershausen 
---

The other day, I wanted to test Duys patch -
under MINGW - to see if the problem is catch(ed)
but hehe git am failed to apply - not a big desaster,
because is is already in master
Here is a follow-up, end we can end the match


 t/t5601-clone.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index c28d51bd59..8bbc7068ac 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -628,7 +628,7 @@ test_expect_success 'clone on case-insensitive fs' '
)
 '
 
-test_expect_success !MINGW,CASE_INSENSITIVE_FS 'colliding file detection' '
+test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' '
grep X icasefs/warning &&
grep x icasefs/warning &&
test_i18ngrep "the following paths have collided" icasefs/warning
-- 
2.19.0.271.gfe8321ec05



Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

2018-11-22 Thread Jeff King
On Wed, Nov 21, 2018 at 11:48:14AM +0100, Ævar Arnfjörð Bjarmason wrote:

> 
> On Wed, Nov 21 2018, Junio C Hamano wrote:
> 
> > * jk/loose-object-cache (2018-11-13) 9 commits
> >   (merged to 'next' on 2018-11-18 at 276691a21b)
> >  + fetch-pack: drop custom loose object cache
> >  + sha1-file: use loose object cache for quick existence check
> >  + object-store: provide helpers for loose_objects_cache
> >  + sha1-file: use an object_directory for the main object dir
> >  + handle alternates paths the same as the main object dir
> >  + sha1_file_name(): overwrite buffer instead of appending
> >  + rename "alternate_object_database" to "object_directory"
> >  + submodule--helper: prefer strip_suffix() to ends_with()
> >  + fsck: do not reuse child_process structs
> >
> >  Code clean-up with optimization for the codepath that checks
> >  (non-)existence of loose objects.
> >
> >  Will cook in 'next'.
> 
> I think as noted in
> https://public-inbox.org/git/e5148b8c-9a3a-5d2e-ac8c-3e536c0f2...@web.de/
> that we should hold off the [89]/9 of this series due to the performance
> regressions this introduces in some cases (while fixing other cases).
> 
> I hadn't had time to follow up on that, and figured it could wait until
> post-2.20 for a re-roll.

Yeah, my intent had been to circle back around to this, but I just
hadn't gotten to it. I'm still pondering a config option or similar,
though I remain unconvinced that the cases in which you've showed it
being slow are actually realistic or worth worrying about (and certainly
having an obscure config option is not enough to help most people). If
we could have it kick in heuristically, that would be better.

However, note that the cache-load for finding abbreviations _must_ have
the complete list. And has been loading it for some time. So if you run
"git-fetch", for example, you've already been running this code for
months (and using the cache in more places is now a free speedup).

At the very least, we'd want this patch on top, too. I also think René's
suggestion use access() is worth pursuing (though to some degree is
orthogonal to the cache).

-- >8 --
Subject: [PATCH] odb_load_loose_cache: fix strbuf leak

Commit 66f04152be (object-store: provide helpers for
loose_objects_cache, 2018-11-12) moved the cache-loading code from
find_short_object_filename(), but forgot the line that releases the path
strbuf.

Reported-by: René Scharfe 
Signed-off-by: Jeff King 
---
 sha1-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sha1-file.c b/sha1-file.c
index 5894e48ea4..5a272f70de 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -2169,6 +2169,7 @@ void odb_load_loose_cache(struct object_directory *odb, 
int subdir_nr)
NULL, NULL,
>loose_objects_cache);
odb->loose_objects_subdir_seen[subdir_nr] = 1;
+   strbuf_release();
 }
 
 static int check_stream_sha1(git_zstream *stream,
-- 
2.20.0.rc1.703.g93fba25b62



Re: [PATCH 6/9] sha1-file: use an object_directory for the main object dir

2018-11-22 Thread Jeff King
On Mon, Nov 12, 2018 at 11:04:52AM -0800, Stefan Beller wrote:

> >   for (odb = the_repository->objects->odb; odb; odb = odb->next) {
> > if (odb->local)
> > return odb->path;
> >   }
> >   return NULL; /* yikes? */
> >
> > ? That feels like it's making things more complicated, not less.
> 
> It depends if the caller cares about the local flag.
> 
> I'd think we can have more than one local, eventually?
> Just think of the partial clone stuff that may have a local
> set of promised stuff and another set of actual objects,
> which may be stored in different local odbs.

Yeah, but I think the definition of "local" gets very tricky there, and
we'll have to think about what it means. So I'd actually prefer to punt
on doing anything too clever at this point.

> If the caller cares about the distinction, they would need
> to write out this loop as above themselves.
> If they don't care, we could migrate them to not
> use this function, so we can get rid of it?

Yes, I do think in the long run we'd want to get rid of most calls to
get_object_directory(). Not only because it uses the_repository, but
because most callers should be asking for a specific action: I want to
write an object, or I want to read an object.

-Peff


Re: how does "clone --filter=sparse:path" work?

2018-11-22 Thread Jeff King
On Thu, Nov 08, 2018 at 01:57:52PM -0500, Jeff Hostetler wrote:

> > Should we simply be disallowing sparse:path filters over upload-pack?
> 
> The option to allow an absolute path over the wire probably needs more
> thought as you suggest.
> 
> Having it in the traverse code was useful for local testing in the
> client.
> 
> But mainly I was thinking of a use case on the client of the form:
> 
> git rev-list
> --objects
> --filter=spec:path=.git/sparse-checkout
> --missing=print
> 
> 
> and get a list of the blobs that you don't have and would need before
> you could checkout  using the current sparse-checkout definition.
> You could then have a pre-checkout hook that would bulk
> fetch them before starting the actual checkout.  Since that would be
> more efficient than demand-loading blobs individually during the
> checkout.  There's more work to do in this area, but that was the idea.
> 
> But back to your point, yes, I think we should restrict this over the
> wire.

Thanks for your thorough response, and sorry for the slow reply. I had
meant to reply with a patch adding in the restriction, but I haven't
quite gotten to it. :)

It's still on my todo list, but I'm going to be offline for a bit for
vacation, and I didn't want to leave this totally hanging without a
response.

-Peff


Re: insteadOf and git-request-pull output

2018-11-22 Thread Jeff King
On Sat, Nov 17, 2018 at 11:07:32PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I suspect it would be less confusing if the rewrite were inverted, like:
> >
> >   [url "gh:"]
> >   rewriteTo = https://github.com
> >   rewritePrivate
> >
> >   [url "git://github.com"]
> >   rewriteTo = https://github.com
> >
> > where the mapping of sections to rewrite rules must be one-to-one, not
> > one-to-many (and you can see that the flip side is that we have to
> > repeat ourselves).
> >
> > I hate to introduce two ways of doing the same thing, but maybe it is
> > simple enough to explain that url.X.insteadOf=Y is identical to
> > url.Y.rewriteTo=X. I do think people have gotten confused by the
> > ordering of insteadOf over the years, so this would let them specify it
> > in whichever way makes the most sense to them.
> 
> I do admit that the current "insteadOf" was too cute a way to
> configure this feature and I often have to read it aloud twice
> before convincing myself I got X and Y right.  It would have been
> cleaner if the definition were in the opposite direction, exactly
> because you can rewrite a single thing into just one way, but we do
> want to support that many things mapping to the same, and the
> approach to use "url.Y.rewriteTo=X" does express it better.

In case anybody is interested in picking this up, the code is actually
quite simple (the underlying data structure remains the same; we're just
inverting the order in the config). It would need documentation and
tests, and probably a matching pushRewriteTo.

diff --git a/remote.c b/remote.c
index 28c7260ed1..26b1a7b119 100644
--- a/remote.c
+++ b/remote.c
@@ -345,6 +345,11 @@ static int handle_config(const char *key, const char 
*value, void *cb)
return config_error_nonbool(key);
rewrite = make_rewrite(_push, name, namelen);
add_instead_of(rewrite, xstrdup(value));
+   } else if (!strcmp(subkey, "rewriteto")) {
+   if (!value)
+   return config_error_nonbool(key);
+   rewrite = make_rewrite(, value, strlen(value));
+   add_instead_of(rewrite, xmemdupz(name, namelen));
}
}
 

However, I did notice the cleanup below as part of writing that. It may
be worth applying independently.

-- >8 --
Subject: [PATCH] remote: check config validity before creating rewrite struct

When parsing url.foo.insteadOf, we call make_rewrite() and only then
check to make sure the config value is a string (and return an error if
it isn't). This isn't quite a leak, because the struct we allocate is
part of a global array, but it does leave a funny half-finished struct.

In practice, it doesn't make much difference because we exit soon after
due to the config error anyway. But let's flip the order here to avoid
leaving a trap for somebody in the future.

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

diff --git a/remote.c b/remote.c
index b850f2feb3..28c7260ed1 100644
--- a/remote.c
+++ b/remote.c
@@ -336,14 +336,14 @@ static int handle_config(const char *key, const char 
*value, void *cb)
if (!name)
return 0;
if (!strcmp(subkey, "insteadof")) {
-   rewrite = make_rewrite(, name, namelen);
if (!value)
return config_error_nonbool(key);
+   rewrite = make_rewrite(, name, namelen);
add_instead_of(rewrite, xstrdup(value));
} else if (!strcmp(subkey, "pushinsteadof")) {
-   rewrite = make_rewrite(_push, name, namelen);
if (!value)
return config_error_nonbool(key);
+   rewrite = make_rewrite(_push, name, namelen);
add_instead_of(rewrite, xstrdup(value));
}
}
-- 
2.20.0.rc1.703.g93fba25b62



Re: Git for Windows v2.20.0-rc0

2018-11-22 Thread Johannes Schindelin
Hi Stefan,

On Thu, 22 Nov 2018, stefan.na...@atlas-elektronik.com wrote:

> Just a quick note:
> 
> I installed v2.20.0-rc0 with these options:
> 
> $ cat /etc/install-options.txt
> Editor Option: VIM
> Custom Editor Path:
> Path Option: Cmd
> SSH Option: OpenSSH
> CURL Option: OpenSSL
> CRLF Option: CRLFCommitAsIs
> Bash Terminal Option: MinTTY
> Performance Tweaks FSCache: Enabled
> Use Credential Manager: Enabled
> Enable Symlinks: Disabled
> Enable Builtin Rebase: Enabled
> Enable Builtin Stash: Enabled
> 
> 
> 
> When starting the git bash two windows pop up instead of one.
> One that's "empty" and the other one containing the real git bash.

Thank you for the report. This has been also reported at
https://github.com/git-for-windows/git/issues/1942, and I fixed it in the
meantime.

Ciao,
Johannes

> 
> Thanks,
>   Stefan
> 
> Am 20.11.2018 um 21:56 schrieb Johannes Schindelin:
> > Team,
> > 
> > On Sun, 18 Nov 2018, Junio C Hamano wrote:
> > 
> >> An early preview release Git v2.20.0-rc0 is now available for
> >> testing at the usual places.  It is comprised of 887 non-merge
> >> commits since v2.19.0, contributed by 71 people, 23 of which are
> >> new faces.
> > 
> > The "for Windows" flavor of Git v2.20.0-rc0 is available here:
> > 
> > https://github.com/git-for-windows/git/releases/tag/v2.20.0-rc0.windows.1
> > 
> > The current change log for v2.20.0 reads like this:
> > 
> > Changes since Git for Windows v2.19.1 (Oct 5th 2018)
> > 
> > Please note: Git CMD is deprecated as of this Git for Windows version. The
> > default is to have git.exe in the PATH anyway, so there is no noticeable
> > difference between CMD and Git CMD. It is impossible to turn off CMD's
> > behavior where it picks up any git.exe in the current directory, so let's
> > discourage the use of Git CMD. Users who dislike Git Bash should switch to
> > Powershell instead.
> > 
> > New Features
> > 
> >   • Comes with OpenSSH v7.9p1.
> >   • The description of the editor option to choose Vim has been clarified
> > to state that this unsets core.editor.
> >   • Comes with cURL v7.62.0.
> >   • The type of symlinks to create (directory or file) can now be
> > specified via the .gitattributes.
> >   • The FSCache feature now uses a faster method to enumerate files,
> > making e.g. git status faster in large repositories.
> >   • Comes with Git Credential Manager v1.18.3.
> >   • Comes with Git LFS v2.6.0.
> >   • Comes with MSYS2 runtime (Git for Windows flavor) based on Cygwin
> > 2.11.2.
> >   • The FSCache feature was optimized to become faster.
> > 
> > Bug Fixes
> > 
> >   • The 64-bit Portable Git no longer sets pack.packSizeLimit.
> > 
> > Thanks,
> > Johannes
> > 
> 
> 
> -- 
> 
> /dev/random says: To save trouble later, Joe named his cat Roadkill Fred
> python -c "print 
> '73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
>  
> GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF

Re: [PATCH 1/3] pack-objects: fix tree_depth and layer invariants

2018-11-22 Thread Jeff King
On Tue, Nov 20, 2018 at 05:37:18PM +0100, Duy Nguyen wrote:

> > But in (b), we use the number of stored objects, _not_ the allocated
> > size of the objects array. So we can run into a situation like this:
> >
> >   1. packlist_alloc() needs to store the Nth object, so it grows the
> >  objects array to M, where M > N.
> >
> >   2. oe_set_tree_depth() wants to store a depth, so it allocates an
> >  array of length N. Now we've violated our invariant.
> >
> >   3. packlist_alloc() needs to store the N+1th object. But it _doesn't_
> >  grow the objects array, since N <= M still holds. We try to assign
> >  to tree_depth[N+1], which is out of bounds.
> 
> Do you think if this splitting data to packing_data is too fragile
> that we should just scrape the whole thing and move all data back to
> object_entry[]? We would use more memory of course but higher memory
> usage is still better than more bugs (if these are likely to show up
> again).

Certainly that thought crossed my mind while working on these patches. :)

Especially given the difficulties it introduced into the recent
bitmap-reuse topic, and the size fixes we had to deal with in v2.19.

Overall, though, I dunno. This fix, while subtle, turned out not to be
too complicated. And the memory savings are real. I consider 100M
objects to be on the large size of feasible for stock Git these days,
and I think we are talking about on the order of 4GB memory savings
there. You need a big machine to handle a repository of that size, but
4GB is still appreciable.

So I guess at this point, with all (known) bugs fixed, we should stick
with it for now. If it becomes a problem for development of a future
feature, then we can re-evaluate then.

-Peff


Re: [PATCH 0/3] delta-island fixes

2018-11-22 Thread Jeff King
On Tue, Nov 20, 2018 at 10:06:57AM -0500, Derrick Stolee wrote:

> On 11/20/2018 4:44 AM, Jeff King wrote:
> > In cases like this I think it's often a good idea to have a perf test.
> > Those are expensive anyway, and we'd have the double benefit of
> > exercising the code and showing off the performance improvement. But the
> > delta-island code only makes sense on a very specialized repo: one where
> > you have multiple related but diverging histories.  You could simulate
> > that by picking two branches in a repository, but the effect is going to
> > be miniscule.
> 
> The changes in this series look correct. Too bad it is difficult to test.
> 
> Perhaps we should add a performance test for the --delta-islands check that
> would trigger the failure (and maybe a clone afterwards). There are lots of
> freely available forks of git.git that present interesting fork structure.
> Here are three that would suffice to make this interesting:
> 
>     https://github.com/git/git
>     https://github.com/git-for-windows/git
>     https://github.com/microsoft/git
> 
> Of course, running it on a specific repo is up to the person running the
> perf suite.

I hadn't really considered the possibility of reconstructing a fork
network repository from public repos. It probably would be possible to
include a script which does so, although:

  - I suspect it's going to be pretty expensive. We can use --reference
to reduce the size of subsequent clones, but just the repacks you
have to do to assemble the final shared repo can get pretty
expensive.

  - That's 3 forks. Our real-world case has over 4000. I'm not sure of
the size of the effects for smaller cases.

So in short, I think it's an interesting avenue to explore, but it might
turn out to be a dead-end. I'm not going to prioritize it right now, but
if somebody wants to play with it, I'd be happy to look over the
results.

-Peff


Re: Git for Windows v2.20.0-rc0, was Re: [ANNOUNCE] Git v2.20.0-rc0

2018-11-22 Thread Jeff King
On Wed, Nov 21, 2018 at 11:28:28AM -0800, Bryan Turner wrote:

> But that test code exists because Bitbucket Server provides a Java API
> [1][2] which allows third-party developers to easily build arbitrary
> Git commands to invoke for their own functionality. Setting
> `GitBranchCreateBuilder.reflog(true)` will trigger adding "-l" to the
> assembled "git branch" command. I've changed the code now so that it
> will use "--create-reflog" instead; however, because many of the
> Bitbucket Server add-ons on Marketplace [3], whether free or paid, are
> not open source, and because there are a significant number of
> in-house plugins that are not listed there, it's difficult to know how
> many might be affected. If I were to hazard a guess it would be
> _none_, but I've been surprised before. The end result is that the net
> impact is hard to predict--especially because Git on the server would
> need to be upgraded to 2.20.

Yeah, that is slightly worrisome. In some sense we can guess that
calling ".reflog(true)" is in the same league as our assumption of
"probably nobody is using -l in the first place", but it's one more
place that might have encouraged people into using it, even if it's a
noop.

> (In case you're curious why we used shorthand options, it's because of
> our Windows support. While "git branch" commands rarely, if ever, get
> very long, as a general rule we use shorthand options where they exist
> to keep our command lines shorter, to allow passing more options
> without hitting the hard limit (generally 32K) imposed by
> Windows--something we _have_ had issues with on other commands. For
> commands like "git diff", where it's not possible to pass in paths via
> stdin, every character matters.)

I follow your reasoning, though that is not the engineering decision I
would have made. The long-names tend to be more robust, and while saving
a few bytes might make a case work that would not otherwise, it is
really only delaying the inevitable. But hey, it's not my product. ;)

We probably should support --stdin in more places to cover situations
like this. Patches, welcome.

Also, I have noticed that performance with a large number of pathspecs
tends to be pretty poor, as we search them linearly. I wonder if you've
run into that, too (or maybe, coming from a Java world, you simply have
a small number of extremely long path names ;) ). A while ago I
experimented with putting non-wildcard pathspecs into a trie structure
that we could traverse while walking the actual trees to diff. I never
really polished it because having a huge number of pathspecs didn't seem
like a common use case.

> To try and protect against the unexpected, we have a Supported
> Platforms [4] page which lists Git versions that we've _explicitly
> tested_ with Bitbucket Server. 2.20 won't be marked tested until a
> future release, so the majority of installs will not use it with older
> versions of the system--but there's always that subset who ignore the
> documentation. (Since we do text parsing on Git output, subtle
> breakages do happen from time to time.)
> 
> I would say it's _unlikely_ that we'll hear of any installations where
> all the conditions are met for this to come up:
> - Git 2.20
> - Bitbucket Server (without fixes)
> - Third-party add-on using `reflog(true)`

Thanks for laying this out (and again, thanks for testing and reporting
this before the release!).

I'm comfortable with continuing with the change in v2.20 at this point,
but I'm also totally fine with bumping it for another release. Your case
is about Bitbucket, but there may well be other scripts in the wild.

> It's really just that a) I was caught off guard by the change (my own
> fault for not reading the 2.19 announcement more carefully) and b)
> it's impossible for me to say with _certainty_ that it won't be an
> issue. I'd imagine that latter point is true of the change in general,
> though (it's not really possible to know what scripts it might break,
> and that's going to be true regardless of when the change actually
> gets released), and I'd agree that that shouldn't hold Git back from
> making useful improvements.

The advantage of bumping is that if you switch Bitbucket to
"--create-reflog" _now_, it's more likely to be deployed by the time the
Git change comes.

In theory that also helps people who may not have upgraded to v2.19 yet,
but I suspect in many cases people don't notice the warning at all
(because it's buried in some script output) and will only notice once
the breaking change ships. Then the deprecation period only gives us a
larger ability to say "I told you so...", but that is small satisfaction
to the person whose script was broken.

-Peff


Re: [PATCH v1 1/2] log -G: Ignore binary files

2018-11-22 Thread Jeff King
On Thu, Nov 22, 2018 at 11:16:38AM +0100, Ævar Arnfjörð Bjarmason wrote:

> > +test_expect_success 'log -G looks into binary files with textconv filter' '
> > +   rm -rf .git &&
> > +   git init &&
> > +   echo "* diff=bin" > .gitattributes &&
> > +   printf "a\0b" >data.bin &&
> > +   git add data.bin &&
> > +   git commit -m "message" &&
> > +   git -c diff.bin.textconv=cat log -G a >actual &&
> > +   git log >expected &&
> > +   test_cmp actual expected
> > +'
> > +
> >  test_done
> 
> This patch seems like the wrong direction to me. In particular the
> assertion that "the concept of differences only makes sense for text
> files". That's just not true. This patch breaks this:

But "-G" is defined as "look for differences whose patch text contains
added/removed lines that match ". We don't have patch text here,
let alone added/removed lines.

For binary files, "-Sfoo" is better defined. I think we _could_ define
"search for  in the added/removed bytes of a binary file".  But
I don't think that's what the current code does (it really does a line
diff on a binary file, which is likely to put tons of unchanged crap
into the "added and removed" lines, because the line divisions aren't
meaningful in the first place).

> (
> rm -rf /tmp/g-test &&
> git init /tmp/g-test &&
> cd /tmp/g-test &&
> for i in {1..10}; do
> echo "Always matching thensome 5" >file &&
> printf "a thensome %d binary \0" $i >>file &&
> git add file &&
> git commit -m"Bump $i"
> done &&
> git log -Gthensome.*5
> )
> 
> Right now this will emit 3/10 patches, and the right ones! I.e. "Bump
> [156]". The 1st one because it introduces the "Always matching thensome
> 5". Then 5/6 because the add/remove the string "a thensome 5 binary",
> respectively. Which matches /thensome.*5/.

Right, this will sometimes do the right thing. But it will also often do
the wrong thing. It's also very expensive (we specifically avoid feeding
large binary files to xdiff, but I think "-G" will happily do so -- I
didn't double check, though).

-Peff


Re: [PATCH v1 1/2] log -G: Ignore binary files

2018-11-22 Thread Jeff King
On Wed, Nov 21, 2018 at 09:52:27PM +0100, Thomas Braun wrote:

> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 69fc55ea1e..8c2558b07d 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct 
> diff_options *o,
>   textconv_two = get_textconv(o->repo->index, p->two);
>   }
>  
> + if ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&
> + ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||
> +  (!textconv_two && diff_filespec_is_binary(o->repo, p->two
> + return 0;

If the user passes "-a" to treat binary files as text, we should
probably skip the binary check. I think we'd need to check
"o->flags.text" here.

> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 844df760f7..42cc8afd8b 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing 
> textconv tool)' '
> [...]
> +test_expect_success 'log -G ignores binary files' '
> [...]
> +test_expect_success 'log -G looks into binary files with textconv filter' '

And likewise add a test here similar to the textconv one.

-Peff


Re: [PATCH] t5562: skip if NO_CURL is enabled

2018-11-22 Thread Jeff King
On Thu, Nov 22, 2018 at 02:17:01AM -0800, Carlo Arenas wrote:

> Peff, could you elaborate on your "load testing" setup? which could
> give us any hints
> on what to look for?, FWIW I hadn't been able to reproduce the problem 
> anywhere
> else (and not for a lack of trying)

The script I use is at:

  https://github.com/peff/git/blob/meta/stress

which you invoke like "/path/to/stress t5562" from the top-level of a
git.git checkout.  It basically just runs a loop of twice as many
simultaneous invocations of the test script as you have CPUs, and waits
for one to fail. The load created by all of the runs tends to flush out
timing effects after a while.

It fails for me on t5562 within 30 seconds or so (but note that in this
particular case it sometimes takes a while to produce the final output
because invoke-with-content-length misses the expected SIGCLD and sleeps
the full 60 seconds).

You'll probably need to tweak the variables at the top of the script for
your system.

> PS. upstreaming the PERL_PATH fix is likely to be good to do soonish
> as I presume at least all BSD might be affected, let me know if you
> would rather me do that instead as I suspect we might be deadlocked
> otherwise ;)

Yeah, the $PERL_PATH thing is totally orthogonal, and should graduate
separately.

-Peff


Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

2018-11-22 Thread Jeff King
On Thu, Nov 22, 2018 at 01:48:53PM +0100, Johannes Schindelin wrote:

> > So it's maybe do-able, but not quite as trivial as one might hope.
> 
> A trivial alternative would be to recommend adding a man page for
> 3rd-party git-s.
> 
> In other words, as soon as `git-sizer` is accompanied by `git-sizer.1` in
> one of the appropriate locations, it is set.

Yes, it would be nice if everything did ship with a manpage.
Unfortunately that complicates installation, where the installation for
many such tools is "save this file to your $PATH".

Tools like git-sizer may be getting big and popular enough to merit the
extra infrastructure, but I think there are many smaller scripts which
don't.

> FWIW I do have a couple of scripts I use that I install into
> `$HOME/bin/git-`. Although, granted, I essentially switched to
> aliases for most of them, where the aliases still call a script that is
> checked out in some folder in my home directory. The reason: this works in
> more circumstances, as I do not have to add `$HOME/bin` to the `PATH`,
> say, in PowerShell.
> 
> So YMMV with git-s. My rule of thumb is: if I want to use this
> myself only, I'll make it an alias. If I want to ship it (e.g. with Git
> for Windows), I'll make it a git-.

I have a handful of personal git-* scripts: mostly ones that are big
enough to be unwieldy as an alias. But then, $PATH management is pretty
straightforward on my platforms, so it's easier to drop a script there
than to point an alias to a non-git-* script.

-Peff


Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

2018-11-22 Thread Linus Torvalds
On Wed, Nov 21, 2018 at 6:08 PM Stephen P. Smith  wrote:
>
> Picking up someones stalled patch is one thing, picking up Linus' patch is in
> a different league.

No, I think it works the other way - my random patches are likely the
_least_ important ones, simply because I can carry them around in my
own tree anyway, and my workflows may be odd by most git standards.

If I'm the only user of the "human" date format, then Junio is
absolutely right to not move it to the main branch.

I have some other private patches in my tree that have been mentioned
on the list but that nobody else got excited about:

 - Add 'human' date mode

 - Make 'git gc --prune=now' mean 'prune after start of command'

 - ls-remote: add "--diff" option to show only refs that differ

none of which are in the least important, but that I find personally useful.

  Linus


Re: Document change in format of raw diff output format

2018-11-22 Thread Jeff King
On Thu, Nov 22, 2018 at 11:58:36AM +0100, Greg Hurrell wrote:

> I was troubleshooting some breakage in some code that consumes the
> output of `git log --raw` and looking on two machines with different
> versions of Git just now I discovered the output format has changed
> somewhere between v2.14.5:
> 
> :00 100644 0... 9773b7718... A  content/snippets/1157.md
> 
> and v2.19.0:
> 
> :00 100644 0 9773b7718 Acontent/snippets/1157.md
> 
> A quick search turns up some patches related to the
> GIT_PRINT_SHA1_ELLIPSIS env variable, which can be used to force the
> old output format, and which landed in v2.16.0, I think.

Yes. The actual commit that flipped the default is 7cb6ac1e4b (diff:
diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value,
2017-12-03). There's more discussion of the possibility of breakage in
this subthread:

  https://public-inbox.org/git/83D263E58ABD46188756D41FE311E469@PhilipOakley/

> Does it sound right that we should update the documentation in
> diff-format.txt to show what the new output format is? The examples
> all show the old output format, which isn't produced by default any
> more.

Yes, we should definitely update the documentation to show the modern
format. I think that was just an oversight in the original series.

> diff --git a/Documentation/diff-format.txt b/Documentation/diff-format.txt
> index 706916c94c..33776459d0 100644
> --- a/Documentation/diff-format.txt
> +++ b/Documentation/diff-format.txt
> @@ -26,12 +26,12 @@ line per changed file.
>  An output line is formatted this way:
> 
>  
> -in-place edit  :100644 100644 bcd1234... 0123456... M file0
> -copy-edit  :100644 100644 abcd123... 1234567... C68 file1 file2
> -rename-edit:100644 100644 abcd123... 1234567... R86 file1 file3
> -create :00 100644 000... 1234567... A file4
> -delete :100644 00 1234567... 000... D file5
> -unmerged   :00 00 000... 000... U file6
> +in-place edit  :100644 100644 bcd123456 012345678 M file0
> +copy-edit  :100644 100644 abcd12345 123456789 C68 file1 file2
> +rename-edit:100644 100644 abcd12345 123456789 R86 file1 file3
> +create :00 100644 0 123456789 A file4
> +delete :100644 00 123456789 0 D file5
> +unmerged   :00 00 0 0 U file6
>  

Yeah, this looks like an improvement.

I think in general that we'd continue to show 7 characters now, just
without the extra dots (though it's auto-scaled based on the number of
objects in the repo these days, so it's not even really a constant).

>  That is, from the left to the right:
> @@ -75,7 +75,7 @@ and it is out of sync with the index.
>  Example:
> 
>  
> -:100644 100644 5be4a4.. 00.. M file.c
> +:100644 100644 5be4a4abc 0 M file.c
>  

I'm not even sure what this original was trying to show. I don't think
we ever produced that any dots. :)

Thanks for noticing.

-Peff

PS As you noticed, "git log" we don't promise that git-log output will
   never change between versions. For machine-consumption you probably
   want to use plumbing like "git rev-list | git diff-tree --stdin",
   which produces unabbreviated hashes.


Dear Friend, i know that this mail will come to you as a surprise as we have never met before, i am contacting you independently of my investigation and no one is informed of this communication. I nee

2018-11-22 Thread David Odo


[PATCH v4 01/10] commit-graph: rename 'num_extra_edges' variable to 'num_large_edges'

2018-11-22 Thread Ævar Arnfjörð Bjarmason
From: SZEDER Gábor 

The commit graph file format describes an optional 'Large Edge List'
chunk, and the function writing out this chunk is called
write_graph_chunk_large_edges().  Then there are two functions in
'commit-graph.c', namely write_graph_chunk_data() and
write_commit_graph(), which have a local variable called
'num_extra_edges'.

It can be confusing on first sight whether large edges and extra edges
refer to the same thing or not, but they do, so let's rename those
variables to 'num_large_edges'.

Signed-off-by: SZEDER Gábor 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 40c855f185..7b4e3a02cf 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -475,7 +475,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
 {
struct commit **list = commits;
struct commit **last = commits + nr_commits;
-   uint32_t num_extra_edges = 0;
+   uint32_t num_large_edges = 0;
 
while (list < last) {
struct commit_list *parent;
@@ -507,7 +507,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
if (!parent)
edge_value = GRAPH_PARENT_NONE;
else if (parent->next)
-   edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | 
num_extra_edges;
+   edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | 
num_large_edges;
else {
edge_value = sha1_pos(parent->item->object.oid.hash,
  commits,
@@ -521,7 +521,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
 
if (edge_value & GRAPH_OCTOPUS_EDGES_NEEDED) {
do {
-   num_extra_edges++;
+   num_large_edges++;
parent = parent->next;
} while (parent);
}
@@ -761,7 +761,7 @@ void write_commit_graph(const char *obj_dir,
uint32_t chunk_ids[5];
uint64_t chunk_offsets[5];
int num_chunks;
-   int num_extra_edges;
+   int num_large_edges;
struct commit_list *parent;
struct progress *progress = NULL;
 
@@ -871,7 +871,7 @@ void write_commit_graph(const char *obj_dir,
commits.alloc = count_distinct;
ALLOC_ARRAY(commits.list, commits.alloc);
 
-   num_extra_edges = 0;
+   num_large_edges = 0;
for (i = 0; i < oids.nr; i++) {
int num_parents = 0;
if (i > 0 && oideq([i - 1], [i]))
@@ -885,11 +885,11 @@ void write_commit_graph(const char *obj_dir,
num_parents++;
 
if (num_parents > 2)
-   num_extra_edges += num_parents - 1;
+   num_large_edges += num_parents - 1;
 
commits.nr++;
}
-   num_chunks = num_extra_edges ? 4 : 3;
+   num_chunks = num_large_edges ? 4 : 3;
 
if (commits.nr >= GRAPH_PARENT_MISSING)
die(_("too many commits to write graph"));
@@ -916,7 +916,7 @@ void write_commit_graph(const char *obj_dir,
chunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;
chunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;
chunk_ids[2] = GRAPH_CHUNKID_DATA;
-   if (num_extra_edges)
+   if (num_large_edges)
chunk_ids[3] = GRAPH_CHUNKID_LARGEEDGES;
else
chunk_ids[3] = 0;
@@ -926,7 +926,7 @@ void write_commit_graph(const char *obj_dir,
chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
chunk_offsets[2] = chunk_offsets[1] + GRAPH_OID_LEN * commits.nr;
chunk_offsets[3] = chunk_offsets[2] + (GRAPH_OID_LEN + 16) * commits.nr;
-   chunk_offsets[4] = chunk_offsets[3] + 4 * num_extra_edges;
+   chunk_offsets[4] = chunk_offsets[3] + 4 * num_large_edges;
 
for (i = 0; i <= num_chunks; i++) {
uint32_t chunk_write[3];
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v4 03/10] commit-graph write: rephrase confusing progress output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Rephrase the title shown for the progress output emitted by
close_reachable(). The message I added in 7b0f229222 ("commit-graph
write: add progress output", 2018-09-17) gave the impression that it
would count up to the number of commit objects.

But that's not what the number means. It just represents the work
we're doing in several for-loops to do various work before the graph
is written out. So let's just say "Annotating commit graph", that
title makes no such promises, and we can add other loops here in the
future and still consistently show progress output.

See [1] for the initial bug report & subsequent discussion about other
approaching to solving this.

1. https://public-inbox.org/git/20181015165447.gh19...@szeder.dev/

Reported-by: SZEDER Gábor 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 965eb23a7b..d11370a2b3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -648,7 +648,7 @@ static void close_reachable(struct packed_oid_list *oids, 
int report_progress)
 
if (report_progress)
progress = start_delayed_progress(
-   _("Annotating commits in commit graph"), 0);
+   _("Annotating commit graph"), 0);
for (i = 0; i < oids->nr; i++) {
display_progress(progress, ++j);
commit = lookup_commit(the_repository, >list[i]);
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v4 02/10] commit-graph: don't call write_graph_chunk_large_edges() unnecessarily

2018-11-22 Thread Ævar Arnfjörð Bjarmason
From: SZEDER Gábor 

The optional 'Large Edge List' chunk of the commit graph file stores
parent information for commits with more than two parents.  Since the
chunk is optional, write_commit_graph() looks through all commits to
find those with more than two parents, and then writes the commit
graph file header accordingly, i.e. if there are no such commits, then
there won't be a 'Large Edge List' chunk written, only the three
mandatory chunks.

However, when it comes to writing chunk data, write_commit_graph()
unconditionally invokes write_graph_chunk_large_edges(), even when it
was decided earlier that that chunk won't be written.  Strictly
speaking there is no bug here, because write_graph_chunk_large_edges()
won't write anything because it won't find any commits with more than
two parents, but then it unnecessarily and in vain looks through all
commits once again in search for such commits.

Don't call write_graph_chunk_large_edges() when that chunk won't be
written to spare an unnecessary iteration over all commits.

Signed-off-by: SZEDER Gábor 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 7b4e3a02cf..965eb23a7b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -940,7 +940,8 @@ void write_commit_graph(const char *obj_dir,
write_graph_chunk_fanout(f, commits.list, commits.nr);
write_graph_chunk_oids(f, GRAPH_OID_LEN, commits.list, commits.nr);
write_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);
-   write_graph_chunk_large_edges(f, commits.list, commits.nr);
+   if (num_large_edges)
+   write_graph_chunk_large_edges(f, commits.list, commits.nr);
 
close_commit_graph(the_repository);
finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v4 04/10] commit-graph write: add "Writing out" progress output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Add progress output to be shown when we're writing out the
commit-graph, this adds to the output already added in 7b0f229222
("commit-graph write: add progress output", 2018-09-17).

As noted in that commit most of the progress output isn't displayed on
small repositories, but before this change we'd noticeably hang for
2-3 seconds at the end on medium sized repositories such as linux.git.

Now we'll instead show output like this, and have no human-observable
point at which we're not producing progress output:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph: 6365442, done.
Annotating commit graph: 2391666, done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph: 100% (318/318), done.

This "Writing out" number is 3x or 4x the number of commits, depending
on the graph we're processing. A later change will make this explicit
to the user.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 48 +++-
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index d11370a2b3..dc57b8fedc 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -433,7 +433,9 @@ struct tree *get_commit_tree_in_graph(struct repository *r, 
const struct commit
 
 static void write_graph_chunk_fanout(struct hashfile *f,
 struct commit **commits,
-int nr_commits)
+int nr_commits,
+struct progress *progress,
+uint64_t *progress_cnt)
 {
int i, count = 0;
struct commit **list = commits;
@@ -447,6 +449,7 @@ static void write_graph_chunk_fanout(struct hashfile *f,
while (count < nr_commits) {
if ((*list)->object.oid.hash[0] != i)
break;
+   display_progress(progress, ++*progress_cnt);
count++;
list++;
}
@@ -456,12 +459,16 @@ static void write_graph_chunk_fanout(struct hashfile *f,
 }
 
 static void write_graph_chunk_oids(struct hashfile *f, int hash_len,
-  struct commit **commits, int nr_commits)
+  struct commit **commits, int nr_commits,
+  struct progress *progress,
+  uint64_t *progress_cnt)
 {
struct commit **list = commits;
int count;
-   for (count = 0; count < nr_commits; count++, list++)
+   for (count = 0; count < nr_commits; count++, list++) {
+   display_progress(progress, ++*progress_cnt);
hashwrite(f, (*list)->object.oid.hash, (int)hash_len);
+   }
 }
 
 static const unsigned char *commit_to_sha1(size_t index, void *table)
@@ -471,7 +478,9 @@ static const unsigned char *commit_to_sha1(size_t index, 
void *table)
 }
 
 static void write_graph_chunk_data(struct hashfile *f, int hash_len,
-  struct commit **commits, int nr_commits)
+  struct commit **commits, int nr_commits,
+  struct progress *progress,
+  uint64_t *progress_cnt)
 {
struct commit **list = commits;
struct commit **last = commits + nr_commits;
@@ -481,6 +490,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
struct commit_list *parent;
int edge_value;
uint32_t packedDate[2];
+   display_progress(progress, ++*progress_cnt);
 
parse_commit(*list);
hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
@@ -542,7 +552,9 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
 
 static void write_graph_chunk_large_edges(struct hashfile *f,
  struct commit **commits,
- int nr_commits)
+ int nr_commits,
+ struct progress *progress,
+ uint64_t *progress_cnt)
 {
struct commit **list = commits;
struct commit **last = commits + nr_commits;
@@ -550,6 +562,9 @@ static void write_graph_chunk_large_edges(struct hashfile 
*f,
 
while (list < last) {
int num_parents = 0;
+
+   display_progress(progress, ++*progress_cnt);
+
for (parent = (*list)->parents; num_parents < 3 && parent;
 parent = parent->next)
num_parents++;
@@ -764,6 +779,7 @@ void write_commit_graph(const char *obj_dir,
int num_large_edges;
struct commit_list *parent;
struct progress 

[PATCH v4 09/10] commit-graph write: add itermediate progress

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Add progress output to sections of code between "Annotating[...]" and
"Computing[...]generation numbers". This can collectively take 5-10
seconds on a large enough repository.

On a test repository with I have with ~7 million commits and ~50
million objects we'll now emit:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph among packed objects: 100% 
(48333911/48333911), done.
Annotating commit graph: 21435984, done.
Counting distinct commits in commit graph: 100% (7145328/7145328), done.
Finding extra edges in commit graph: 100% (7145328/7145328), done.
Computing commit graph generation numbers: 100% (7145328/7145328), done.
Writing out commit graph in 4 passes: 100% (28581312/28581312), done.

Whereas on a medium-sized repository such as linux.git these new
progress bars won't have time to kick in and as before and we'll still
emit output like:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph among packed objects: 100% 
(6365442/6365442), done.
Annotating commit graph: 2391666, done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph in 4 passes: 100% (318/318), done.

The "Counting distinct commits in commit graph" phase will spend most
of its time paused at "0/*" as we QSORT(...) the list. That's not
optimal, but at least we don't seem to be stalling anymore.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 199155bd68..80f201adf4 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -889,12 +889,19 @@ void write_commit_graph(const char *obj_dir,
 
close_reachable(, report_progress);
 
+   if (report_progress)
+   progress = start_delayed_progress(
+   _("Counting distinct commits in commit graph"),
+   oids.nr);
+   display_progress(progress, 0); /* TODO: Measure QSORT() progress */
QSORT(oids.list, oids.nr, commit_compare);
count_distinct = 1;
for (i = 1; i < oids.nr; i++) {
+   display_progress(progress, i + 1);
if (!oideq([i - 1], [i]))
count_distinct++;
}
+   stop_progress();
 
if (count_distinct >= GRAPH_PARENT_MISSING)
die(_("the commit graph format cannot write %d commits"), 
count_distinct);
@@ -904,8 +911,13 @@ void write_commit_graph(const char *obj_dir,
ALLOC_ARRAY(commits.list, commits.alloc);
 
num_large_edges = 0;
+   if (report_progress)
+   progress = start_delayed_progress(
+   _("Finding extra edges in commit graph"),
+   oids.nr);
for (i = 0; i < oids.nr; i++) {
int num_parents = 0;
+   display_progress(progress, i + 1);
if (i > 0 && oideq([i - 1], [i]))
continue;
 
@@ -922,6 +934,7 @@ void write_commit_graph(const char *obj_dir,
commits.nr++;
}
num_chunks = num_large_edges ? 4 : 3;
+   stop_progress();
 
if (commits.nr >= GRAPH_PARENT_MISSING)
die(_("too many commits to write graph"));
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 00/10] commit-graph write: progress output improvements

2018-11-22 Thread Ævar Arnfjörð Bjarmason
The "Writing out" progress output was off-by-one because I'd screwed
up a merge conflict. Fix that, and update the various progress output.

On my test setup the "Annotating commit graph" progress sometimes
shows up on linux.git, sometimes not, it's right on that edge of
taking 1 second. So always show it in the commit message examples,
that's less confusing for the reader.

SZEDER Gábor (2):
  commit-graph: rename 'num_extra_edges' variable to 'num_large_edges'
  commit-graph: don't call write_graph_chunk_large_edges() unnecessarily

Ævar Arnfjörð Bjarmason (8):
  commit-graph write: rephrase confusing progress output
  commit-graph write: add "Writing out" progress output
  commit-graph write: more descriptive "writing out" output
  commit-graph write: show progress for object search
  commit-graph write: add more descriptive progress output
  commit-graph write: remove empty line for readability
  commit-graph write: add itermediate progress
  commit-graph write: emit a percentage for all progress

 commit-graph.c | 130 ++---
 1 file changed, 102 insertions(+), 28 deletions(-)

Range-diff:
1:  2b52ad2284 ! 1:  9c17f56ed3 commit-graph write: add "Writing out" progress 
output
 a => b | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

@@ -15,10 +15,11 @@
 
 $ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
 Finding commits for commit graph: 6365442, done.
+Annotating commit graph: 2391666, done.
 Computing commit graph generation numbers: 100% (797222/797222), 
done.
-Writing out commit graph: 100% (3986110/3986110), done.
+Writing out commit graph: 100% (318/318), done.
 
-This "Writing out" number is 4x or 5x the number of commits, depending
+This "Writing out" number is 3x or 4x the number of commits, depending
 on the graph we're processing. A later change will make this explicit
 to the user.
 
@@ -126,7 +127,7 @@
 +   * below loops over our N commits. This number must be
 +   * kept in sync with the number of passes we're doing.
 +   */
-+  int graph_passes = 4;
++  int graph_passes = 3;
 +  if (num_large_edges)
 +  graph_passes++;
 +  progress = start_delayed_progress(
2:  b1773677b1 ! 2:  79b0a467d9 commit-graph write: more descriptive "writing 
out" output
 a => b | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

@@ -3,7 +3,7 @@
 commit-graph write: more descriptive "writing out" output
 
 Make the "Writing out" part of the progress output more
-descriptive. Depending on the shape of the graph we either make 4 or 5
+descriptive. Depending on the shape of the graph we either make 3 or 4
 passes over it.
 
 Let's present this information to the user in case they're wondering
@@ -13,8 +13,9 @@
 
 $ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
 Finding commits for commit graph: 6365442, done.
+Annotating commit graph: 2391666, done.
 Computing commit graph generation numbers: 100% (797222/797222), 
done.
-Writing out commit graph in 5 passes: 100% (3986110/3986110), done.
+Writing out commit graph in 4 passes: 100% (318/318), done.
 
 A note on i18n: Why are we using the Q_() function and passing a
 number & English text for a singular which'll never be used? Because
@@ -35,7 +36,7 @@
if (!commit_graph_compatible(the_repository))
return;
 @@
-   int graph_passes = 4;
+   int graph_passes = 3;
if (num_large_edges)
graph_passes++;
 +  strbuf_addf(_title,
3:  3138b00a2c ! 3:  b32be83b38 commit-graph write: show progress for object 
search
 a => b | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

@@ -8,12 +8,12 @@
 
 Before we'd emit on e.g. linux.git with "commit-graph write":
 
-Finding commits for commit graph: 6365492, done.
+Finding commits for commit graph: 6365442, done.
 [...]
 
 And now:
 
-Finding commits for commit graph: 100% (6365492/6365492), done.
+Finding commits for commit graph: 100% (6365442/6365442), done.
 [...]
 
 Since the commit graph only includes those commits that are packed
4:  f41e3b3eb3 ! 4:  54276723c0 commit-graph write: add more descriptive 
progress output
 a => b | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

@@ -10,7 +10,7 @@
 we support:
 
 $ git commit-graph write
-Finding commits for commit graph among packed objects: 100% 
(6365492/6365492), done.
+Finding commits for 

[PATCH v4 10/10] commit-graph write: emit a percentage for all progress

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Change the "Annotating commit graph" progress output to show a
completion percentage. I added this in 7b0f229222 ("commit-graph
write: add progress output", 2018-09-17) and evidently didn't notice
how easy it was to add a completion percentage.

Now for e.g. linux.git we'll emit:

~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph among packed objects: 100% 
(6365442/6365442), done.
Annotating commit graph: 100% (2391666/2391666), done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph in 4 passes: 100% (318/318), done.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 80f201adf4..6c6edc679b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -660,10 +660,17 @@ static void close_reachable(struct packed_oid_list *oids, 
int report_progress)
struct commit *commit;
struct progress *progress = NULL;
int j = 0;
+   /*
+* We loop over the OIDs N times to close the graph
+* below. This number must be kept in sync with the number of
+* passes.
+*/
+   const int oid_passes = 3;
 
if (report_progress)
progress = start_delayed_progress(
-   _("Annotating commit graph"), 0);
+   _("Annotating commit graph"),
+   oid_passes * oids->nr);
for (i = 0; i < oids->nr; i++) {
display_progress(progress, ++j);
commit = lookup_commit(the_repository, >list[i]);
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v4 05/10] commit-graph write: more descriptive "writing out" output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Make the "Writing out" part of the progress output more
descriptive. Depending on the shape of the graph we either make 3 or 4
passes over it.

Let's present this information to the user in case they're wondering
what this number, which is much larger than their number of commits,
has to do with writing out the commit graph. Now e.g. on linux.git we
emit:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph: 6365442, done.
Annotating commit graph: 2391666, done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph in 4 passes: 100% (318/318), done.

A note on i18n: Why are we using the Q_() function and passing a
number & English text for a singular which'll never be used? Because
the plural rules of translated languages may not match those of
English, and to use the plural function we need to use this format.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index dc57b8fedc..3de65bc2e9 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -780,6 +780,7 @@ void write_commit_graph(const char *obj_dir,
struct commit_list *parent;
struct progress *progress = NULL;
uint64_t progress_cnt = 0;
+   struct strbuf progress_title = STRBUF_INIT;
 
if (!commit_graph_compatible(the_repository))
return;
@@ -962,8 +963,13 @@ void write_commit_graph(const char *obj_dir,
int graph_passes = 3;
if (num_large_edges)
graph_passes++;
+   strbuf_addf(_title,
+   Q_("Writing out commit graph in %d pass",
+  "Writing out commit graph in %d passes",
+  graph_passes),
+   graph_passes);
progress = start_delayed_progress(
-   _("Writing out commit graph"),
+   progress_title.buf,
graph_passes * commits.nr);
}
write_graph_chunk_fanout(f, commits.list, commits.nr, progress, 
_cnt);
@@ -973,6 +979,8 @@ void write_commit_graph(const char *obj_dir,
write_graph_chunk_large_edges(f, commits.list, commits.nr, 
progress, _cnt);
stop_progress();
 
+   strbuf_release(_title);
+
close_commit_graph(the_repository);
finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
commit_lock_file();
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v4 08/10] commit-graph write: remove empty line for readability

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Remove the empty line between a QSORT(...) and the subsequent oideq()
for-loop. This makes it clearer that the QSORT(...) is being done so
that we can run the oideq() loop on adjacent OIDs. Amends code added
in 08fd81c9b6 ("commit-graph: implement write_commit_graph()",
2018-04-02).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 43b15785f6..199155bd68 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -890,7 +890,6 @@ void write_commit_graph(const char *obj_dir,
close_reachable(, report_progress);
 
QSORT(oids.list, oids.nr, commit_compare);
-
count_distinct = 1;
for (i = 1; i < oids.nr; i++) {
if (!oideq([i - 1], [i]))
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v4 06/10] commit-graph write: show progress for object search

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Show the percentage progress for the "Finding commits for commit
graph" phase for the common case where we're operating on all packs in
the repository, as "commit-graph write" or "gc" will do.

Before we'd emit on e.g. linux.git with "commit-graph write":

Finding commits for commit graph: 6365442, done.
[...]

And now:

Finding commits for commit graph: 100% (6365442/6365442), done.
[...]

Since the commit graph only includes those commits that are packed
(via for_each_packed_object(...)) the approximate_object_count()
returns the actual number of objects we're going to process.

Still, it is possible due to a race with "gc" or another process
maintaining packs that the number of objects we're going to process is
lower than what approximate_object_count() reported. In that case we
don't want to stop the progress bar short of 100%. So let's make sure
it snaps to 100% at the end.

The inverse case is also possible and more likely. I.e. that a new
pack has been added between approximate_object_count() and
for_each_packed_object(). In that case the percentage will go beyond
100%, and we'll do nothing to snap it back to 100% at the end.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 3de65bc2e9..42d8365f0d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -781,12 +781,14 @@ void write_commit_graph(const char *obj_dir,
struct progress *progress = NULL;
uint64_t progress_cnt = 0;
struct strbuf progress_title = STRBUF_INIT;
+   unsigned long approx_nr_objects;
 
if (!commit_graph_compatible(the_repository))
return;
 
oids.nr = 0;
-   oids.alloc = approximate_object_count() / 32;
+   approx_nr_objects = approximate_object_count();
+   oids.alloc = approx_nr_objects / 32;
oids.progress = NULL;
oids.progress_done = 0;
 
@@ -866,8 +868,11 @@ void write_commit_graph(const char *obj_dir,
if (!pack_indexes && !commit_hex) {
if (report_progress)
oids.progress = start_delayed_progress(
-   _("Finding commits for commit graph"), 0);
+   _("Finding commits for commit graph"),
+   approx_nr_objects);
for_each_packed_object(add_packed_commits, , 0);
+   if (oids.progress_done < approx_nr_objects)
+   display_progress(oids.progress, approx_nr_objects);
stop_progress();
}
 
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v4 07/10] commit-graph write: add more descriptive progress output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Make the progress output shown when we're searching for commits to
include in the graph more descriptive. This amends code I added in
7b0f229222 ("commit-graph write: add progress output", 2018-09-17).

Now, on linux.git, we'll emit this sort of output in the various modes
we support:

$ git commit-graph write
Finding commits for commit graph among packed objects: 100% 
(6365442/6365442), done.
[...]

# Actually we don't emit this since this takes almost no time at
# all. But if we did (s/_delayed//) we'd show:
$ git for-each-ref --format='%(objectname)' | git commit-graph write 
--stdin-commits
Finding commits for commit graph from 584 refs: 100% (584/584), done.
[...]

$ (cd .git/objects/pack/ && ls *idx) | git commit-graph write --stdin-pack
Finding commits for commit graph in 2 packs: 6365442, done.
[...]

The middle on of those is going to be the output users might see in
practice, since it'll be emitted when they get the commit graph via
gc.writeCommitGraph=true. But as noted above you need a really large
number of refs for this message to show. It'll show up on a test
repository I have with ~165k refs:

Finding commits for commit graph from 165203 refs: 100% (165203/165203), 
done.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 42d8365f0d..43b15785f6 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -818,8 +818,12 @@ void write_commit_graph(const char *obj_dir,
strbuf_addf(, "%s/pack/", obj_dir);
dirlen = packname.len;
if (report_progress) {
-   oids.progress = start_delayed_progress(
-   _("Finding commits for commit graph"), 0);
+   strbuf_addf(_title,
+   Q_("Finding commits for commit graph in %d 
pack",
+  "Finding commits for commit graph in %d 
packs",
+  pack_indexes->nr),
+   pack_indexes->nr);
+   oids.progress = 
start_delayed_progress(progress_title.buf, 0);
oids.progress_done = 0;
}
for (i = 0; i < pack_indexes->nr; i++) {
@@ -836,14 +840,20 @@ void write_commit_graph(const char *obj_dir,
free(p);
}
stop_progress();
+   strbuf_reset(_title);
strbuf_release();
}
 
if (commit_hex) {
-   if (report_progress)
-   progress = start_delayed_progress(
-   _("Finding commits for commit graph"),
-   commit_hex->nr);
+   if (report_progress) {
+   strbuf_addf(_title,
+   Q_("Finding commits for commit graph from 
%d ref",
+  "Finding commits for commit graph from 
%d refs",
+  commit_hex->nr),
+   commit_hex->nr);
+   progress = start_delayed_progress(progress_title.buf,
+ commit_hex->nr);
+   }
for (i = 0; i < commit_hex->nr; i++) {
const char *end;
struct object_id oid;
@@ -863,12 +873,13 @@ void write_commit_graph(const char *obj_dir,
}
}
stop_progress();
+   strbuf_reset(_title);
}
 
if (!pack_indexes && !commit_hex) {
if (report_progress)
oids.progress = start_delayed_progress(
-   _("Finding commits for commit graph"),
+   _("Finding commits for commit graph among 
packed objects"),
approx_nr_objects);
for_each_packed_object(add_packed_commits, , 0);
if (oids.progress_done < approx_nr_objects)
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 08/10] commit-graph write: remove empty line for readability

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Remove the empty line between a QSORT(...) and the subsequent oideq()
for-loop. This makes it clearer that the QSORT(...) is being done so
that we can run the oideq() loop on adjacent OIDs. Amends code added
in 08fd81c9b6 ("commit-graph: implement write_commit_graph()",
2018-04-02).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index cb1aebeb79..21751231e0 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -890,7 +890,6 @@ void write_commit_graph(const char *obj_dir,
close_reachable(, report_progress);
 
QSORT(oids.list, oids.nr, commit_compare);
-
count_distinct = 1;
for (i = 1; i < oids.nr; i++) {
if (!oideq([i - 1], [i]))
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 09/10] commit-graph write: add itermediate progress

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Add progress output to sections of code between "Annotating[...]" and
"Computing[...]generation numbers". This can collectively take 5-10
seconds on a large enough repository.

On a test repository with I have with ~7 million commits and ~50
million objects we'll now emit:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph among packed objects: 100% 
(50026015/50026015), done.
Annotating commit graph: 21567407, done.
Counting distinct commits in commit graph: 100% (7189147/7189147), done.
Finding extra edges in commit graph: 100% (7189147/7189147), done.
Computing commit graph generation numbers: 100% (7144680/7144680), done.
Writing out commit graph: 21434417, done.

Whereas on a medium-sized repository such as linux.git these new
progress bars won't have time to kick in and as before and we'll still
emit output like:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph among packed objects: 100% 
(6365492/6365492), done.
Annotating commit graph: 2391666, done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph: 2399912, done.

The "Counting distinct commits in commit graph" phase will spend most
of its time paused at "0/*" as we QSORT(...) the list. That's not
optimal, but at least we don't seem to be stalling anymore.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 21751231e0..a6e6eeb56b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -889,12 +889,19 @@ void write_commit_graph(const char *obj_dir,
 
close_reachable(, report_progress);
 
+   if (report_progress)
+   progress = start_delayed_progress(
+   _("Counting distinct commits in commit graph"),
+   oids.nr);
+   display_progress(progress, 0); /* TODO: Measure QSORT() progress */
QSORT(oids.list, oids.nr, commit_compare);
count_distinct = 1;
for (i = 1; i < oids.nr; i++) {
+   display_progress(progress, i + 1);
if (!oideq([i - 1], [i]))
count_distinct++;
}
+   stop_progress();
 
if (count_distinct >= GRAPH_PARENT_MISSING)
die(_("the commit graph format cannot write %d commits"), 
count_distinct);
@@ -904,8 +911,13 @@ void write_commit_graph(const char *obj_dir,
ALLOC_ARRAY(commits.list, commits.alloc);
 
num_large_edges = 0;
+   if (report_progress)
+   progress = start_delayed_progress(
+   _("Finding extra edges in commit graph"),
+   oids.nr);
for (i = 0; i < oids.nr; i++) {
int num_parents = 0;
+   display_progress(progress, i + 1);
if (i > 0 && oideq([i - 1], [i]))
continue;
 
@@ -922,6 +934,7 @@ void write_commit_graph(const char *obj_dir,
commits.nr++;
}
num_chunks = num_large_edges ? 4 : 3;
+   stop_progress();
 
if (commits.nr >= GRAPH_PARENT_MISSING)
die(_("too many commits to write graph"));
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 10/10] commit-graph write: emit a percentage for all progress

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Change the "Annotating commit graph" progress output to show a
completion percentage. I added this in 7b0f229222 ("commit-graph
write: add progress output", 2018-09-17) and evidently didn't notice
how easy it was to add a completion percentage.

Now for the very large test repository mentioned in previous commits
we'll emit (shows all progress output):

Finding commits for commit graph among packed objects: 100% 
(48333911/48333911), done.
Annotating commit graph: 100% (21435984/21435984), done.
Counting distinct commits in commit graph: 100% (7145328/7145328), done.
Finding extra edges in commit graph: 100% (7145328/7145328), done.
Computing commit graph generation numbers: 100% (7145328/7145328), done.
Writing out commit graph in 5 passes: 100% (35726640/35726640), done.

And for linux.git:

Finding commits for commit graph among packed objects: 100% 
(6365442/6365442), done.
Annotating commit graph: 100% (2391666/2391666), done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph in 5 passes: 100% (3986110/3986110), done.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index a6e6eeb56b..c893466042 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -660,10 +660,17 @@ static void close_reachable(struct packed_oid_list *oids, 
int report_progress)
struct commit *commit;
struct progress *progress = NULL;
int j = 0;
+   /*
+* We loop over the OIDs N times to close the graph
+* below. This number must be kept in sync with the number of
+* passes.
+*/
+   const int oid_passes = 3;
 
if (report_progress)
progress = start_delayed_progress(
-   _("Annotating commit graph"), 0);
+   _("Annotating commit graph"),
+   oid_passes * oids->nr);
for (i = 0; i < oids->nr; i++) {
display_progress(progress, ++j);
commit = lookup_commit(the_repository, >list[i]);
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 04/10] commit-graph write: add "Writing out" progress output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Add progress output to be shown when we're writing out the
commit-graph, this adds to the output already added in 7b0f229222
("commit-graph write: add progress output", 2018-09-17).

As noted in that commit most of the progress output isn't displayed on
small repositories, but before this change we'd noticeably hang for
2-3 seconds at the end on medium sized repositories such as linux.git.

Now we'll instead show output like this, and have no human-observable
point at which we're not producing progress output:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph: 6365442, done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph: 100% (3986110/3986110), done.

This "Writing out" number is 4x or 5x the number of commits, depending
on the graph we're processing. A later change will make this explicit
to the user.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 48 +++-
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index d11370a2b3..e32a5cc1bc 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -433,7 +433,9 @@ struct tree *get_commit_tree_in_graph(struct repository *r, 
const struct commit
 
 static void write_graph_chunk_fanout(struct hashfile *f,
 struct commit **commits,
-int nr_commits)
+int nr_commits,
+struct progress *progress,
+uint64_t *progress_cnt)
 {
int i, count = 0;
struct commit **list = commits;
@@ -447,6 +449,7 @@ static void write_graph_chunk_fanout(struct hashfile *f,
while (count < nr_commits) {
if ((*list)->object.oid.hash[0] != i)
break;
+   display_progress(progress, ++*progress_cnt);
count++;
list++;
}
@@ -456,12 +459,16 @@ static void write_graph_chunk_fanout(struct hashfile *f,
 }
 
 static void write_graph_chunk_oids(struct hashfile *f, int hash_len,
-  struct commit **commits, int nr_commits)
+  struct commit **commits, int nr_commits,
+  struct progress *progress,
+  uint64_t *progress_cnt)
 {
struct commit **list = commits;
int count;
-   for (count = 0; count < nr_commits; count++, list++)
+   for (count = 0; count < nr_commits; count++, list++) {
+   display_progress(progress, ++*progress_cnt);
hashwrite(f, (*list)->object.oid.hash, (int)hash_len);
+   }
 }
 
 static const unsigned char *commit_to_sha1(size_t index, void *table)
@@ -471,7 +478,9 @@ static const unsigned char *commit_to_sha1(size_t index, 
void *table)
 }
 
 static void write_graph_chunk_data(struct hashfile *f, int hash_len,
-  struct commit **commits, int nr_commits)
+  struct commit **commits, int nr_commits,
+  struct progress *progress,
+  uint64_t *progress_cnt)
 {
struct commit **list = commits;
struct commit **last = commits + nr_commits;
@@ -481,6 +490,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
struct commit_list *parent;
int edge_value;
uint32_t packedDate[2];
+   display_progress(progress, ++*progress_cnt);
 
parse_commit(*list);
hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
@@ -542,7 +552,9 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
 
 static void write_graph_chunk_large_edges(struct hashfile *f,
  struct commit **commits,
- int nr_commits)
+ int nr_commits,
+ struct progress *progress,
+ uint64_t *progress_cnt)
 {
struct commit **list = commits;
struct commit **last = commits + nr_commits;
@@ -550,6 +562,9 @@ static void write_graph_chunk_large_edges(struct hashfile 
*f,
 
while (list < last) {
int num_parents = 0;
+
+   display_progress(progress, ++*progress_cnt);
+
for (parent = (*list)->parents; num_parents < 3 && parent;
 parent = parent->next)
num_parents++;
@@ -764,6 +779,7 @@ void write_commit_graph(const char *obj_dir,
int num_large_edges;
struct commit_list *parent;
struct progress *progress = NULL;
+   uint64_t 

[PATCH v3 00/10] commit-graph write: progress output improvements

2018-11-22 Thread Ævar Arnfjörð Bjarmason
This incorporates SZEDER's recent two-part series, rebases mine on
top, and fixes a few things while I'm at it. Now there's no progress
output where we don't show a completion percentage.

SZEDER Gábor (2):
  commit-graph: rename 'num_extra_edges' variable to 'num_large_edges'
  commit-graph: don't call write_graph_chunk_large_edges() unnecessarily

Ævar Arnfjörð Bjarmason (8):
  commit-graph write: rephrase confusing progress output
  commit-graph write: add "Writing out" progress output
  commit-graph write: more descriptive "writing out" output
  commit-graph write: show progress for object search
  commit-graph write: add more descriptive progress output
  commit-graph write: remove empty line for readability
  commit-graph write: add itermediate progress
  commit-graph write: emit a percentage for all progress

 commit-graph.c | 130 ++---
 1 file changed, 102 insertions(+), 28 deletions(-)

Range-diff:

By the way, is there any way to

 [.. snipped lots of irrelevant commits...]
 -:  -- > 14:  07d06c50c0 commit-graph: rename 'num_extra_edges' 
variable to 'num_large_edges'
 -:  -- > 15:  904dda1e7a commit-graph: don't call 
write_graph_chunk_large_edges() unnecessarily

Pass the equivalent of "git range-diff origin/master topic-2 topic-3"
to git-format-patch?

 1:  9f7fb459bd = 16:  1126c7e29d commit-graph write: rephrase confusing 
progress output
 a => b | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

 2:  093c63e99f ! 17:  2b52ad2284 commit-graph write: add more progress output
 a => b | 0
 1 file changed, 0 insertions(+), 0 deletions(-)

@@ -1,9 +1,10 @@
 Author: Ævar Arnfjörð Bjarmason 
 
-commit-graph write: add more progress output
+commit-graph write: add "Writing out" progress output
 
-Add more progress output to the output already added in
-7b0f229222 ("commit-graph write: add progress output", 2018-09-17).
+Add progress output to be shown when we're writing out the
+commit-graph, this adds to the output already added in 7b0f229222
+("commit-graph write: add progress output", 2018-09-17).
 
 As noted in that commit most of the progress output isn't displayed on
 small repositories, but before this change we'd noticeably hang for
@@ -13,30 +14,13 @@
 point at which we're not producing progress output:
 
 $ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
-Finding commits for commit graph: 6365492, done.
+Finding commits for commit graph: 6365442, done.
 Computing commit graph generation numbers: 100% (797222/797222), 
done.
-Writing out commit graph: 2399912, done.
+Writing out commit graph: 100% (3986110/3986110), done.
 
-This "writing out" number is not meant to be meaningful to the user,
-but just to show that we're doing work and the command isn't
-hanging.
-
-In the current implementation it's approximately 4x the number of
-commits. As noted in on-list discussion[1] we could add the loops up
-and show percentage progress here, but I don't think it's worth it. It
-would make the implementation more complex and harder to maintain for
-very little gain.
-
-On a much larger in-house repository I have we'll show (note how we
-also say "Annotating[...]"):
-
-$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
-Finding commits for commit graph: 50026015, done.
-Annotating commit graph: 21567407, done.
-Computing commit graph generation numbers: 100% (7144680/7144680), 
done.
-Writing out commit graph: 21434417, done.
-
-1. https://public-inbox.org/git/20181120165800.gb30...@szeder.dev/
+This "Writing out" number is 4x or 5x the number of commits, depending
+on the graph we're processing. A later change will make this explicit
+to the user.
 
 Signed-off-by: Ævar Arnfjörð Bjarmason 
 
@@ -55,13 +39,13 @@
int i, count = 0;
struct commit **list = commits;
 @@
-*/
-   for (i = 0; i < 256; i++) {
while (count < nr_commits) {
-+  display_progress(progress, ++*progress_cnt);
if ((*list)->object.oid.hash[0] != i)
break;
++  display_progress(progress, ++*progress_cnt);
count++;
+   list++;
+   }
 @@
  }
  
@@ -112,15 +96,17 @@
struct commit **list = commits;
struct commit **last = commits + nr_commits;
 @@
- commits,
- nr_commits,
- 

[PATCH v3 07/10] commit-graph write: add more descriptive progress output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Make the progress output shown when we're searching for commits to
include in the graph more descriptive. This amends code I added in
7b0f229222 ("commit-graph write: add progress output", 2018-09-17).

Now, on linux.git, we'll emit this sort of output in the various modes
we support:

$ git commit-graph write
Finding commits for commit graph among packed objects: 100% 
(6365492/6365492), done.
[...]

# Actually we don't emit this since this takes almost no time at
# all. But if we did (s/_delayed//) we'd show:
$ git for-each-ref --format='%(objectname)' | git commit-graph write 
--stdin-commits
Finding commits for commit graph from 584 refs: 100% (584/584), done.
[...]

$ (cd .git/objects/pack/ && ls *idx) | git commit-graph write --stdin-pack
Finding commits for commit graph in 3 packs: 6365492, done.
[...]

The middle on of those is going to be the output users might see in
practice, since it'll be emitted when they get the commit graph via
gc.writeCommitGraph=true. But as noted above you need a really large
number of refs for this message to show. It'll show up on a test
repository I have with ~165k refs:

Finding commits for commit graph from 165203 refs: 100% (165203/165203), 
done.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 25 ++---
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index d6166beb19..cb1aebeb79 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -818,8 +818,12 @@ void write_commit_graph(const char *obj_dir,
strbuf_addf(, "%s/pack/", obj_dir);
dirlen = packname.len;
if (report_progress) {
-   oids.progress = start_delayed_progress(
-   _("Finding commits for commit graph"), 0);
+   strbuf_addf(_title,
+   Q_("Finding commits for commit graph in %d 
pack",
+  "Finding commits for commit graph in %d 
packs",
+  pack_indexes->nr),
+   pack_indexes->nr);
+   oids.progress = 
start_delayed_progress(progress_title.buf, 0);
oids.progress_done = 0;
}
for (i = 0; i < pack_indexes->nr; i++) {
@@ -836,14 +840,20 @@ void write_commit_graph(const char *obj_dir,
free(p);
}
stop_progress();
+   strbuf_reset(_title);
strbuf_release();
}
 
if (commit_hex) {
-   if (report_progress)
-   progress = start_delayed_progress(
-   _("Finding commits for commit graph"),
-   commit_hex->nr);
+   if (report_progress) {
+   strbuf_addf(_title,
+   Q_("Finding commits for commit graph from 
%d ref",
+  "Finding commits for commit graph from 
%d refs",
+  commit_hex->nr),
+   commit_hex->nr);
+   progress = start_delayed_progress(progress_title.buf,
+ commit_hex->nr);
+   }
for (i = 0; i < commit_hex->nr; i++) {
const char *end;
struct object_id oid;
@@ -863,12 +873,13 @@ void write_commit_graph(const char *obj_dir,
}
}
stop_progress();
+   strbuf_reset(_title);
}
 
if (!pack_indexes && !commit_hex) {
if (report_progress)
oids.progress = start_delayed_progress(
-   _("Finding commits for commit graph"),
+   _("Finding commits for commit graph among 
packed objects"),
approx_nr_objects);
for_each_packed_object(add_packed_commits, , 0);
if (oids.progress_done < approx_nr_objects)
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 06/10] commit-graph write: show progress for object search

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Show the percentage progress for the "Finding commits for commit
graph" phase for the common case where we're operating on all packs in
the repository, as "commit-graph write" or "gc" will do.

Before we'd emit on e.g. linux.git with "commit-graph write":

Finding commits for commit graph: 6365492, done.
[...]

And now:

Finding commits for commit graph: 100% (6365492/6365492), done.
[...]

Since the commit graph only includes those commits that are packed
(via for_each_packed_object(...)) the approximate_object_count()
returns the actual number of objects we're going to process.

Still, it is possible due to a race with "gc" or another process
maintaining packs that the number of objects we're going to process is
lower than what approximate_object_count() reported. In that case we
don't want to stop the progress bar short of 100%. So let's make sure
it snaps to 100% at the end.

The inverse case is also possible and more likely. I.e. that a new
pack has been added between approximate_object_count() and
for_each_packed_object(). In that case the percentage will go beyond
100%, and we'll do nothing to snap it back to 100% at the end.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 8e5970f0b9..d6166beb19 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -781,12 +781,14 @@ void write_commit_graph(const char *obj_dir,
struct progress *progress = NULL;
uint64_t progress_cnt = 0;
struct strbuf progress_title = STRBUF_INIT;
+   unsigned long approx_nr_objects;
 
if (!commit_graph_compatible(the_repository))
return;
 
oids.nr = 0;
-   oids.alloc = approximate_object_count() / 32;
+   approx_nr_objects = approximate_object_count();
+   oids.alloc = approx_nr_objects / 32;
oids.progress = NULL;
oids.progress_done = 0;
 
@@ -866,8 +868,11 @@ void write_commit_graph(const char *obj_dir,
if (!pack_indexes && !commit_hex) {
if (report_progress)
oids.progress = start_delayed_progress(
-   _("Finding commits for commit graph"), 0);
+   _("Finding commits for commit graph"),
+   approx_nr_objects);
for_each_packed_object(add_packed_commits, , 0);
+   if (oids.progress_done < approx_nr_objects)
+   display_progress(oids.progress, approx_nr_objects);
stop_progress();
}
 
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 02/10] commit-graph: don't call write_graph_chunk_large_edges() unnecessarily

2018-11-22 Thread Ævar Arnfjörð Bjarmason
From: SZEDER Gábor 

The optional 'Large Edge List' chunk of the commit graph file stores
parent information for commits with more than two parents.  Since the
chunk is optional, write_commit_graph() looks through all commits to
find those with more than two parents, and then writes the commit
graph file header accordingly, i.e. if there are no such commits, then
there won't be a 'Large Edge List' chunk written, only the three
mandatory chunks.

However, when it comes to writing chunk data, write_commit_graph()
unconditionally invokes write_graph_chunk_large_edges(), even when it
was decided earlier that that chunk won't be written.  Strictly
speaking there is no bug here, because write_graph_chunk_large_edges()
won't write anything because it won't find any commits with more than
two parents, but then it unnecessarily and in vain looks through all
commits once again in search for such commits.

Don't call write_graph_chunk_large_edges() when that chunk won't be
written to spare an unnecessary iteration over all commits.

Signed-off-by: SZEDER Gábor 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 7b4e3a02cf..965eb23a7b 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -940,7 +940,8 @@ void write_commit_graph(const char *obj_dir,
write_graph_chunk_fanout(f, commits.list, commits.nr);
write_graph_chunk_oids(f, GRAPH_OID_LEN, commits.list, commits.nr);
write_graph_chunk_data(f, GRAPH_OID_LEN, commits.list, commits.nr);
-   write_graph_chunk_large_edges(f, commits.list, commits.nr);
+   if (num_large_edges)
+   write_graph_chunk_large_edges(f, commits.list, commits.nr);
 
close_commit_graph(the_repository);
finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 05/10] commit-graph write: more descriptive "writing out" output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Make the "Writing out" part of the progress output more
descriptive. Depending on the shape of the graph we either make 4 or 5
passes over it.

Let's present this information to the user in case they're wondering
what this number, which is much larger than their number of commits,
has to do with writing out the commit graph. Now e.g. on linux.git we
emit:

$ ~/g/git/git --exec-path=$HOME/g/git commit-graph write
Finding commits for commit graph: 6365442, done.
Computing commit graph generation numbers: 100% (797222/797222), done.
Writing out commit graph in 5 passes: 100% (3986110/3986110), done.

A note on i18n: Why are we using the Q_() function and passing a
number & English text for a singular which'll never be used? Because
the plural rules of translated languages may not match those of
English, and to use the plural function we need to use this format.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index e32a5cc1bc..8e5970f0b9 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -780,6 +780,7 @@ void write_commit_graph(const char *obj_dir,
struct commit_list *parent;
struct progress *progress = NULL;
uint64_t progress_cnt = 0;
+   struct strbuf progress_title = STRBUF_INIT;
 
if (!commit_graph_compatible(the_repository))
return;
@@ -962,8 +963,13 @@ void write_commit_graph(const char *obj_dir,
int graph_passes = 4;
if (num_large_edges)
graph_passes++;
+   strbuf_addf(_title,
+   Q_("Writing out commit graph in %d pass",
+  "Writing out commit graph in %d passes",
+  graph_passes),
+   graph_passes);
progress = start_delayed_progress(
-   _("Writing out commit graph"),
+   progress_title.buf,
graph_passes * commits.nr);
}
write_graph_chunk_fanout(f, commits.list, commits.nr, progress, 
_cnt);
@@ -973,6 +979,8 @@ void write_commit_graph(const char *obj_dir,
write_graph_chunk_large_edges(f, commits.list, commits.nr, 
progress, _cnt);
stop_progress();
 
+   strbuf_release(_title);
+
close_commit_graph(the_repository);
finalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);
commit_lock_file();
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 03/10] commit-graph write: rephrase confusing progress output

2018-11-22 Thread Ævar Arnfjörð Bjarmason
Rephrase the title shown for the progress output emitted by
close_reachable(). The message I added in 7b0f229222 ("commit-graph
write: add progress output", 2018-09-17) gave the impression that it
would count up to the number of commit objects.

But that's not what the number means. It just represents the work
we're doing in several for-loops to do various work before the graph
is written out. So let's just say "Annotating commit graph", that
title makes no such promises, and we can add other loops here in the
future and still consistently show progress output.

See [1] for the initial bug report & subsequent discussion about other
approaching to solving this.

1. https://public-inbox.org/git/20181015165447.gh19...@szeder.dev/

Reported-by: SZEDER Gábor 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index 965eb23a7b..d11370a2b3 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -648,7 +648,7 @@ static void close_reachable(struct packed_oid_list *oids, 
int report_progress)
 
if (report_progress)
progress = start_delayed_progress(
-   _("Annotating commits in commit graph"), 0);
+   _("Annotating commit graph"), 0);
for (i = 0; i < oids->nr; i++) {
display_progress(progress, ++j);
commit = lookup_commit(the_repository, >list[i]);
-- 
2.20.0.rc0.387.gc7a69e6b6c



[PATCH v3 01/10] commit-graph: rename 'num_extra_edges' variable to 'num_large_edges'

2018-11-22 Thread Ævar Arnfjörð Bjarmason
From: SZEDER Gábor 

The commit graph file format describes an optional 'Large Edge List'
chunk, and the function writing out this chunk is called
write_graph_chunk_large_edges().  Then there are two functions in
'commit-graph.c', namely write_graph_chunk_data() and
write_commit_graph(), which have a local variable called
'num_extra_edges'.

It can be confusing on first sight whether large edges and extra edges
refer to the same thing or not, but they do, so let's rename those
variables to 'num_large_edges'.

Signed-off-by: SZEDER Gábor 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 commit-graph.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index 40c855f185..7b4e3a02cf 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -475,7 +475,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
 {
struct commit **list = commits;
struct commit **last = commits + nr_commits;
-   uint32_t num_extra_edges = 0;
+   uint32_t num_large_edges = 0;
 
while (list < last) {
struct commit_list *parent;
@@ -507,7 +507,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
if (!parent)
edge_value = GRAPH_PARENT_NONE;
else if (parent->next)
-   edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | 
num_extra_edges;
+   edge_value = GRAPH_OCTOPUS_EDGES_NEEDED | 
num_large_edges;
else {
edge_value = sha1_pos(parent->item->object.oid.hash,
  commits,
@@ -521,7 +521,7 @@ static void write_graph_chunk_data(struct hashfile *f, int 
hash_len,
 
if (edge_value & GRAPH_OCTOPUS_EDGES_NEEDED) {
do {
-   num_extra_edges++;
+   num_large_edges++;
parent = parent->next;
} while (parent);
}
@@ -761,7 +761,7 @@ void write_commit_graph(const char *obj_dir,
uint32_t chunk_ids[5];
uint64_t chunk_offsets[5];
int num_chunks;
-   int num_extra_edges;
+   int num_large_edges;
struct commit_list *parent;
struct progress *progress = NULL;
 
@@ -871,7 +871,7 @@ void write_commit_graph(const char *obj_dir,
commits.alloc = count_distinct;
ALLOC_ARRAY(commits.list, commits.alloc);
 
-   num_extra_edges = 0;
+   num_large_edges = 0;
for (i = 0; i < oids.nr; i++) {
int num_parents = 0;
if (i > 0 && oideq([i - 1], [i]))
@@ -885,11 +885,11 @@ void write_commit_graph(const char *obj_dir,
num_parents++;
 
if (num_parents > 2)
-   num_extra_edges += num_parents - 1;
+   num_large_edges += num_parents - 1;
 
commits.nr++;
}
-   num_chunks = num_extra_edges ? 4 : 3;
+   num_chunks = num_large_edges ? 4 : 3;
 
if (commits.nr >= GRAPH_PARENT_MISSING)
die(_("too many commits to write graph"));
@@ -916,7 +916,7 @@ void write_commit_graph(const char *obj_dir,
chunk_ids[0] = GRAPH_CHUNKID_OIDFANOUT;
chunk_ids[1] = GRAPH_CHUNKID_OIDLOOKUP;
chunk_ids[2] = GRAPH_CHUNKID_DATA;
-   if (num_extra_edges)
+   if (num_large_edges)
chunk_ids[3] = GRAPH_CHUNKID_LARGEEDGES;
else
chunk_ids[3] = 0;
@@ -926,7 +926,7 @@ void write_commit_graph(const char *obj_dir,
chunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;
chunk_offsets[2] = chunk_offsets[1] + GRAPH_OID_LEN * commits.nr;
chunk_offsets[3] = chunk_offsets[2] + (GRAPH_OID_LEN + 16) * commits.nr;
-   chunk_offsets[4] = chunk_offsets[3] + 4 * num_extra_edges;
+   chunk_offsets[4] = chunk_offsets[3] + 4 * num_large_edges;
 
for (i = 0; i <= num_chunks; i++) {
uint32_t chunk_write[3];
-- 
2.20.0.rc0.387.gc7a69e6b6c



Re: [RFC/PATCH 0/5] stop installing old libexec aliases like "git-init"

2018-11-22 Thread Johannes Schindelin
Hi Peff,

On Sat, 17 Nov 2018, Jeff King wrote:

> On Fri, Nov 16, 2018 at 08:22:11PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> > So maybe we should just document this interface better. It seems one
> > implicit dependency is that we expect a manpage for the tool to exist
> > for --help.
> 
> Yeah, I think this really the only problematic assumption. The rest of
> "-c", "--git-dir", etc, are just manipulating the environment, and that
> gets passed through to sub-invocations of Git (so if I have a script
> which calls git-config, it will pick up the "-c" config).
> 
> It would be nice if there was a way to ask "is there a manpage?", and
> fallback to running "git-cmd --help". But:
> 
>   1. I don't think there is a portable way to query that via man. And
>  anyway, depending platform and config, it may be opening a browser
>  to show HTML documentation (or I think even texinfo?).
> 
>   2. We can just ask whether "man git-sizer" (or whatever help display
>  command) returned a non-zero exit code, and fall back to "git-sizer
>  --help". But there's an infinite-loop possibility here: running
>  "git-sizer --help" does what we want. But if "man git-log" failed,
>  we'd run "git-log --help", which in turn runs "git help log", which
>  runs "man git-log", and so on.
> 
>  You can break that loop with an environment variable for "I already
>  tried to show the manpage", which would I guess convert "--help" to
>  "-h".
> 
> So it's maybe do-able, but not quite as trivial as one might hope.

A trivial alternative would be to recommend adding a man page for
3rd-party git-s.

In other words, as soon as `git-sizer` is accompanied by `git-sizer.1` in
one of the appropriate locations, it is set.

(Actually, it is not: on Windows, it would have to add git-sizer.html in
the appropriate location, but we can deal with this if needed.)

> > But I don't remember the details, and can't reproduce it now with:
> > 
> > $ cat ~/bin/git-sigint 
> > #!/usr/bin/env perl
> > $SIG{INT} = sub { warn localtime . " " . $$ };
> > sleep 1 while 1;
> > $ git sigint # or git-sigint
> > [... behaves the same either way ...]
> > 
> > So maybe it was something else, or I'm misremembering...
> 
> I think that generally works because hitting ^C is going to send SIGINT
> to the whole process group. A more interesting case is:
> 
>   git sigint &
>   kill -INT $!
> 
> There $! is a parent "git" process that is just waiting on git-sigint to
> die. But that works, too, because the parent relays common signals due
> to 10c6cddd92 (dashed externals: kill children on exit, 2012-01-08). So
> you might have been remembering issues prior to that commit (or uncommon
> signals; these come from sigchain_push_common).

FWIW I do have a couple of scripts I use that I install into
`$HOME/bin/git-`. Although, granted, I essentially switched to
aliases for most of them, where the aliases still call a script that is
checked out in some folder in my home directory. The reason: this works in
more circumstances, as I do not have to add `$HOME/bin` to the `PATH`,
say, in PowerShell.

So YMMV with git-s. My rule of thumb is: if I want to use this
myself only, I'll make it an alias. If I want to ship it (e.g. with Git
for Windows), I'll make it a git-.

Ciao,
Dscho

[PATCH 0/1] Fix Windows build of next

2018-11-22 Thread Johannes Schindelin via GitGitGadget
The topic ot/ref-filter-object-info broke the Windows build since it entered 
pu, and as a consequence we have no test coverage of the other topics in pu.

Sadly, this topic now advanced to next. Junio, I would like to ask you to
merge this fix down to next and to advance to master together with 
ot/ref-filter-object-info.

Johannes Schindelin (1):
  ref-filter: replace unportable `%lld` format

 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: d364d6b33e15dbc6e57d83f9f1457a8e8fe77046
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-87%2Fdscho%2Fno-percent-lld-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-87/dscho/no-percent-lld-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/87
-- 
gitgitgadget


[PATCH 1/1] ref-filter: replace unportable `%lld` format

2018-11-22 Thread Johannes Schindelin via GitGitGadget
From: Johannes Schindelin 

The `%lld` format is supported on Linux and macOS, but not on Windows.
This issue has been reported ten days ago (Message-ID:
nycvar.qro.7.76.6.1811121300520...@tvgsbejvaqbjf.bet), but the
corresponding topic still advanced to `next` in the meantime, breaking
the Windows build.

Let's use `PRIdMAX` and a cast to `intmax_t` instead, which unbreaks the
build, and imitates how we do things e.g. in `json-writer.c` already.

Signed-off-by: Johannes Schindelin 
---
 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3cfe01a039..69cdf2dbb5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -897,7 +897,7 @@ static void grab_common_values(struct atom_value *val, int 
deref, struct expand_
v->s = xstrdup(type_name(oi->type));
else if (!strcmp(name, "objectsize:disk")) {
v->value = oi->disk_size;
-   v->s = xstrfmt("%lld", (long long)oi->disk_size);
+   v->s = xstrfmt("%"PRIdMAX, (intmax_t)oi->disk_size);
} else if (!strcmp(name, "objectsize")) {
v->value = oi->size;
v->s = xstrfmt("%lu", oi->size);
-- 
gitgitgadget


git@vger.kernel.org - this account has been hacked! Change your password e7d1w8i2n right now!

2018-11-22 Thread git
Hello!

I have very bad news for you.
06/08/2018 - on this day I hacked your operating system and got full access to 
your account git@vger.kernel.org
On that day your account git@vger.kernel.org password was: e7d1w8i2n

It is useless to change the password, my malware intercepts it every time.

How it was:
In the software of the router to which you were connected that day, there was a 
vulnerability.
I first hacked this router and placed my malicious code on it.
When you entered in the Internet, my trojan was installed on the operating 
system of your device.

After that, I made a full dump of your disk (I have all your address book, 
history of viewing sites, all files, phone numbers and addresses of all your 
contacts).

A month ago, I wanted to lock your device and ask for a small amount of money 
to unlock.
But I looked at the sites that you regularly visit, and came to the big delight 
of your favorite resources.
I'm talking about sites for adults.

I want to say - you are a big, big pervert. You have unbridled fantasy!!!

After that, an idea came to my mind.
I made a screenshot of the intimate website where you have fun (you know what 
it is about, right?).
After that, I made a screenshot of your joys (using the camera of your device) 
and joined all together.
It turned out beautifully, do not doubt.

I am strongly belive that you would not like to show these pictures to your 
relatives, friends or colleagues.
I think $707 is a very small amount for my silence.
Besides, I spent a lot of time on you!

I accept money only in Bitcoins.
My BTC wallet: 1Bu2NDQScVQwixvhf4z4xbZQVNFWuXokSJ

You do not know how to replenish a Bitcoin wallet?
In any search engine write "how to send money to btc wallet".
It's easier than send money to a credit card!

For payment you have a little more than two days (exactly 50 hours).
Do not worry, the timer will start at the moment when you open this letter. 
Yes, yes .. it has already started!

After payment, my virus and dirty photos with you self-destruct automatically.
Narrative, if I do not receive the specified amount from you, then your device 
will be blocked, and all your contacts will receive a photos with your "joys".

I want you to be prudent.
- Do not try to find and destroy my virus! (All your data is already uploaded 
to a remote server)
- Do not try to contact me (this is not feasible, I sent you an email from your 
account)
- Various security services will not help you; formatting a disk or destroying 
a device will not help either, since your data is already on a remote server.

P.S. I guarantee you that I will not disturb you again after payment, as you 
are not my single victim.
 This is a hacker code of honor.

>From now on, I advise you to use good antiviruses and update them regularly 
>(several times a day)!

Don't be mad at me, everyone has their own work.
Farewell.



Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option

2018-11-22 Thread Johannes Schindelin
Hi Junio,

On Wed, 21 Nov 2018, Junio C Hamano wrote:

> Оля Тележная   writes:
> 
> >> I am OK if we avoid PRIdMAX and use PRIuMAX instead with a cast to
> >> the corresponding size in this codepath, as long as we properly
> >> handle negative oi.disk_size field, which may be telling some
> >> "unusual" condition to us.
> >
> > Maybe we want to change the type (from off_t to unsigned) directly in
> > struct object_info? That will help us not to make additional
> > checkings. Or, at least, I suggest to add check to
> > oid_object_info_extended() so that this function will give a guarantee
> > that the size is non-negative.
> 
> I am fine with the approach.  The potential gain of allowing
> oi.disk_size is it would allow the code to say "I'll give these
> pieces of info about the object, but the on-disk size is unknown"
> without failing the whole object_info_extended() request.  And I
> personally do not think such an ability is not all that important
> or useful.

I see that this topic advanced to `next`, which means that the Windows
build of `next` is now broken.

To fix this, I prepared a GitGitGadget PR
(https://github.com/gitgitgadget/git/pull/87) and will submit it as soon
as I am satisfied that the build works.

Ciao,
Dscho

Document change in format of raw diff output format

2018-11-22 Thread Greg Hurrell
I was troubleshooting some breakage in some code that consumes the output of 
`git log --raw` and looking on two machines with different versions of Git just 
now I discovered the output format has changed somewhere between v2.14.5:

:00 100644 0... 9773b7718... A  content/snippets/1157.md

and v2.19.0:

:00 100644 0 9773b7718 Acontent/snippets/1157.md

A quick search turns up some patches related to the GIT_PRINT_SHA1_ELLIPSIS env 
variable, which can be used to force the old output format, and which landed in 
v2.16.0, I think.

Does it sound right that we should update the documentation in diff-format.txt 
to show what the new output format is? The examples all show the old output 
format, which isn't produced by default any more.

Something like the following? If the answer is yes, I can turn it into a real 
patch.

Cheers,
Greg


diff --git a/Documentation/diff-format.txt b/Documentation/diff-format.txt
index 706916c94c..33776459d0 100644
--- a/Documentation/diff-format.txt
+++ b/Documentation/diff-format.txt
@@ -26,12 +26,12 @@ line per changed file.
 An output line is formatted this way:

 
-in-place edit  :100644 100644 bcd1234... 0123456... M file0
-copy-edit  :100644 100644 abcd123... 1234567... C68 file1 file2
-rename-edit:100644 100644 abcd123... 1234567... R86 file1 file3
-create :00 100644 000... 1234567... A file4
-delete :100644 00 1234567... 000... D file5
-unmerged   :00 00 000... 000... U file6
+in-place edit  :100644 100644 bcd123456 012345678 M file0
+copy-edit  :100644 100644 abcd12345 123456789 C68 file1 file2
+rename-edit:100644 100644 abcd12345 123456789 R86 file1 file3
+create :00 100644 0 123456789 A file4
+delete :100644 00 123456789 0 D file5
+unmerged   :00 00 0 0 U file6
 

 That is, from the left to the right:
@@ -75,7 +75,7 @@ and it is out of sync with the index.
 Example:

 
-:100644 100644 5be4a4.. 00.. M file.c
+:100644 100644 5be4a4abc 0 M file.c
 

 Without the `-z` option, pathnames with "unusual" characters are
@@ -100,7 +100,7 @@ from the format described above in the following way:
 Example:

 
-::100644 100644 100644 fabadb8... cc95eb0... 4866510... MM describe.c
+::100644 100644 100644 fabadb827 cc95eb0f2 4866510ea MMdescribe.c
 

 Note that 'combined diff' lists only files which were modified from



Re: What's cooking in git.git (Nov 2018, #06; Wed, 21)

2018-11-22 Thread Ævar Arnfjörð Bjarmason
On Thu, Nov 22, 2018 at 11:31 AM Stephen P. Smith  wrote:
>
> On Wednesday, November 21, 2018 2:00:16 AM MST Junio C Hamano wrote:
> > [Stalled]
> >
> > * lt/date-human (2018-07-09) 1 commit
> >  - Add 'human' date format
> >
> >  A new date format "--date=human" that morphs its output depending
> >  on how far the time is from the current time has been introduced.
> >  "--date=auto" can be used to use this new format when the output is
> >  goint to the pager or to the terminal and otherwise the default
> >  format.
>
> What needs to be done with this patch to move it along?

In e.g. "Git Test Coverage Report (Wednesday Nov 21)" by Stolee you
can see that the new code in date.c is largely uncovered. Adding tests
for the behavior would be a good start.

> I see that both Linus and Junio have signed the patch.

That just means Linus wrote it and Junio ran "git am -s" on it.


Re: [PATCH] t5562: skip if NO_CURL is enabled

2018-11-22 Thread Carlo Arenas
On Wed, Nov 21, 2018 at 10:37 PM Max Kirillov  wrote:
>
> On Wed, Nov 21, 2018 at 05:04:25PM -0800, Carlo Arenas wrote:
> > the last child of its children long gone with an error as shown by :
> >
> >   9255  1 git-http-backend CALL  close(1)
> ...
> >   9255  1 git-http-backend CALL  write(2,0xbfb2a604,0x36)
> >   9255  1 git-http-backend GIO   fd 2 wrote 54 bytes
> >"fatal: request ended in the middle of the gzip stream\n"
>
> This should be some other test than push_plain, some of the
> gzip related ones. Are there other tests failing?

it should, but I should note that for test 9 to fail, then either (or both)
tests 7 and 8 should first succeed; not that I'd seen any other test fail (after
I locally patched the perl path, of course) even when reordering them and
while making sure tests 1 and 2 run first to create the dependencies
for the rest

Peff, could you elaborate on your "load testing" setup? which could
give us any hints
on what to look for?, FWIW I hadn't been able to reproduce the problem anywhere
else (and not for a lack of trying)

> >   9255  1 git-http-backend RET   write 54/0x36
> >   9255  1 git-http-backend CALL  write(1,0xb781f0e0,0x94)
> >   9255  1 git-http-backend RET   write -1 errno 9 Bad file descriptor
>
> This is interesting. http-backend for some reason closes its
> stdout. Here it then tries to write there something. I have
> not seen it in my push_plain run. Maybe it worth redirecting instead
> to stderr, to avoid losing some diagnostics?

that should help with the garbled output from stderr, AFAIK the
process API allows creating
a pipe specifically for that with would be better than redirecting
stderr into stdout.

the fact we got EBADF means that there is a problem somewhere though
in the way the
previous failure that closed stdout got handled (which should had been
most likely in
the call to die)

Carlo

PS. upstreaming the PERL_PATH fix is likely to be good to do soonish
as I presume at least all BSD might be affected, let me know if you
would rather me do that instead as I suspect we might be deadlocked
otherwise ;)


Re: [PATCH v1 1/2] log -G: Ignore binary files

2018-11-22 Thread Ævar Arnfjörð Bjarmason
>
On Wed, Nov 21 2018, Thomas Braun wrote:

> The -G  option of log looks for the differences whose patch text
> contains added/removed lines that match regex.
>
> The concept of differences only makes sense for text files, therefore
> we need to ignore binary files when searching with -G  as well.
>
> Signed-off-by: Thomas Braun 
> ---
>  Documentation/gitdiffcore.txt |  2 +-
>  diffcore-pickaxe.c|  5 +
>  t/t4209-log-pickaxe.sh| 22 ++
>  3 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
> index c0a60f3158..059ddd3431 100644
> --- a/Documentation/gitdiffcore.txt
> +++ b/Documentation/gitdiffcore.txt
> @@ -242,7 +242,7 @@ textual diff has an added or a deleted line that matches 
> the given
>  regular expression.  This means that it will detect in-file (or what
>  rename-detection considers the same file) moves, which is noise.  The
>  implementation runs diff twice and greps, and this can be quite
> -expensive.
> +expensive.  Binary files without textconv filter are ignored.
>
>  When `-S` or `-G` are used without `--pickaxe-all`, only filepairs
>  that match their respective criterion are kept in the output.  When
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index 69fc55ea1e..8c2558b07d 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -144,6 +144,11 @@ static int pickaxe_match(struct diff_filepair *p, struct 
> diff_options *o,
>   textconv_two = get_textconv(o->repo->index, p->two);
>   }
>
> + if ((o->pickaxe_opts & DIFF_PICKAXE_KIND_G) &&
> + ((!textconv_one && diff_filespec_is_binary(o->repo, p->one)) ||
> +  (!textconv_two && diff_filespec_is_binary(o->repo, p->two
> + return 0;
> +
>   /*
>* If we have an unmodified pair, we know that the count will be the
>* same and don't even have to load the blobs. Unless textconv is in
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 844df760f7..42cc8afd8b 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -106,4 +106,26 @@ test_expect_success 'log -S --no-textconv (missing 
> textconv tool)' '
>   rm .gitattributes
>  '
>
> +test_expect_success 'log -G ignores binary files' '
> + rm -rf .git &&
> + git init &&
> + printf "a\0b" >data.bin &&
> + git add data.bin &&
> + git commit -m "message" &&
> + git log -G a >result &&

Would be less confusing as "-Ga" since that's the invocation we
document, even though I see (but wasn't aware that...) "-G a" works too.

> + test_must_be_empty result
> +'
> +
> +test_expect_success 'log -G looks into binary files with textconv filter' '
> + rm -rf .git &&
> + git init &&
> + echo "* diff=bin" > .gitattributes &&
> + printf "a\0b" >data.bin &&
> + git add data.bin &&
> + git commit -m "message" &&
> + git -c diff.bin.textconv=cat log -G a >actual &&
> + git log >expected &&
> + test_cmp actual expected
> +'
> +
>  test_done

This patch seems like the wrong direction to me. In particular the
assertion that "the concept of differences only makes sense for text
files". That's just not true. This patch breaks this:

(
rm -rf /tmp/g-test &&
git init /tmp/g-test &&
cd /tmp/g-test &&
for i in {1..10}; do
echo "Always matching thensome 5" >file &&
printf "a thensome %d binary \0" $i >>file &&
git add file &&
git commit -m"Bump $i"
done &&
git log -Gthensome.*5
)

Right now this will emit 3/10 patches, and the right ones! I.e. "Bump
[156]". The 1st one because it introduces the "Always matching thensome
5". Then 5/6 because the add/remove the string "a thensome 5 binary",
respectively. Which matches /thensome.*5/.

I.e. in the first one we do a regex match against the content here
because we don't have both sides:
https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L48-L53

And then for the later ones where we have both sides we end up in
diffgrep_consume():
https://github.com/git/git/blob/v2.19.2/diffcore-pickaxe.c#L27-L36

I think there may be a real issue here to address, which might be some
combination of:

 a) Even though the diffcore can do a binary diff internally, this is
not what it exposes with "-p", we just say "Binary files differ".

I don't know how to emit the raw version we'll end up passing to
diffgrep_consume() in this case. Is it just --binary without the
encoding? I don't know...

 b) Your test case shows that you're matching a string at a \0
boundary. Is this perhaps something you ran into? I.e. that we don't
have some -F version of -G so we can't supply regexes that match
past a \0? I had some related work on grep for this that hasn't been
carried over to the diffcore:

git log --grep='grep:.*\\0' --author=Ævar

 c) Is this binary 

Re: [PATCH v1 2/2] log -S: Add test which searches in binary files

2018-11-22 Thread Ævar Arnfjörð Bjarmason


On Wed, Nov 21 2018, Thomas Braun wrote:

> The -S  option of log looks for differences that changes the
> number of occurrences of the specified string (i.e. addition/deletion)
> in a file.
>
> Add a test to ensure that we keep looking into binary files with -S
> as changing that would break backwards compatibility in unexpected ways.
>
> Signed-off-by: Thomas Braun 
> ---
>  t/t4209-log-pickaxe.sh | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 42cc8afd8b..d430f6f2f9 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -128,4 +128,15 @@ test_expect_success 'log -G looks into binary files with 
> textconv filter' '
>   test_cmp actual expected
>  '
>
> +test_expect_success 'log -S looks into binary files' '
> + rm -rf .git &&
> + git init &&
> + printf "a\0b" >data.bin &&
> + git add data.bin &&
> + git commit -m "message" &&
> + git log -S a >actual &&
> + git log >expected &&
> + test_cmp actual expected
> +'
> +
>  test_done

This should just be part of 1/2 since the behavior is changed there &
the commit message should describe both cases.


Re: How to propagate critical fixs from master to develope branch.

2018-11-22 Thread Mateusz Loskot
On Thu, 22 Nov 2018 at 07:39, Mgr Georg Black  wrote:
> Hello everyone.I red git manual but I can't figure out how to propagate 
> critical change from master branch to long live develop branch.

Have you considered "using git cherry-pick can be used to backport
changes one by one" [1]

> I red chapter about rebasing that I think could solve it but at the end of 
> this chapter is written that it's not recommended for pubic repositories. I 
> don't know how to do it without merging develop branch to master.

rebase vs merge vs cherry-pick is a topic of constant debates and
opinionated practices indeed [2]

The cherry-pick has its cons, like duplicate commits (BTW, it is a
good idea to learn more about pros and cons of cherry-pick before you
apply this technique).
That is why workflows like GitFlow require to prepare a hotfix in a
topic branch which is merged back to long-lived branch.
In your case, I assume, you've done your work in long-lived master, so
I think easiest is to pick that change from master
and port it to the develop branch [3].

I would cherry-pick.

[1] https://www.atlassian.com/blog/git/the-essence-of-branch-based-workflows
[2] https://www.atlassian.com/git/articles/git-team-workflows-merge-or-rebase
[3] 
https://reallifeprogramming.com/git-process-that-works-say-no-to-gitflow-50bf2038ccf7

Best regards,
-- 
Mateusz Loskot, http://mateusz.loskot.net


Re: Git for Windows v2.20.0-rc0

2018-11-22 Thread stefan.naewe
Just a quick note:

I installed v2.20.0-rc0 with these options:

$ cat /etc/install-options.txt
Editor Option: VIM
Custom Editor Path:
Path Option: Cmd
SSH Option: OpenSSH
CURL Option: OpenSSL
CRLF Option: CRLFCommitAsIs
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Enabled
Enable Symlinks: Disabled
Enable Builtin Rebase: Enabled
Enable Builtin Stash: Enabled



When starting the git bash two windows pop up instead of one.
One that's "empty" and the other one containing the real git bash.

Thanks,
  Stefan

Am 20.11.2018 um 21:56 schrieb Johannes Schindelin:
> Team,
> 
> On Sun, 18 Nov 2018, Junio C Hamano wrote:
> 
>> An early preview release Git v2.20.0-rc0 is now available for
>> testing at the usual places.  It is comprised of 887 non-merge
>> commits since v2.19.0, contributed by 71 people, 23 of which are
>> new faces.
> 
> The "for Windows" flavor of Git v2.20.0-rc0 is available here:
> 
> https://github.com/git-for-windows/git/releases/tag/v2.20.0-rc0.windows.1
> 
> The current change log for v2.20.0 reads like this:
> 
> Changes since Git for Windows v2.19.1 (Oct 5th 2018)
> 
> Please note: Git CMD is deprecated as of this Git for Windows version. The
> default is to have git.exe in the PATH anyway, so there is no noticeable
> difference between CMD and Git CMD. It is impossible to turn off CMD's
> behavior where it picks up any git.exe in the current directory, so let's
> discourage the use of Git CMD. Users who dislike Git Bash should switch to
> Powershell instead.
> 
> New Features
> 
>   • Comes with OpenSSH v7.9p1.
>   • The description of the editor option to choose Vim has been clarified
> to state that this unsets core.editor.
>   • Comes with cURL v7.62.0.
>   • The type of symlinks to create (directory or file) can now be
> specified via the .gitattributes.
>   • The FSCache feature now uses a faster method to enumerate files,
> making e.g. git status faster in large repositories.
>   • Comes with Git Credential Manager v1.18.3.
>   • Comes with Git LFS v2.6.0.
>   • Comes with MSYS2 runtime (Git for Windows flavor) based on Cygwin
> 2.11.2.
>   • The FSCache feature was optimized to become faster.
> 
> Bug Fixes
> 
>   • The 64-bit Portable Git no longer sets pack.packSizeLimit.
> 
> Thanks,
> Johannes
> 


-- 

/dev/random says: To save trouble later, Joe named his cat Roadkill Fred
python -c "print 
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF