Re: [PATCH v6 00/15] rebase -i: offer to recreate commit topology

[Sergey Organov]

Hi Jacob,

Jacob Keller  writes:
> On Wed, Apr 11, 2018 at 6:13 AM, Sergey Organov  wrote:
>> It was rather --recreate-merges just a few weeks ago, and I've seen
>> nobody actually commented either in favor or against the
>> --rebase-merges.
>>
>> git rebase --rebase-merges
>>
>
> I'm going to jump in here and say that *I* prefer --rebase-merges, as
> it clearly mentions merge commits (which is the thing that changes).

OK, thanks, it's fair and the first argument in favor of --rebase-merges
I see.

I don't get why this detail matters so much it should be reflected in
the option name, and if it is what matters most, why the patch series
are not headed:


rebase -i: offer to rebase merge commits.

Once upon a time, I dreamt of an interactive rebase that would not
drop merge commits, but instead rebase them.


> I hadn't mentioned this before, because it was a suggestion that
> someone else made and it seemed that Johannes liked it, so I didn't
> think further discussion was worthwhile.

So you guys seem to be winning 2:1, or even 3:1, counting the guy who
made the suggestion. Except it was Buga's suggestion [1], and I believe
I was able to convince him that something like --no-flatten would be
better [2]:


> I hope he'd be pleased to be able to say --no-flatten=remerge and get
> back his current mode of operation, that he obviously has a good use
> for.

Makes sense, I like it, thanks for elaborating. [ Especially that you 
used "(no) flatten" phrasing, where original `--preserve-merges` 
documentation says it`s used "not to flatten the history", nice touch
;) ]


So I assume it's 2:2 by now, with the author of original suggestion on
my side.

I still find

git rebase --rebase-merges

both being ugly and missing the point.

When I look at it with a fresh eye, the questions that immediately rise
are: "What the hell else could 'git _rebase_' do with (merge) commits
but _rebase_ them? Why do I even need to specify this option? Should I
also specify --rebase-non-merges to rebase the rest of commits?"

Well, if it was called something like --[no-]keep-merges, it'd make more
sense as it'd be obvious that alternative is to drop merges (notice how
the old --preserve-merges does match this criteria). However, it'd still
miss to reflect the generic intent of the patch series, -- to preserve
history shape as much as possible, -- now citing author's head message
non-twisted: 


rebase -i: offer to recreate commit topology

Once upon a time, I dreamt of an interactive rebase that would not
flatten branch structure, but instead recreate the commit topology
faithfully.


-- Sergey

[1] https://public-inbox.org/git/bc9f82fb-fd18-ee45-36a4-921a1381b...@gmail.com/
[2] https://public-inbox.org/git/a3d40dca-f508-5853-89bc-1f9ab3934...@gmail.com/

Re: [PATCH v4] git{,-blame}.el: remove old bitrotting Emacs code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> However, since downstream packagers such as Debian are packaging this
> as git-el it's less disruptive to still carry these files as Elisp
> code that'll error out with a message suggesting alternatives, rather
> than drop the files entirely[2].
>
> Then rather than receive a cryptic load error when they upgrade
> existing users will get an error directing them to the README file, or
> to just stop requiring these modes. I think it makes sense to link to
> GitHub's hosting of contrib/emacs/README (which'll be updated by the
> time users see this) so they don't have to hunt down the packaged
> README on their local system.
> ...
>
>  contrib/emacs/.gitignore   |1 -
>  contrib/emacs/Makefile |   21 -
>  contrib/emacs/README   |   32 +-
>  contrib/emacs/git-blame.el |  489 +--
>  contrib/emacs/git.el   | 1710 +---
>  5 files changed, 25 insertions(+), 2228 deletions(-)
>  delete mode 100644 contrib/emacs/.gitignore
>  delete mode 100644 contrib/emacs/Makefile

I know I am to blame for prodding you to reopen this topic, but I am
wondering if removal of Makefile is sensible.  Is the assumption
that the distro packagers won't be using this Makefile at all and
have their own (e.g. debian/rules for Debian) build procedure, hence
*.el files are all they need to have?

The reason why I am wondering is because I do not know what distro
folks would do when they find that their build procedure does not
work---I suspect the would punt, and end users of the distro would
find that git-el package is no longer with them.  These end users
are whom this discussion is trying to help, but then to these
packagers, the reason why their build procedure no longer works does
not really matter, whether git.el is not shipped, or Makefile that
their debian/rules-equivalent depends on is not there, for them to
decide dropping the git-el package.

On the other hand, the 6-lines of e-lisp you wrote for git.el
replacement is something the packagers could have written for their
users, so (1) if we really want to go extra mile without trusting
that distro packagers are less competent than us in helping their
users, we'd be better off to leave Makefile in, or (2) if we trust
packagers and leave possible end-user confusion as their problem
(not ours), then we can just remove as your previous round did.

And from that point of view, I find this round slightly odd.

Re: [Git] recursive merge on 'master' severely broken?

[Junio C Hamano]

Elijah Newren  writes:

> On Wed, Apr 11, 2018 at 12:19 AM, Junio C Hamano  wrote:
>> It appears that a topic recently graduated to 'master' introduces a
>> severe regression to "git merge" when it merges a side branch that
>> renames paths while the trunk has further updates.
>>
>> The symptom can easily be seen by trying to recreate the merge I
>> made at the tip of 'pu' 29dea678 ("Merge branch
>> 'sb/filenames-with-dashes' into pu", 2018-04-11) that I'll be.
>> pushing out shortly.  The side branch renames a file exec_cmd.h to
>> exec-cmd.h (an underscore changed to a dash) without changing any
>> contents, while the branch being merged to has made some changes to
>> the contents while keeping the original pathname.
> ...
> I agree, that is _really_ bad.  My sincerest apologies.  I'll dig into it.

Thanks.  It is not unusual for a moderately large set of changes to
have glitches that need time to be discovered.  I was hoping that
placing it in 'next' and keeping it there for several weeks would
have been sufficient for people who do use stuff from there in real
life but apparently that wasn't the case.

[PATCH 10/15] replace-object: add repository argument to lookup_replace_object

[Stefan Beller]

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

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

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 builtin/mktag.c  | 2 +-
 object.c | 2 +-
 replace-object.h | 3 ++-
 sha1_file.c  | 6 +++---
 streaming.c  | 2 +-
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/mktag.c b/builtin/mktag.c
index e3d20a7722..82a6e86077 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -25,7 +25,7 @@ static int verify_object(const struct object_id *oid, const 
char *expected_type)
enum object_type type;
unsigned long size;
void *buffer = read_object_file(oid, , );
-   const struct object_id *repl = lookup_replace_object(oid);
+   const struct object_id *repl = lookup_replace_object(the_repository, 
oid);
 
if (buffer) {
if (type == type_from_string(expected_type))
diff --git a/object.c b/object.c
index 998ec2a25f..66cffaf6e5 100644
--- a/object.c
+++ b/object.c
@@ -247,7 +247,7 @@ struct object *parse_object(const struct object_id *oid)
unsigned long size;
enum object_type type;
int eaten;
-   const struct object_id *repl = lookup_replace_object(oid);
+   const struct object_id *repl = lookup_replace_object(the_repository, 
oid);
void *buffer;
struct object *obj;
 
diff --git a/replace-object.h b/replace-object.h
index ddeb0470bd..dff57bfa1e 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -23,7 +23,8 @@ extern const struct object_id 
*do_lookup_replace_object_the_repository(const str
  * either sha1 or a pointer to a permanently-allocated value.  When
  * object replacement is suppressed, always return sha1.
  */
-static inline const struct object_id *lookup_replace_object(const struct 
object_id *oid)
+#define lookup_replace_object(r, s) lookup_replace_object_##r(s)
+static inline const struct object_id 
*lookup_replace_object_the_repository(const struct object_id *oid)
 {
if (!check_replace_refs ||
(the_repository->objects->replace_map &&
diff --git a/sha1_file.c b/sha1_file.c
index c38e41e49e..028a4357c5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1240,7 +1240,7 @@ int oid_object_info_extended(const struct object_id *oid, 
struct object_info *oi
int already_retried = 0;
 
if (flags & OBJECT_INFO_LOOKUP_REPLACE)
-   real = lookup_replace_object(oid);
+   real = lookup_replace_object(the_repository, oid);
 
if (is_null_oid(real))
return -1;
@@ -1379,8 +1379,8 @@ void *read_object_file_extended(const struct object_id 
*oid,
const struct packed_git *p;
const char *path;
struct stat st;
-   const struct object_id *repl = lookup_replace ? 
lookup_replace_object(oid)
- : oid;
+   const struct object_id *repl = lookup_replace ?
+   lookup_replace_object(the_repository, oid) : oid;
 
errno = 0;
data = read_object(repl->hash, type, size);
diff --git a/streaming.c b/streaming.c
index a6e1162946..cce7b17ea7 100644
--- a/streaming.c
+++ b/streaming.c
@@ -140,7 +140,7 @@ struct git_istream *open_istream(const struct object_id 
*oid,
 {
struct git_istream *st;
struct object_info oi = OBJECT_INFO_INIT;
-   const struct object_id *real = lookup_replace_object(oid);
+   const struct object_id *real = lookup_replace_object(the_repository, 
oid);
enum input_source src = istream_source(real, type, );
 
if (src < 0)
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 06/15] refs: add repository argument to get_main_ref_store

[Stefan Beller]

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

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

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 builtin/pack-refs.c   |  3 +-
 refs.c| 67 ---
 refs.h|  4 ++-
 revision.c|  5 +--
 t/helper/test-ref-store.c |  3 +-
 5 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index b106a392a4..f3353564f9 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "parse-options.h"
 #include "refs.h"
+#include "repository.h"
 
 static char const * const pack_refs_usage[] = {
N_("git pack-refs []"),
@@ -17,5 +18,5 @@ int cmd_pack_refs(int argc, const char **argv, const char 
*prefix)
};
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
-   return refs_pack_refs(get_main_ref_store(), flags);
+   return refs_pack_refs(get_main_ref_store(the_repository), flags);
 }
diff --git a/refs.c b/refs.c
index 8b7a77fe5e..74d4ed97cb 100644
--- a/refs.c
+++ b/refs.c
@@ -13,6 +13,7 @@
 #include "tag.h"
 #include "submodule.h"
 #include "worktree.h"
+#include "repository.h"
 
 /*
  * List of all available backends
@@ -206,7 +207,7 @@ char *refs_resolve_refdup(struct ref_store *refs,
 char *resolve_refdup(const char *refname, int resolve_flags,
 struct object_id *oid, int *flags)
 {
-   return refs_resolve_refdup(get_main_ref_store(),
+   return refs_resolve_refdup(get_main_ref_store(the_repository),
   refname, resolve_flags,
   oid, flags);
 }
@@ -228,7 +229,7 @@ int refs_read_ref_full(struct ref_store *refs, const char 
*refname,
 
 int read_ref_full(const char *refname, int resolve_flags, struct object_id 
*oid, int *flags)
 {
-   return refs_read_ref_full(get_main_ref_store(), refname,
+   return refs_read_ref_full(get_main_ref_store(the_repository), refname,
  resolve_flags, oid, flags);
 }
 
@@ -375,7 +376,7 @@ int refs_for_each_tag_ref(struct ref_store *refs, 
each_ref_fn fn, void *cb_data)
 
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 {
-   return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data);
+   return refs_for_each_tag_ref(get_main_ref_store(the_repository), fn, 
cb_data);
 }
 
 int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
@@ -385,7 +386,7 @@ int refs_for_each_branch_ref(struct ref_store *refs, 
each_ref_fn fn, void *cb_da
 
 int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 {
-   return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data);
+   return refs_for_each_branch_ref(get_main_ref_store(the_repository), fn, 
cb_data);
 }
 
 int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
@@ -395,7 +396,7 @@ int refs_for_each_remote_ref(struct ref_store *refs, 
each_ref_fn fn, void *cb_da
 
 int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 {
-   return refs_for_each_remote_ref(get_main_ref_store(), fn, cb_data);
+   return refs_for_each_remote_ref(get_main_ref_store(the_repository), fn, 
cb_data);
 }
 
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
@@ -730,7 +731,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
struct strbuf err = STRBUF_INIT;
 
if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
-   assert(refs == get_main_ref_store());
+   assert(refs == get_main_ref_store(the_repository));
return delete_pseudoref(refname, old_oid);
}
 
@@ -752,7 +753,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
 int delete_ref(const char *msg, const char *refname,
   const struct object_id *old_oid, unsigned int flags)
 {
-   return refs_delete_ref(get_main_ref_store(), msg, refname,
+   return refs_delete_ref(get_main_ref_store(the_repository), msg, refname,
   old_oid, flags);
 }
 
@@ -928,7 +929,7 @@ struct ref_transaction *ref_store_transaction_begin(struct 
ref_store *refs,
 
 struct ref_transaction *ref_transaction_begin(struct strbuf *err)
 {
-   return ref_store_transaction_begin(get_main_ref_store(), err);
+   return ref_store_transaction_begin(get_main_ref_store(the_repository), 
err);
 }
 
 void ref_transaction_free(struct ref_transaction *transaction)
@@ -1060,7 +1061,7 @@ int refs_update_ref(struct ref_store *refs, const char 
*msg,
   

[PATCH 11/15] refs: store the main ref store inside the repository struct

[Stefan Beller]

This moves the 'main_ref_store', which was a global variable in refs.c
into the repository struct.

This patch does not deal with the parts in the refs subsystem which deal
with the submodules there. A later patch needs to get rid of the submodule
exposure in the refs API, such as 'get_submodule_ref_store(path)'.

Acked-by: Michael Haggerty 
Signed-off-by: Stefan Beller 
---
 refs.c   | 13 +
 refs.h   |  4 +---
 refs/files-backend.c |  4 
 repository.h |  3 +++
 4 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/refs.c b/refs.c
index f58b9fb7df..36df1bc73a 100644
--- a/refs.c
+++ b/refs.c
@@ -1608,9 +1608,6 @@ static struct ref_store_hash_entry 
*alloc_ref_store_hash_entry(
return entry;
 }
 
-/* A pointer to the ref_store for the main repository: */
-static struct ref_store *main_ref_store;
-
 /* A hashmap of ref_stores, stored by submodule name: */
 static struct hashmap submodule_ref_stores;
 
@@ -1652,13 +1649,13 @@ static struct ref_store *ref_store_init(const char 
*gitdir,
return refs;
 }
 
-struct ref_store *get_main_ref_store_the_repository(void)
+struct ref_store *get_main_ref_store(struct repository *r)
 {
-   if (main_ref_store)
-   return main_ref_store;
+   if (r->refs)
+   return r->refs;
 
-   main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS);
-   return main_ref_store;
+   r->refs = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
+   return r->refs;
 }
 
 /*
diff --git a/refs.h b/refs.h
index ab3d2bec2f..f5ab68c0ed 100644
--- a/refs.h
+++ b/refs.h
@@ -760,9 +760,7 @@ int reflog_expire(const char *refname, const struct 
object_id *oid,
 
 int ref_storage_backend_exists(const char *name);
 
-#define get_main_ref_store(r) \
-   get_main_ref_store_##r()
-struct ref_store *get_main_ref_store_the_repository(void);
+struct ref_store *get_main_ref_store(struct repository *r);
 /*
  * Return the ref_store instance for the specified submodule. For the
  * main repository, use submodule==NULL; such a call cannot fail. For
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bec8e30e9e..5c76a75817 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -61,10 +61,6 @@ struct ref_lock {
struct object_id old_oid;
 };
 
-/*
- * Future: need to be in "struct repository"
- * when doing a full libification.
- */
 struct files_ref_store {
struct ref_store base;
unsigned int store_flags;
diff --git a/repository.h b/repository.h
index 09df94a472..e6e00f541b 100644
--- a/repository.h
+++ b/repository.h
@@ -26,6 +26,9 @@ struct repository {
 */
struct raw_object_store *objects;
 
+   /* The store in which the refs are held. */
+   struct ref_store *refs;
+
/*
 * Path to the repository's graft file.
 * Cannot be NULL after initialization.
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 09/15] replace-object: add repository argument to do_lookup_replace_object

[Stefan Beller]

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

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

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 replace-object.h | 5 +++--
 replace_object.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/replace-object.h b/replace-object.h
index dbc51265ec..ddeb0470bd 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -14,7 +14,8 @@ struct replace_object {
  * This internal function is only declared here for the benefit of
  * lookup_replace_object().  Please do not call it directly.
  */
-extern const struct object_id *do_lookup_replace_object(const struct object_id 
*oid);
+#define do_lookup_replace_object(r, s) do_lookup_replace_object_##r(s)
+extern const struct object_id *do_lookup_replace_object_the_repository(const 
struct object_id *oid);
 
 /*
  * If object sha1 should be replaced, return the replacement object's
@@ -28,7 +29,7 @@ static inline const struct object_id 
*lookup_replace_object(const struct object_
(the_repository->objects->replace_map &&
 the_repository->objects->replace_map->map.tablesize == 0))
return oid;
-   return do_lookup_replace_object(oid);
+   return do_lookup_replace_object(the_repository, oid);
 }
 
 #endif /* REPLACE_OBJECT_H */
diff --git a/replace_object.c b/replace_object.c
index 567d9da708..adfed78901 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -55,7 +55,7 @@ static void prepare_replace_object_the_repository(void)
  * permanently-allocated value.  This function always respects replace
  * references, regardless of the value of check_replace_refs.
  */
-const struct object_id *do_lookup_replace_object(const struct object_id *oid)
+const struct object_id *do_lookup_replace_object_the_repository(const struct 
object_id *oid)
 {
int depth = MAXREPLACEDEPTH;
const struct object_id *cur = oid;
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 08/15] replace-object: add repository argument to prepare_replace_object

[Stefan Beller]

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

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

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 replace_object.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index 16a95ea416..567d9da708 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -31,7 +31,9 @@ static int register_replace_ref(const char *refname,
return 0;
 }
 
-static void prepare_replace_object(void)
+#define prepare_replace_object(r) \
+   prepare_replace_object_##r()
+static void prepare_replace_object_the_repository(void)
 {
if (the_repository->objects->replace_map)
return;
@@ -58,7 +60,7 @@ const struct object_id *do_lookup_replace_object(const struct 
object_id *oid)
int depth = MAXREPLACEDEPTH;
const struct object_id *cur = oid;
 
-   prepare_replace_object();
+   prepare_replace_object(the_repository);
 
/* Try to recursively replace the object */
while (depth-- > 0) {
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 05/15] replace-object: check_replace_refs is safe in multi repo environment

[Stefan Beller]

In ecef23 (inline lookup_replace_object() calls, 2011-05-15) a shortcut
for checking the object replacement was added by setting check_replace_refs
to 0 once the replacements were evaluated to not exist. This works fine in
with the assumption of only one repository in existence.

The assumption won't hold true any more when we work on multiple instances
of a repository structs (e.g. one struct per submodule), as the first
repository to be inspected may have no replacements and would set the
global variable. Other repositories would then completely omit their
evaluation of replacements.

This reverts back the meaning of the flag `check_replace_refs` of
"Do we need to check with the lookup table?" to "Do we need to read
the replacement definition?", adding the bypassing logic to
lookup_replace_object after the replacement definition was read.
As with the original patch, delay the renaming of the global variable

Signed-off-by: Stefan Beller 
---
 environment.c| 2 +-
 replace-object.h | 5 -
 replace_object.c | 3 ---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/environment.c b/environment.c
index 39b3d906c8..b991fc0a87 100644
--- a/environment.c
+++ b/environment.c
@@ -50,7 +50,7 @@ const char *editor_program;
 const char *askpass_program;
 const char *excludes_file;
 enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;
-int check_replace_refs = 1;
+int check_replace_refs = 1; /* NEEDSWORK: rename to read_replace_refs */
 char *git_replace_ref_base;
 enum eol core_eol = EOL_UNSET;
 int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
diff --git a/replace-object.h b/replace-object.h
index 15315311fb..dbc51265ec 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -3,6 +3,7 @@
 
 #include "oidmap.h"
 #include "repository.h"
+#include "object-store.h"
 
 struct replace_object {
struct oidmap_entry original;
@@ -23,7 +24,9 @@ extern const struct object_id *do_lookup_replace_object(const 
struct object_id *
  */
 static inline const struct object_id *lookup_replace_object(const struct 
object_id *oid)
 {
-   if (!check_replace_refs)
+   if (!check_replace_refs ||
+   (the_repository->objects->replace_map &&
+the_repository->objects->replace_map->map.tablesize == 0))
return oid;
return do_lookup_replace_object(oid);
 }
diff --git a/replace_object.c b/replace_object.c
index 953fa9cc40..b2405f6027 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -41,9 +41,6 @@ static void prepare_replace_object(void)
oidmap_init(the_repository->objects->replace_map, 0);
 
for_each_replace_ref(register_replace_ref, NULL);
-
-   if (!the_repository->objects->replace_map->map.tablesize)
-   check_replace_refs = 0;
 }
 
 /* We allow "recursive" replacement. Only within reason, though */
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 04/15] replace-object: eliminate replace objects prepared flag

[Stefan Beller]

Make the oidmap a pointer.

That way we eliminate the need for the global boolean
variable 'replace_object_prepared' as we can put this information
into the pointer being NULL or not.

Another advantage of this is that we would more quickly catch
code that tries to access replace-map without initializing it.

This also allows the '#include "oidmap.h"' introduced in a previous
patch to be replaced by the forward declaration of 'struct oidmap;'.
Keeping the type opaque discourages circumventing accessor functions;
not dragging in other headers avoids some compile time overhead.

One disadvantage of this is change is performance as we need to
pay the overhead for a malloc. The alternative of moving the
global variable into the object store is less modular code.

Helped-by: René Scharfe 
Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
 object-store.h   |  2 +-
 replace_object.c | 16 +---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/object-store.h b/object-store.h
index c04b4c95eb..1ff862c7f9 100644
--- a/object-store.h
+++ b/object-store.h
@@ -99,7 +99,7 @@ struct raw_object_store {
 * Objects that should be substituted by other objects
 * (see git-replace(1)).
 */
-   struct oidmap replace_map;
+   struct oidmap *replace_map;
 
/*
 * private data
diff --git a/replace_object.c b/replace_object.c
index afbdf2df25..953fa9cc40 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -25,7 +25,7 @@ static int register_replace_ref(const char *refname,
oidcpy(_obj->replacement, oid);
 
/* Register new object */
-   if (oidmap_put(_repository->objects->replace_map, repl_obj))
+   if (oidmap_put(the_repository->objects->replace_map, repl_obj))
die("duplicate replace ref: %s", refname);
 
return 0;
@@ -33,14 +33,16 @@ static int register_replace_ref(const char *refname,
 
 static void prepare_replace_object(void)
 {
-   static int replace_object_prepared;
-
-   if (replace_object_prepared)
+   if (the_repository->objects->replace_map)
return;
 
+   the_repository->objects->replace_map =
+   xmalloc(sizeof(*the_repository->objects->replace_map));
+   oidmap_init(the_repository->objects->replace_map, 0);
+
for_each_replace_ref(register_replace_ref, NULL);
-   replace_object_prepared = 1;
-   if (!the_repository->objects->replace_map.map.tablesize)
+
+   if (!the_repository->objects->replace_map->map.tablesize)
check_replace_refs = 0;
 }
 
@@ -64,7 +66,7 @@ const struct object_id *do_lookup_replace_object(const struct 
object_id *oid)
/* Try to recursively replace the object */
while (depth-- > 0) {
struct replace_object *repl_obj =
-   oidmap_get(_repository->objects->replace_map, cur);
+   oidmap_get(the_repository->objects->replace_map, cur);
if (!repl_obj)
return cur;
cur = _obj->replacement;
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 03/15] object-store: move lookup_replace_object to replace-object.h

[Stefan Beller]

lookup_replace_object is a low-level function that most users of the
object store do not need to use directly.

Move it to replace-object.h to avoid a dependency loop in an upcoming
change to its inline definition that will make use of repository.h.

Signed-off-by: Stefan Beller 
Signed-off-by: Jonathan Nieder 
---
 builtin/mktag.c  |  1 +
 cache.h  | 19 ---
 object.c |  1 +
 replace-object.h | 22 ++
 sha1_file.c  |  1 +
 streaming.c  |  1 +
 6 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/builtin/mktag.c b/builtin/mktag.c
index 9f5a50a8fd..e3d20a7722 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "tag.h"
+#include "replace-object.h"
 
 /*
  * A signature file has a very simple fixed format: four lines
diff --git a/cache.h b/cache.h
index a5c4fddf77..e3c6cba514 100644
--- a/cache.h
+++ b/cache.h
@@ -1187,25 +1187,6 @@ static inline void *read_object_file(const struct 
object_id *oid, enum object_ty
return read_object_file_extended(oid, type, size, 1);
 }
 
-/*
- * This internal function is only declared here for the benefit of
- * lookup_replace_object().  Please do not call it directly.
- */
-extern const struct object_id *do_lookup_replace_object(const struct object_id 
*oid);
-
-/*
- * If object sha1 should be replaced, return the replacement object's
- * name (replaced recursively, if necessary).  The return value is
- * either sha1 or a pointer to a permanently-allocated value.  When
- * object replacement is suppressed, always return sha1.
- */
-static inline const struct object_id *lookup_replace_object(const struct 
object_id *oid)
-{
-   if (!check_replace_refs)
-   return oid;
-   return do_lookup_replace_object(oid);
-}
-
 /* Read and unpack an object file into memory, write memory to an object file 
*/
 extern int oid_object_info(const struct object_id *, unsigned long *);
 
diff --git a/object.c b/object.c
index a0a756f24f..998ec2a25f 100644
--- a/object.c
+++ b/object.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "object.h"
+#include "replace-object.h"
 #include "blob.h"
 #include "tree.h"
 #include "commit.h"
diff --git a/replace-object.h b/replace-object.h
index f9a2b70eb8..15315311fb 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -1,9 +1,31 @@
 #ifndef REPLACE_OBJECT_H
 #define REPLACE_OBJECT_H
 
+#include "oidmap.h"
+#include "repository.h"
+
 struct replace_object {
struct oidmap_entry original;
struct object_id replacement;
 };
 
+/*
+ * This internal function is only declared here for the benefit of
+ * lookup_replace_object().  Please do not call it directly.
+ */
+extern const struct object_id *do_lookup_replace_object(const struct object_id 
*oid);
+
+/*
+ * If object sha1 should be replaced, return the replacement object's
+ * name (replaced recursively, if necessary).  The return value is
+ * either sha1 or a pointer to a permanently-allocated value.  When
+ * object replacement is suppressed, always return sha1.
+ */
+static inline const struct object_id *lookup_replace_object(const struct 
object_id *oid)
+{
+   if (!check_replace_refs)
+   return oid;
+   return do_lookup_replace_object(oid);
+}
+
 #endif /* REPLACE_OBJECT_H */
diff --git a/sha1_file.c b/sha1_file.c
index 3e0af41892..c38e41e49e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -23,6 +23,7 @@
 #include "sha1-lookup.h"
 #include "bulk-checkin.h"
 #include "repository.h"
+#include "replace-object.h"
 #include "streaming.h"
 #include "dir.h"
 #include "list.h"
diff --git a/streaming.c b/streaming.c
index 7d55ba64c7..a6e1162946 100644
--- a/streaming.c
+++ b/streaming.c
@@ -5,6 +5,7 @@
 #include "streaming.h"
 #include "repository.h"
 #include "object-store.h"
+#include "replace-object.h"
 #include "packfile.h"
 
 enum input_source {
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 12/15] refs: allow for_each_replace_ref to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 refs.c | 4 ++--
 refs.h | 4 +---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 36df1bc73a..9b56fa9b81 100644
--- a/refs.c
+++ b/refs.c
@@ -1415,9 +1415,9 @@ int refs_for_each_fullref_in(struct ref_store *refs, 
const char *prefix,
return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_replace_ref_the_repository(each_ref_fn fn, void *cb_data)
+int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data)
 {
-   return do_for_each_ref(get_main_ref_store(the_repository),
+   return do_for_each_ref(get_main_ref_store(r),
   git_replace_ref_base, fn,
   strlen(git_replace_ref_base),
   DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
diff --git a/refs.h b/refs.h
index f5ab68c0ed..15f3a91cc4 100644
--- a/refs.h
+++ b/refs.h
@@ -300,9 +300,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, 
void *cb_data,
 int for_each_tag_ref(each_ref_fn fn, void *cb_data);
 int for_each_branch_ref(each_ref_fn fn, void *cb_data);
 int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-#define for_each_replace_ref(r, fn, cb) \
-   for_each_replace_ref_##r(fn, cb)
-int for_each_replace_ref_the_repository(each_ref_fn fn, void *cb_data);
+int for_each_replace_ref(struct repository *r, each_ref_fn fn, void *cb_data);
 int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
 int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 const char *prefix, void *cb_data);
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 14/15] replace-object: allow do_lookup_replace_object to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 replace-object.h | 4 ++--
 replace_object.c | 7 ---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/replace-object.h b/replace-object.h
index dff57bfa1e..3520fd7ff7 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -14,8 +14,8 @@ struct replace_object {
  * This internal function is only declared here for the benefit of
  * lookup_replace_object().  Please do not call it directly.
  */
-#define do_lookup_replace_object(r, s) do_lookup_replace_object_##r(s)
-extern const struct object_id *do_lookup_replace_object_the_repository(const 
struct object_id *oid);
+extern const struct object_id *do_lookup_replace_object(struct repository *r,
+   const struct object_id 
*oid);
 
 /*
  * If object sha1 should be replaced, return the replacement object's
diff --git a/replace_object.c b/replace_object.c
index eae52c66f3..246b98cd4f 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -53,17 +53,18 @@ static void prepare_replace_object(struct repository *r)
  * permanently-allocated value.  This function always respects replace
  * references, regardless of the value of check_replace_refs.
  */
-const struct object_id *do_lookup_replace_object_the_repository(const struct 
object_id *oid)
+const struct object_id *do_lookup_replace_object(struct repository *r,
+const struct object_id *oid)
 {
int depth = MAXREPLACEDEPTH;
const struct object_id *cur = oid;
 
-   prepare_replace_object(the_repository);
+   prepare_replace_object(r);
 
/* Try to recursively replace the object */
while (depth-- > 0) {
struct replace_object *repl_obj =
-   oidmap_get(the_repository->objects->replace_map, cur);
+   oidmap_get(r->objects->replace_map, cur);
if (!repl_obj)
return cur;
cur = _obj->replacement;
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 02/15] replace-object: move replace_map to object store

[Stefan Beller]

The relationship between an object X and another object Y that
replaces the object X is defined only within the scope of a
single repository.

The exception in reachability rule around these replacement objects
is also local to a repository (i.e. if traversal from refs reaches
X, then both X and Y are reachable and need to be kept from gc).

Signed-off-by: Stefan Beller 
---
 object-store.h   |  8 
 replace-object.h |  9 +
 replace_object.c | 17 +++--
 3 files changed, 24 insertions(+), 10 deletions(-)
 create mode 100644 replace-object.h

diff --git a/object-store.h b/object-store.h
index fef33f345f..c04b4c95eb 100644
--- a/object-store.h
+++ b/object-store.h
@@ -1,6 +1,8 @@
 #ifndef OBJECT_STORE_H
 #define OBJECT_STORE_H
 
+#include "oidmap.h"
+
 struct alternate_object_database {
struct alternate_object_database *next;
 
@@ -93,6 +95,12 @@ struct raw_object_store {
struct alternate_object_database *alt_odb_list;
struct alternate_object_database **alt_odb_tail;
 
+   /*
+* Objects that should be substituted by other objects
+* (see git-replace(1)).
+*/
+   struct oidmap replace_map;
+
/*
 * private data
 *
diff --git a/replace-object.h b/replace-object.h
new file mode 100644
index 00..f9a2b70eb8
--- /dev/null
+++ b/replace-object.h
@@ -0,0 +1,9 @@
+#ifndef REPLACE_OBJECT_H
+#define REPLACE_OBJECT_H
+
+struct replace_object {
+   struct oidmap_entry original;
+   struct object_id replacement;
+};
+
+#endif /* REPLACE_OBJECT_H */
diff --git a/replace_object.c b/replace_object.c
index a757a5ebf2..afbdf2df25 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -1,15 +1,11 @@
 #include "cache.h"
 #include "oidmap.h"
+#include "object-store.h"
+#include "replace-object.h"
 #include "refs.h"
+#include "repository.h"
 #include "commit.h"
 
-struct replace_object {
-   struct oidmap_entry original;
-   struct object_id replacement;
-};
-
-static struct oidmap replace_map = OIDMAP_INIT;
-
 static int register_replace_ref(const char *refname,
const struct object_id *oid,
int flag, void *cb_data)
@@ -29,7 +25,7 @@ static int register_replace_ref(const char *refname,
oidcpy(_obj->replacement, oid);
 
/* Register new object */
-   if (oidmap_put(_map, repl_obj))
+   if (oidmap_put(_repository->objects->replace_map, repl_obj))
die("duplicate replace ref: %s", refname);
 
return 0;
@@ -44,7 +40,7 @@ static void prepare_replace_object(void)
 
for_each_replace_ref(register_replace_ref, NULL);
replace_object_prepared = 1;
-   if (!replace_map.map.tablesize)
+   if (!the_repository->objects->replace_map.map.tablesize)
check_replace_refs = 0;
 }
 
@@ -67,7 +63,8 @@ const struct object_id *do_lookup_replace_object(const struct 
object_id *oid)
 
/* Try to recursively replace the object */
while (depth-- > 0) {
-   struct replace_object *repl_obj = oidmap_get(_map, cur);
+   struct replace_object *repl_obj =
+   oidmap_get(_repository->objects->replace_map, cur);
if (!repl_obj)
return cur;
cur = _obj->replacement;
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 15/15] replace-object: allow lookup_replace_object to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 replace-object.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/replace-object.h b/replace-object.h
index 3520fd7ff7..9f607a929b 100644
--- a/replace-object.h
+++ b/replace-object.h
@@ -23,14 +23,14 @@ extern const struct object_id 
*do_lookup_replace_object(struct repository *r,
  * either sha1 or a pointer to a permanently-allocated value.  When
  * object replacement is suppressed, always return sha1.
  */
-#define lookup_replace_object(r, s) lookup_replace_object_##r(s)
-static inline const struct object_id 
*lookup_replace_object_the_repository(const struct object_id *oid)
+static inline const struct object_id *lookup_replace_object(struct repository 
*r,
+   const struct 
object_id *oid)
 {
if (!check_replace_refs ||
-   (the_repository->objects->replace_map &&
-the_repository->objects->replace_map->map.tablesize == 0))
+   (r->objects->replace_map &&
+r->objects->replace_map->map.tablesize == 0))
return oid;
-   return do_lookup_replace_object(the_repository, oid);
+   return do_lookup_replace_object(r, oid);
 }
 
 #endif /* REPLACE_OBJECT_H */
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 13/15] replace-object: allow prepare_replace_object to handle arbitrary repositories

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 replace_object.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index adfed78901..eae52c66f3 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -31,18 +31,16 @@ static int register_replace_ref(const char *refname,
return 0;
 }
 
-#define prepare_replace_object(r) \
-   prepare_replace_object_##r()
-static void prepare_replace_object_the_repository(void)
+static void prepare_replace_object(struct repository *r)
 {
-   if (the_repository->objects->replace_map)
+   if (r->objects->replace_map)
return;
 
-   the_repository->objects->replace_map =
+   r->objects->replace_map =
xmalloc(sizeof(*the_repository->objects->replace_map));
-   oidmap_init(the_repository->objects->replace_map, 0);
+   oidmap_init(r->objects->replace_map, 0);
 
-   for_each_replace_ref(the_repository, register_replace_ref, NULL);
+   for_each_replace_ref(r, register_replace_ref, NULL);
 }
 
 /* We allow "recursive" replacement. Only within reason, though */
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 07/15] refs: add repository argument to for_each_replace_ref

[Stefan Beller]

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

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

Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 builtin/replace.c | 4 +++-
 refs.c| 2 +-
 refs.h| 4 +++-
 replace_object.c  | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 19006e52bc..ef8145e556 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -14,6 +14,8 @@
 #include "refs.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "object-store.h"
+#include "repository.h"
 #include "tag.h"
 
 static const char * const git_replace_usage[] = {
@@ -83,7 +85,7 @@ static int list_replace_refs(const char *pattern, const char 
*format)
"valid formats are 'short', 'medium' and 'long'\n",
format);
 
-   for_each_replace_ref(show_reference, (void *));
+   for_each_replace_ref(the_repository, show_reference, (void *));
 
return 0;
 }
diff --git a/refs.c b/refs.c
index 74d4ed97cb..f58b9fb7df 100644
--- a/refs.c
+++ b/refs.c
@@ -1415,7 +1415,7 @@ int refs_for_each_fullref_in(struct ref_store *refs, 
const char *prefix,
return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_replace_ref(each_ref_fn fn, void *cb_data)
+int for_each_replace_ref_the_repository(each_ref_fn fn, void *cb_data)
 {
return do_for_each_ref(get_main_ref_store(the_repository),
   git_replace_ref_base, fn,
diff --git a/refs.h b/refs.h
index 0d013377ce..ab3d2bec2f 100644
--- a/refs.h
+++ b/refs.h
@@ -300,7 +300,9 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, 
void *cb_data,
 int for_each_tag_ref(each_ref_fn fn, void *cb_data);
 int for_each_branch_ref(each_ref_fn fn, void *cb_data);
 int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-int for_each_replace_ref(each_ref_fn fn, void *cb_data);
+#define for_each_replace_ref(r, fn, cb) \
+   for_each_replace_ref_##r(fn, cb)
+int for_each_replace_ref_the_repository(each_ref_fn fn, void *cb_data);
 int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
 int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 const char *prefix, void *cb_data);
diff --git a/replace_object.c b/replace_object.c
index b2405f6027..16a95ea416 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -40,7 +40,7 @@ static void prepare_replace_object(void)
xmalloc(sizeof(*the_repository->objects->replace_map));
oidmap_init(the_repository->objects->replace_map, 0);
 
-   for_each_replace_ref(register_replace_ref, NULL);
+   for_each_replace_ref(the_repository, register_replace_ref, NULL);
 }
 
 /* We allow "recursive" replacement. Only within reason, though */
-- 
2.17.0.484.g0c8726318c-goog

[PATCHv3 00/15] replace_object.c: rename to use dash in file name

[Stefan Beller]

v3:
* interdiff below,
  the only changes are renaming the variable 
  -   struct ref_store *main_ref_store;
  +   struct ref_store *refs;
  in struct repository.
  as well as dropping the file rename patch.
* improved commit messages from discussion on the single patches.

v2:
This applies on top of a merge of
origin/bc/object-id and origin/sb/packfiles-in-repository,
both of which are pending merge to master. It is also available at
https://github.com/stefanbeller/git/tree/object-store-3

* removed whitespaces as noted by Stolee
* incorporated Renes patch as the first patch of this series
  (It may go independently if this series takes too long)
* Adressed Erics concern regarding sloppy commit messages
  (removed #Conflict markers), typo in comment
* I did not drop the main_ from the ref store, yet, as asked by Duy.

Thanks,
Stefan

v1:
This applies on top of 464416a2eaadf84d2bfdf795007863d03b222b7c
(sb/packfiles-in-repository).
It is also available at https://github.com/stefanbeller/git/tree/object-store-3

This series will bring the replacement mechanism (git replace)
into the object store.

The first patches are cleaning up a bit, and patches 6-19 are converting
one function at a time using the tick-tock pattern with the #define trick.
See cfc62fc98c (sha1_file: add repository argument to link_alt_odb_entry,
2018-03-23) for explanation.

Thanks,
Stefan

René Scharfe (1):
  replace_object: use oidmap

Stefan Beller (14):
  replace-object: move replace_map to object store
  object-store: move lookup_replace_object to replace-object.h
  replace-object: eliminate replace objects prepared flag
  replace-object: check_replace_refs is safe in multi repo environment
  refs: add repository argument to get_main_ref_store
  refs: add repository argument to for_each_replace_ref
  replace-object: add repository argument to prepare_replace_object
  replace-object: add repository argument to do_lookup_replace_object
  replace-object: add repository argument to lookup_replace_object
  refs: store the main ref store inside the repository struct
  refs: allow for_each_replace_ref to handle arbitrary repositories
  replace-object: allow prepare_replace_object to handle arbitrary
repositories
  replace-object: allow do_lookup_replace_object to handle arbitrary
repositories
  replace-object: allow lookup_replace_object to handle arbitrary
repositories

 builtin/mktag.c   |  3 +-
 builtin/pack-refs.c   |  3 +-
 builtin/replace.c |  4 +-
 cache.h   | 19 
 environment.c |  2 +-
 object-store.h|  8 
 object.c  |  3 +-
 refs.c| 80 
 refs.h|  4 +-
 refs/files-backend.c  |  4 --
 replace-object.h  | 36 +++
 replace_object.c  | 97 ++-
 repository.h  |  3 ++
 revision.c|  5 +-
 sha1_file.c   |  7 +--
 streaming.c   |  3 +-
 t/helper/test-ref-store.c |  3 +-
 17 files changed, 134 insertions(+), 150 deletions(-)
 create mode 100644 replace-object.h


$ git diff github/object-store-3
 diff --git c/Makefile w/Makefile
index 94e0bf47b1..de4b8f0c02 100644
--- c/Makefile
+++ w/Makefile
@@ -871,7 +871,7 @@ LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
-LIB_OBJS += replace-object.o
+LIB_OBJS += replace_object.o
 LIB_OBJS += repository.o
 LIB_OBJS += rerere.o
 LIB_OBJS += resolve-undo.o
diff --git c/refs.c w/refs.c
index bed5f88405..9b56fa9b81 100644
--- c/refs.c
+++ w/refs.c
@@ -1651,11 +1651,11 @@ static struct ref_store *ref_store_init(const char 
*gitdir,
 
 struct ref_store *get_main_ref_store(struct repository *r)
 {
-   if (r->main_ref_store)
-   return r->main_ref_store;
+   if (r->refs)
+   return r->refs;
 
-   r->main_ref_store = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
-   return r->main_ref_store;
+   r->refs = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);
+   return r->refs;
 }
 
 /*
diff --git c/refs/files-backend.c w/refs/files-backend.c
index bec8e30e9e..5c76a75817 100644
--- c/refs/files-backend.c
+++ w/refs/files-backend.c
@@ -61,10 +61,6 @@ struct ref_lock {
struct object_id old_oid;
 };
 
-/*
- * Future: need to be in "struct repository"
- * when doing a full libification.
- */
 struct files_ref_store {
struct ref_store base;
unsigned int store_flags;
diff --git c/replace-object.c w/replace_object.c
similarity index 100%
rename from replace-object.c
rename to replace_object.c
diff --git c/repository.h w/repository.h
index 7d0710b273..e6e00f541b 100644
--- c/repository.h
+++ w/repository.h
@@ -27,7 +27,7 @@ struct repository {
struct raw_object_store *objects;
 
/* The store in which the refs are held. */
-   struct ref_store *main_ref_store;
+   struct 

[PATCH 01/15] replace_object: use oidmap

[Stefan Beller]

From: René Scharfe 

Load the replace objects into an oidmap to allow for easy lookups in
constant time.

Signed-off-by: Rene Scharfe 
Signed-off-by: Stefan Beller 
---
 replace_object.c | 76 ++--
 1 file changed, 16 insertions(+), 60 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index 336357394d..a757a5ebf2 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -1,54 +1,14 @@
 #include "cache.h"
-#include "sha1-lookup.h"
+#include "oidmap.h"
 #include "refs.h"
 #include "commit.h"
 
-/*
- * An array of replacements.  The array is kept sorted by the original
- * sha1.
- */
-static struct replace_object {
-   struct object_id original;
+struct replace_object {
+   struct oidmap_entry original;
struct object_id replacement;
-} **replace_object;
-
-static int replace_object_alloc, replace_object_nr;
+};
 
-static const unsigned char *replace_sha1_access(size_t index, void *table)
-{
-   struct replace_object **replace = table;
-   return replace[index]->original.hash;
-}
-
-static int replace_object_pos(const unsigned char *sha1)
-{
-   return sha1_pos(sha1, replace_object, replace_object_nr,
-   replace_sha1_access);
-}
-
-static int register_replace_object(struct replace_object *replace,
-  int ignore_dups)
-{
-   int pos = replace_object_pos(replace->original.hash);
-
-   if (0 <= pos) {
-   if (ignore_dups)
-   free(replace);
-   else {
-   free(replace_object[pos]);
-   replace_object[pos] = replace;
-   }
-   return 1;
-   }
-   pos = -pos - 1;
-   ALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc);
-   replace_object_nr++;
-   if (pos < replace_object_nr)
-   MOVE_ARRAY(replace_object + pos + 1, replace_object + pos,
-  replace_object_nr - pos - 1);
-   replace_object[pos] = replace;
-   return 0;
-}
+static struct oidmap replace_map = OIDMAP_INIT;
 
 static int register_replace_ref(const char *refname,
const struct object_id *oid,
@@ -59,7 +19,7 @@ static int register_replace_ref(const char *refname,
const char *hash = slash ? slash + 1 : refname;
struct replace_object *repl_obj = xmalloc(sizeof(*repl_obj));
 
-   if (get_oid_hex(hash, _obj->original)) {
+   if (get_oid_hex(hash, _obj->original.oid)) {
free(repl_obj);
warning("bad replace ref name: %s", refname);
return 0;
@@ -69,7 +29,7 @@ static int register_replace_ref(const char *refname,
oidcpy(_obj->replacement, oid);
 
/* Register new object */
-   if (register_replace_object(repl_obj, 1))
+   if (oidmap_put(_map, repl_obj))
die("duplicate replace ref: %s", refname);
 
return 0;
@@ -84,7 +44,7 @@ static void prepare_replace_object(void)
 
for_each_replace_ref(register_replace_ref, NULL);
replace_object_prepared = 1;
-   if (!replace_object_nr)
+   if (!replace_map.map.tablesize)
check_replace_refs = 0;
 }
 
@@ -100,21 +60,17 @@ static void prepare_replace_object(void)
  */
 const struct object_id *do_lookup_replace_object(const struct object_id *oid)
 {
-   int pos, depth = MAXREPLACEDEPTH;
+   int depth = MAXREPLACEDEPTH;
const struct object_id *cur = oid;
 
prepare_replace_object();
 
/* Try to recursively replace the object */
-   do {
-   if (--depth < 0)
-   die("replace depth too high for object %s",
-   oid_to_hex(oid));
-
-   pos = replace_object_pos(cur->hash);
-   if (0 <= pos)
-   cur = _object[pos]->replacement;
-   } while (0 <= pos);
-
-   return cur;
+   while (depth-- > 0) {
+   struct replace_object *repl_obj = oidmap_get(_map, cur);
+   if (!repl_obj)
+   return cur;
+   cur = _obj->replacement;
+   }
+   die("replace depth too high for object %s", oid_to_hex(oid));
 }
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH v6 00/15] rebase -i: offer to recreate commit topology

[Jacob Keller]

On Wed, Apr 11, 2018 at 6:13 AM, Sergey Organov  wrote:
> It was rather --recreate-merges just a few weeks ago, and I've seen
> nobody actually commented either in favor or against the
> --rebase-merges.
>
> git rebase --rebase-merges
>

I'm going to jump in here and say that *I* prefer --rebase-merges, as
it clearly mentions merge commits (which is the thing that changes).

I hadn't mentioned this before, because it was a suggestion that
someone else made and it seemed that Johannes liked it, so I didn't
think further discussion was worthwhile.

Thanks,
Jake

Re: [PATCH v2 1/2] fsexcludes: add a programmatic way to exclude files from git's working directory traversal logic

[Junio C Hamano]

I haven't studied and thought about the motivation behind these two
patches, but one thing I noticed...

Ben Peart  writes:

> diff --git a/dir.c b/dir.c
> index 63a917be45..1aa639b9f4 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1102,6 +1103,12 @@ int is_excluded_from_list(const char *pathname,
> struct exclude_list *el, struct index_state *istate)
>  {
>   struct exclude *exclude;
> +
> + if (*dtype == DT_UNKNOWN)
> + *dtype = get_dtype(NULL, istate, pathname, pathlen);
> + if (fsexcludes_is_excluded_from(istate, pathname, pathlen, *dtype) > 0)
> + return 1;
> +
>   exclude = last_exclude_matching_from_list(pathname, pathlen, basename,
> dtype, el, istate);
>   if (exclude)
> @@ -1317,8 +1324,15 @@ struct exclude *last_exclude_matching(struct 
> dir_struct *dir,
>  int is_excluded(struct dir_struct *dir, struct index_state *istate,
>   const char *pathname, int *dtype_p)
>  {
> - struct exclude *exclude =
> - last_exclude_matching(dir, istate, pathname, dtype_p);
> + struct exclude *exclude;
> + int pathlen = strlen(pathname);
> +
> + if (*dtype_p == DT_UNKNOWN)
> + *dtype_p = get_dtype(NULL, istate, pathname, pathlen);
> + if (fsexcludes_is_excluded_from(istate, pathname, pathlen, *dtype_p) > 
> 0)
> + return 1;
> +
> + exclude = last_exclude_matching(dir, istate, pathname, dtype_p);
>   if (exclude)
>   return exclude->flags & EXC_FLAG_NEGATIVE ? 0 : 1;
>   return 0;

A piece of impression I am getting from the above two hunks is that
the fsexcludes_is_excluded_from() function requires a real dtype in
its last parameter (i.e. DT_UNKNOWN is not acceptable).

> @@ -1671,6 +1685,9 @@ static enum path_treatment treat_one_path(struct 
> dir_struct *dir,
>   if (dtype != DT_DIR && has_path_in_index)
>   return path_none;
>  
> + if (fsexcludes_is_excluded_from(istate, path->buf, path->len, dtype) > 
> 0)
> + return path_excluded;
> +

And this hunk reinforces that impression (we are comparing dtype
with DT_DIR, so we know we cannot be passing DT_UNKNOWN to it).

> @@ -2011,6 +2028,8 @@ static enum path_treatment 
> read_directory_recursive(struct dir_struct *dir,
>   /* add the path to the appropriate result list */
>   switch (state) {
>   case path_excluded:
> + if (fsexcludes_is_excluded_from(istate, path.buf, 
> path.len, DTYPE(cdir.de)) > 0)
> + break;

Then the use of DTYPE() looks a bit odd here.  On
NO_D_TYPE_IN_DIRENT platforms, we would get DT_UNKNOWN out of it and
then end up passing DT_UNKNOWN to the function.

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Stefan Beller]

On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren
 wrote:
> When ran with '--merges-only', git bisect will only look at merge commits -- 
> commits with 2 or more parents or the initial commit.

There has been quite some talk on the mailing list, e.g.
https://public-inbox.org/git/20160427204551.GB4613@virgo.localdomain/
which suggests a --first-parent mode instead. For certain histories
these are the same,
but merges-only is more restrictive for back-and-forth-cross merges.



>
> Signed-off-by: Harald Nordgren 
> ---
>
> Notes:
> Proof of concept of a feature that I have wanted in Git for a while. In 
> my daily work my company uses GitHub, which creates lots of merge commits. In 
> general, tests are only ran on the tip of a branch before merging, so the 
> different commits within a merge commit are allowed not to be buildable. 
> Therefore 'git bisect' often doesn't work since it will give lots of false 
> positives for anything that is not a merge commit. If we could have a feature 
> to only bisect merge commits then it would be easier to pinpoint which merge 
> causes any particular issue. After that, a bisect could be done within the 
> merge to pinpoint futher. As a follow-up to this patch -- we could 
> potentially create a feature that automatically continues into regular bisect 
> within the bad merge commit after completed '--merges-only' bisection.

The github workflow you mention sounds as if --first-parent would do, too?

Re: [PATCH v2 03/10] commit: add generation number to struct commmit

[Junio C Hamano]

Derrick Stolee  writes:

> How about we do a slightly different
> arrangement for these overflow commits?
>
> Instead of storing the commits in the commit-graph file as "0" (which
> currently means "written by a version of git that did not compute
> generation numbers") we could let GENERATION_NUMBER_MAX be the maximum
> generation of a commit in the commit-graph, and if a commit would have
> larger generation, we collapse it down to that value.

Sure.  Any value we can tell that it is special is fine.  Thanks.

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

[Junio C Hamano]

Eric Sunshine  writes:

> On Wed, Apr 11, 2018 at 2:07 PM, Stefan Beller  wrote:
>> On Wed, Apr 11, 2018 at 10:57 AM, Harald Nordgren
>>  wrote:
>>> There have been no new comments for the last few days. I know Jeff
>>> King will be away for the next two weeks, but should we still move
>>> forward with this? The initial reactions to the idea seemed positive.
>> ...
>> It will be merged to next and if no people speak up (due to bugs
>> observed or such)
>
> I would guess, however, that Harald is wondering more specifically
> about the "--sort" patch he added atop Peff's patches. Disposition for
> _that_ patch is not specified in the latest "What's cooking":
>
> * hn/sort-ls-remote (2018-04-09) 1 commit
>  - ls-remote: create '--sort' option
>  (this branch uses jk/ref-array-push.)
>
>  "git ls-remote" learned an option to allow sorting its output based
>  on the refnames being shown.

Thanks all for summarizing.

I think the preliminary clean-up patches by Peff are all
independently good and can be advanced, even without him being
around.  I haven't formed a firm opinion on the doneness of the
latest round of the "--sort" thing.  I usually do not explicitly
write "I haven't formed an opinion" in "What's cooking" report,
so please read the lack of "disposition" as such.

Re: [PATCH v3] git-svn: allow empty email-address using authors-prog and authors-file

[Junio C Hamano]

Eric Wong  writes:

> Andreas Heiduk  wrote:
>> Am 05.04.2018 um 09:51 schrieb Eric Wong:
>> > Can you confirm it's OK for you?  Thanks.
>> 
>> Looks good, works for me.
>> 
>> Do you squash this patch with with my commit or do you need a reroll?
>
> Nope, no need to reroll.  Pushed to my repo for Junio.  Thanks all.
>
> The following changes since commit 468165c1d8a442994a825f3684528361727cd8c0:
>
>   Git 2.17 (2018-04-02 10:13:35 -0700)
>
> are available in the Git repository at:
>
>   git://bogomips.org/git-svn.git svn/authors-prog-2
>
> for you to fetch changes up to cb427e9eb0243fe7a1a22ea3bd0a46b7410c0bf3:
>
>   git-svn: allow empty email-address using authors-prog and authors-file 
> (2018-04-05 19:22:06 +)

Sorry; this message fell under my radar and I had to privately get
reminded of it.  Pulled.

Thanks, both.

Re: [PATCH 0/6] Rename files to use dashes instead of underscores

[brian m. carlson]

On Tue, Apr 10, 2018 at 02:26:15PM -0700, Stefan Beller wrote:
> This is the followup for 
> https://public-inbox.org/git/xmqqbmer4vfh@gitster-ct.c.googlers.com/
> 
> We have no files left with underscores in their names.

This series looked good to me.  It's a nice change.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH] Create '--merges-only' option for 'git bisect'

[Harald Nordgren]

When ran with '--merges-only', git bisect will only look at merge commits -- 
commits with 2 or more parents or the initial commit.

Signed-off-by: Harald Nordgren 
---

Notes:
Proof of concept of a feature that I have wanted in Git for a while. In my 
daily work my company uses GitHub, which creates lots of merge commits. In 
general, tests are only ran on the tip of a branch before merging, so the 
different commits within a merge commit are allowed not to be buildable. 
Therefore 'git bisect' often doesn't work since it will give lots of false 
positives for anything that is not a merge commit. If we could have a feature 
to only bisect merge commits then it would be easier to pinpoint which merge 
causes any particular issue. After that, a bisect could be done within the 
merge to pinpoint futher. As a follow-up to this patch -- we could potentially 
create a feature that automatically continues into regular bisect within the 
bad merge commit after completed '--merges-only' bisection.

 bisect.c|  87 ++
 bisect.h|   4 +-
 builtin/bisect--helper.c|  28 +-
 builtin/rev-list.c  |   2 +-
 git-bisect.sh   |   5 ++
 t/t6070-bisect-merge-commits.sh | 116 
 6 files changed, 228 insertions(+), 14 deletions(-)
 create mode 100755 t/t6070-bisect-merge-commits.sh

diff --git a/bisect.c b/bisect.c
index a579b5088..e8a2f77c7 100644
--- a/bisect.c
+++ b/bisect.c
@@ -254,7 +254,7 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list, int n
  */
 static struct commit_list *do_find_bisection(struct commit_list *list,
 int nr, int *weights,
-int find_all)
+int find_all, int 
only_merge_commits)
 {
int n, counted;
struct commit_list *p;
@@ -266,6 +266,13 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
unsigned flags = commit->object.flags;
 
p->item->util = [n++];
+
+   if (only_merge_commits) {
+   weight_set(p, -2);
+   counted++;
+   continue;
+   }
+
switch (count_interesting_parents(commit)) {
case 0:
if (!(flags & TREESAME)) {
@@ -305,11 +312,17 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
 * way, and then fill the blanks using cheaper algorithm.
 */
for (p = list; p; p = p->next) {
+   int distance;
if (p->item->object.flags & UNINTERESTING)
continue;
if (weight(p) != -2)
continue;
-   weight_set(p, count_distance(p));
+
+   if (only_merge_commits)
+   distance = count_distance(p) - 1;
+   else
+   distance = count_distance(p);
+   weight_set(p, distance);
clear_distance(list);
 
/* Does it happen to be at exactly half-way? */
@@ -330,11 +343,17 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
for (q = p->item->parents; q; q = q->next) {
if (q->item->object.flags & UNINTERESTING)
continue;
+   if (!q->item->util)
+   break;
if (0 <= weight(q))
break;
}
if (!q)
continue;
+   if (!q->item->util) {
+   counted++;
+   continue;
+   }
 
/*
 * weight for p is unknown but q is known.
@@ -364,8 +383,43 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
return best_bisection_sorted(list, nr);
 }
 
+int merge_commit_or_root(const struct commit c)
+{
+   if (!c.parents)
+   return 1;
+
+   return !!c.parents->next;
+}
+
+void filter_non_merge_commits(struct commit_list **commit_list)
+{
+   struct commit_list *list1 = *commit_list;
+   struct commit_list *list2 = NULL;
+   *commit_list = NULL;
+
+   for ( ; list1; list1 = list1->next) {
+   if (merge_commit_or_root(*list1->item)) {
+   list2 = list1;
+   list1 = list1->next;
+   list2->next = NULL;
+   list2->item->parents = NULL;
+   *commit_list = list2;
+   break;
+   }
+ 

Re: [PATCH/RFC 0/5] Keep all info in command-list.txt in git binary

[Philip Oakley]

From: "Eric Sunshine"  Monday, April 09, 2018 6:17 
AM

On Mon, Mar 26, 2018 at 12:55 PM, Nguyễn Thái Ngọc Duy
 wrote:

This is pretty rough but I'd like to see how people feel about this
first.

I notice we have two places for command classification. One in
command-list.txt, one in __git_list_porcelain_commands() in
git-completion.bash. People who are following nd/parseopt-completion
probably know that I'm try to reduce duplication in this script as
much as possible, this is another step towards that.

By keeping all information of command-list.txt in git binary, we could
provide the porcelain list to git-completion.bash via "git
--list-cmds=porcelain", so we don't neeed a separate command
classification in git-completion.bash anymore.


I like the direction this series is taking.


Because we have all command synopsis as a side effect, we could
now support "git help -a --verbose" which prints something like "git
help", a command name and a description, but we could do it for _all_
recognized commands. This could help people look for a command even if
we don't provide "git appropos".


Nice idea, and you practically get this for free (aside from the the
obvious new code) since generate-cmdlist.sh already plucks the summary
for each command directly from Documentation/git-*.txt.

I'm only just catching up, but does/can this series also capture the 
non-command guides that are available in git so that the 'git help -g' can 
begin to list them all?


It was something I looked at some years ago (when I added the -g option) but 
at the time the idea of updating the command-list.txt was too invasive.


Just a thought.

Philip

[PATCH v4 1/3] fast-import: rename mem_pool type to mp_block

[Jameson Miller]

This is part of a patch series to extract the memory pool logic in
fast-import into a more generalized version. The existing mem_pool type
maps more closely to a "block of memory" (mp_block) in the more
generalized memory pool. This commit renames the mem_pool to mp_block to
reduce churn in future patches.

Signed-off-by: Jameson Miller 
---
 fast-import.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 99f8f56e8c..38af0a294b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -211,8 +211,8 @@ struct last_object {
unsigned no_swap : 1;
 };
 
-struct mem_pool {
-   struct mem_pool *next_pool;
+struct mp_block {
+   struct mp_block *next_block;
char *next_free;
char *end;
uintmax_t space[FLEX_ARRAY]; /* more */
@@ -306,9 +306,9 @@ static int global_argc;
 static const char **global_argv;
 
 /* Memory pools */
-static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mem_pool);
+static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mp_block);
 static size_t total_allocd;
-static struct mem_pool *mem_pool;
+static struct mp_block *mp_block_head;
 
 /* Atom management */
 static unsigned int atom_table_sz = 4451;
@@ -638,14 +638,14 @@ static unsigned int hc_str(const char *s, size_t len)
 
 static void *pool_alloc(size_t len)
 {
-   struct mem_pool *p;
+   struct mp_block *p;
void *r;
 
/* round up to a 'uintmax_t' alignment */
if (len & (sizeof(uintmax_t) - 1))
len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
 
-   for (p = mem_pool; p; p = p->next_pool)
+   for (p = mp_block_head; p; p = p->next_block)
if ((p->end - p->next_free >= len))
break;
 
@@ -654,12 +654,12 @@ static void *pool_alloc(size_t len)
total_allocd += len;
return xmalloc(len);
}
-   total_allocd += sizeof(struct mem_pool) + mem_pool_alloc;
-   p = xmalloc(st_add(sizeof(struct mem_pool), mem_pool_alloc));
-   p->next_pool = mem_pool;
+   total_allocd += sizeof(struct mp_block) + mem_pool_alloc;
+   p = xmalloc(st_add(sizeof(struct mp_block), mem_pool_alloc));
+   p->next_block = mp_block_head;
p->next_free = (char *) p->space;
p->end = p->next_free + mem_pool_alloc;
-   mem_pool = p;
+   mp_block_head = p;
}
 
r = p->next_free;
-- 
2.14.3

fixup! [PATCH 2/6] doc: align 'diff --no-index' in text with synopsis

[Andreas Heiduk]

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-diff.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index ee1c509bd3..6593b58299 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git diff' [options] --cached [] [--] [...]
 'git diff' [options]   [--] [...]
 'git diff' [options]  
-'git diff' [options] [--no-index] [--]  
+'git diff' [options] --no-index [--]  
 
 DESCRIPTION
 ---
@@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
further add to the index but you still haven't.  You can
stage these changes by using linkgit:git-add[1].
 
-'git diff' [--options] [--no-index] [--]  ::
+'git diff' [--options] --no-index [--]  ::
 
This form is to compare the given two paths on the
filesystem.  You can omit the `--no-index` option when
-- 
2.16.2

Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Wed, Apr 11, 2018 at 4:50 PM, Thomas Gummerer  wrote:
> And just as I'm re-reading my commit messages, I guess there was one
> more motivation for printing the "HEAD is now at ..." line ourselves:
>
> If the '--no-checkout' flag is given, the output of 'git worktree add'
> is just:
>
> Preparing foo (identifier foo)
>
> even though the HEAD is set to a commit, which is just not checked out.
>
> I think I can live with that for now, I personally don't use
> '--no-checkout', and we could fix this at some point in the future if
> we desire to do so.  I think we can consider that out of scope for
> this patch series, as its main goal is to introduce the new dwim.

Sounds reasonable, especially since the proposed "Preparing worktree
(_branch description_)" provides similarly useful feedback.

fixup! [PATCH 1/6] doc: fix formatting inconsistency in githooks.txt

[Andreas Heiduk]

- reflow some paragraphs
---
 Documentation/githooks.txt | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index be31376767..ab5ce80e13 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -194,10 +194,10 @@ for an example of how to do this.
 pre-push
 
 
-This hook is called by linkgit:git-push[1] and can be used to prevent a push 
from taking
-place.  The hook is called with two parameters which provide the name and
-location of the destination remote, if a named remote is not being used both
-values will be the same.
+This hook is called by linkgit:git-push[1] and can be used to prevent
+a push from taking place.  The hook is called with two parameters
+which provide the name and location of the destination remote, if a
+named remote is not being used both values will be the same.
 
 Information about what is to be pushed is provided on the hook's standard
 input with lines of the form:
@@ -410,9 +410,9 @@ with the difference between the branches.
 pre-auto-gc
 ~~~
 
-This hook is invoked by `git gc --auto` (see linkgit:git-gc[1]). It takes no 
parameter, and
-exiting with non-zero status from this script causes the `git gc --auto`
-to abort.
+This hook is invoked by `git gc --auto` (see linkgit:git-gc[1]). It
+takes no parameter, and exiting with non-zero status from this script
+causes the `git gc --auto` to abort.
 
 post-rewrite
 
-- 
2.16.2

fixup! [PATCH 1/6] doc: fix formatting inconsistency in githooks.txt

[Andreas Heiduk]

- add linkgit: to callers of hooks
- change 'git-foo' and similar to `git foo`
- add some more `` for fsmonitor
---
 Documentation/githooks.txt | 101 +++--
 1 file changed, 51 insertions(+), 50 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 070e745b41..be31376767 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -31,7 +31,7 @@ Hooks can get their arguments via the environment, 
command-line
 arguments, and stdin. See the documentation for each hook below for
 details.
 
-'git init' may copy hooks to the new repository, depending on its
+`git init` may copy hooks to the new repository, depending on its
 configuration. See the "TEMPLATE DIRECTORY" section in
 linkgit:git-init[1] for details. When the rest of this document refers
 to "default hooks" it's talking about the default template shipped
@@ -45,9 +45,9 @@ HOOKS
 applypatch-msg
 ~~
 
-This hook is invoked by 'git am'.  It takes a single
+This hook is invoked by linkgit:git-am[1].  It takes a single
 parameter, the name of the file that holds the proposed commit
-log message.  Exiting with a non-zero status causes 'git am' to abort
+log message.  Exiting with a non-zero status causes `git am` to abort
 before applying the patch.
 
 The hook is allowed to edit the message file in place, and can
@@ -61,7 +61,7 @@ The default 'applypatch-msg' hook, when enabled, runs the
 pre-applypatch
 ~~
 
-This hook is invoked by 'git am'.  It takes no parameter, and is
+This hook is invoked by linkgit:git-am[1].  It takes no parameter, and is
 invoked after the patch is applied, but before a commit is made.
 
 If it exits with non-zero status, then the working tree will not be
@@ -76,7 +76,7 @@ The default 'pre-applypatch' hook, when enabled, runs the
 post-applypatch
 ~~~
 
-This hook is invoked by 'git am'.  It takes no parameter,
+This hook is invoked by linkgit:git-am[1].  It takes no parameter,
 and is invoked after the patch is applied and a commit is made.
 
 This hook is meant primarily for notification, and cannot affect
@@ -85,24 +85,24 @@ the outcome of 'git am'.
 pre-commit
 ~~
 
-This hook is invoked by 'git commit', and can be bypassed
+This hook is invoked by linkgit:git-commit[1], and can be bypassed
 with the `--no-verify` option.  It takes no parameters, and is
 invoked before obtaining the proposed commit log message and
 making a commit.  Exiting with a non-zero status from this script
-causes the 'git commit' command to abort before creating a commit.
+causes the `git commit` command to abort before creating a commit.
 
 The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
 such a line is found.
 
-All the 'git commit' hooks are invoked with the environment
+All the `git commit` hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
 to modify the commit message.
 
 prepare-commit-msg
 ~~
 
-This hook is invoked by 'git commit' right after preparing the
+This hook is invoked by linkgit:git-commit[1] right after preparing the
 default log message, and before the editor is started.
 
 It takes one to three parameters.  The first is the name of the file
@@ -114,7 +114,7 @@ commit is a merge or a `.git/MERGE_MSG` file exists); 
`squash`
 (if a `.git/SQUASH_MSG` file exists); or `commit`, followed by
 a commit SHA-1 (if a `-c`, `-C` or `--amend` option was given).
 
-If the exit status is non-zero, 'git commit' will abort.
+If the exit status is non-zero, `git commit` will abort.
 
 The purpose of the hook is to edit the message file in place, and
 it is not suppressed by the `--no-verify` option.  A non-zero exit
@@ -127,7 +127,7 @@ help message found in the commented portion of the commit 
template.
 commit-msg
 ~~
 
-This hook is invoked by 'git commit' and 'git merge', and can be
+This hook is invoked by linkgit:git-commit[1] and linkgit:git-merge[1], and 
can be
 bypassed with the `--no-verify` option.  It takes a single parameter,
 the name of the file that holds the proposed commit log message.
 Exiting with a non-zero status causes the command to abort.
@@ -143,16 +143,16 @@ The default 'commit-msg' hook, when enabled, detects 
duplicate
 post-commit
 ~~~
 
-This hook is invoked by 'git commit'. It takes no parameters, and is
+This hook is invoked by linkgit:git-commit[1]. It takes no parameters, and is
 invoked after a commit is made.
 
 This hook is meant primarily for notification, and cannot affect
-the outcome of 'git commit'.
+the outcome of `git commit`.
 
 pre-rebase
 ~~
 
-This hook is called by 'git rebase' and can be used to prevent a
+This hook is called by linkgit:git-rebase[1] and can be used to prevent a
 branch from getting rebased.  The hook may be called with one or
 two parameters.  The first parameter is the 

Re: [PATCH 1/6] doc: fix formatting inconsistency in githooks.txt

[Andreas Heiduk]

So the following two fixups should cleanup that page considerably.

Re: [PATCH v8 03/14] commit-graph: add format document

[Jakub Narebski]

Derrick Stolee  writes:

> +CHUNK DATA:
> +
> +  OID Fanout (ID: {'O', 'I', 'D', 'F'}) (256 * 4 bytes)
> +  The ith entry, F[i], stores the number of OIDs with first
> +  byte at most i. Thus F[255] stores the total
> +  number of commits (N).
> +
> +  OID Lookup (ID: {'O', 'I', 'D', 'L'}) (N * H bytes)
> +  The OIDs for all commits in the graph, sorted in ascending order.
> +
> +  Commit Data (ID: {'C', 'G', 'E', 'T' }) (N * (H + 16) bytes)

I think it is a typo, and it should be CDAT, not CGET
(CDAT seem to me to stand for Commit DATa):

  +  Commit Data (ID: {'C', 'D', 'A', 'T' }) (N * (H + 16) bytes)

This is what you use in actual implementation, in PATCH v8 06/14

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

-- 
Jakub Narębski

Re: What's cooking in git.git (Apr 2018, #01; Mon, 9)

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 11 2018, Jakub Narebski wrote:

> Junio C Hamano  writes:
>
>
>> * ab/nuke-emacs-contrib (2018-03-13) 1 commit
>>   (merged to 'next' on 2018-03-15 at 13eb4e2d8b)
>>  + git{,-blame}.el: remove old bitrotting Emacs code
>>
>>  The scripts in contrib/emacs/ have outlived their usefulness and
>>  have been removed.
>>
>>  Will kick back to 'pu'.
>>
>>  There were some noises about better migration strategy that lets
>>  git.el to nudge users to magit or something when used.  Is it
>>  something we want to pursue further?
>
> Maybe simply delete all files, leaving only README pointing to Magit?

I've just submitted a v4 which should address the issue raised in v3,
which was that we don't want to delete the files, but instead have a
boilerplate *.el file that errors out, thus helping e.g. users of git-el
in Debian to migrate.

See
https://public-inbox.org/git/20180411204206.28498-1-ava...@gmail.com/

Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

On 04/11, Thomas Gummerer wrote:
> On 04/09, Eric Sunshine wrote:
> > On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer  
> > wrote:
> > > On 04/08, Eric Sunshine wrote:
> > >> As with Junio, I'm fine with this hidden option (for now), however, I
> > >> think you can take this a step further. Rather than having a (hidden)
> > >> git-reset option which suppresses "HEAD is now at...", instead have a
> > >> (hidden) option which augments the message. For example,
> > >> --new-head-desc="New worktree" would make it output "New worktree HEAD
> > >> is now at...". Changes to builtin/reset.c to support this would hardly
> > >> be larger than the changes you already made.
> > >
> > > Something else I just noticed that may make this a worse solution is
> > > that this breaks the sentence in two pieces for translators.  I guess
> > > we could somehow get the "New worktree" part of the option translated,
> > > but that still means that if some language would require to move parts
> > > of the sentence around that would be less than ideal for translation.
> > 
> > Good point.
> > 
> > One solution would be to have the new hidden option replace the string
> > entirely: --new-head-msg="New worktree HEAD is now at %s", which would
> > allow translators to deal with the entire sentence. Even clearer would
> > be to drop "now", as in "New worktree HEAD is at %s". (Default in
> > reset.c would still be "HEAD is now at %s", of course.)
> > 
> > Another solution would be not to augment the "HEAD is now at..."
> > message at all. I realize that that augmentation was one of the
> > original motivations for this patch series, but with the upcoming
> > restoration of the "Preparing worktree" message:
> 
> My original motivation of the series was to just make the new dwim
> work :)  Because that's adding some magic, the secondary motivation
> became improving the UI, to help users see which dwim was used.  I
> felt like this was going to be one of those improvements, especially
> after we get rid of the "Preparing ..." line.
> 
> I do however like your suggestion of the "Preparing worktree (_branch
> disposition_)", as that doesn't add more lines to the output, while
> still giving a good indication of what exactly is happening.  At that
> point just showing "HEAD is now at ..." is fine by me, and doesn't
> require adding the hidden flag to 'git reset'.  So I'm happy just
> dropping the change in the message here, which will simplify things.

And just as I'm re-reading my commit messages, I guess there was one
more motivation for printing the "HEAD is now at ..." line ourselves:

If the '--no-checkout' flag is given, the output of 'git worktree add'
is just:

Preparing foo (identifier foo)

even though the HEAD is set to a commit, which is just not checked out.

I think I can live with that for now, I personally don't use
'--no-checkout', and we could fix this at some point in the future if
we desire to do so.  I think we can consider that out of scope for
this patch series, as its main goal is to introduce the new dwim.

> Thanks for the suggestion!

Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Wed, Apr 11, 2018 at 4:09 PM, Thomas Gummerer  wrote:
> On 04/09, Eric Sunshine wrote:
>> Another solution would be not to augment the "HEAD is now at..."
>> message at all. I realize that that augmentation was one of the
>> original motivations for this patch series, but with the upcoming
>> restoration of the "Preparing worktree" message:
>
> My original motivation of the series was to just make the new dwim
> work :)  Because that's adding some magic, the secondary motivation
> became improving the UI, to help users see which dwim was used.  I
> felt like this was going to be one of those improvements, especially
> after we get rid of the "Preparing ..." line.
>
> I do however like your suggestion of the "Preparing worktree (_branch
> disposition_)", as that doesn't add more lines to the output, while
> still giving a good indication of what exactly is happening.  At that
> point just showing "HEAD is now at ..." is fine by me, and doesn't
> require adding the hidden flag to 'git reset'.  So I'm happy just
> dropping the change in the message here, which will simplify things.

Nice. This sounds like a good plan for moving forward.

Re: [PATCH resend] SubmittingPatches: mention the git contacts command

[Derrick Stolee]

On 4/11/2018 4:20 PM, Thomas Gummerer wrote:

Instead of just mentioning 'git blame' and 'git shortlog', which make it
quite hard for new contributors to pick out the appropriate list of
people to cc on their patch series, mention the 'git contacts' utility,
which makes it much easier to get a reasonable list of contacts for a
change.

This should help new contributors pick out a reasonable cc list by
simply using a single command.

Signed-off-by: Thomas Gummerer 
---

I've originally sent this at <20180316213323.GC2224@hank>, during an
the rc period.  Eric had some comments, which I interpreted as being
okay with the change (hope I'm not mistaken there :)).  As I still
think this would be an improvement for new contributors, I'm resending
it here.


I didn't know about this tool, and it seems helpful. I plan to use it 
now. Thanks!



  Documentation/SubmittingPatches | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index a1d0feca36..945f8edb46 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -260,8 +260,8 @@ that starts with `-BEGIN PGP SIGNED MESSAGE-`.  
That is
  not a text/plain, it's something else.
  
  Send your patch with "To:" set to the mailing list, with "cc:" listing

-people who are involved in the area you are touching (the output from
-`git blame $path` and `git shortlog --no-merges $path` would help to
+people who are involved in the area you are touching (the `git
+contacts` command in `contrib/contacts/` can help to
  identify them), to solicit comments and reviews.
  
  :1: footnote:[The current maintainer: gits...@pobox.com]

Re: [PATCH resend] SubmittingPatches: mention the git contacts command

[Eric Sunshine]

On Wed, Apr 11, 2018 at 4:20 PM, Thomas Gummerer  wrote:
> Instead of just mentioning 'git blame' and 'git shortlog', which make it
> quite hard for new contributors to pick out the appropriate list of
> people to cc on their patch series, mention the 'git contacts' utility,
> which makes it much easier to get a reasonable list of contacts for a
> change.
>
> This should help new contributors pick out a reasonable cc list by
> simply using a single command.
>
> Signed-off-by: Thomas Gummerer 
> ---
>
> I've originally sent this at <20180316213323.GC2224@hank>, during an
> the rc period.  Eric had some comments, which I interpreted as being
> okay with the change (hope I'm not mistaken there :)).  As I still
> think this would be an improvement for new contributors, I'm resending
> it here.

I don't think you misinterpreted my response. Your answers to my
review questions seemed satisfactory, so I wasn't expecting any
changes.

[PATCH v4] git{,-blame}.el: remove old bitrotting Emacs code

[Ævar Arnfjörð Bjarmason]

The git-blame.el mode has been superseded by Emacs's own
vc-annotate (invoked by C-x v g). Users of the git.el mode are now
much better off using either Magit or the Git backend for Emacs's own
VC mode.

These modes were added over 10 years ago when Emacs's own Git support
was much less mature, and there weren't other mature modes in the wild
or shipped with Emacs itself.

These days these modes have few if any users, and users of git aren't
well served by us shipping these (some OS's install them alongside git
by default, which is confusing and leads users astray).

So let's remove these per Alexandre Julliard's message to the
ML[1]. If someone still wants these for some reason they're better
served by hosting these elsewhere (e.g. on ELPA), instead of us
distributing them with git.

However, since downstream packagers such as Debian are packaging this
as git-el it's less disruptive to still carry these files as Elisp
code that'll error out with a message suggesting alternatives, rather
than drop the files entirely[2].

Then rather than receive a cryptic load error when they upgrade
existing users will get an error directing them to the README file, or
to just stop requiring these modes. I think it makes sense to link to
GitHub's hosting of contrib/emacs/README (which'll be updated by the
time users see this) so they don't have to hunt down the packaged
README on their local system.

1. "Re: [PATCH] git.el: handle default excludesfile
   properly" (87muzlwhb0@winehq.org) --
   https://public-inbox.org/git/87muzlwhb0@winehq.org/

2. "Re: [PATCH v3] git{,-blame}.el: remove old bitrotting Emacs
   code" (20180327165751.ga4...@aiede.svl.corp.google.com) --
   https://public-inbox.org/git/20180327165751.ga4...@aiede.svl.corp.google.com/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Tue, Mar 27 2018, Jonathan Nieder wrote:

> The trouble with removing these so abruptly is that it makes for a bad
> user experience.
> [...]

Indeed. Sorry for taking so long to act on this. Haven't had much time
for git recently. Now we still ship these Elisp files, but they just
error out with a message as suggested. Junio: This should address your
note about this series in the last What's Cooking.

tbdiff with v3:

1: 5bc3d3848d ! 1: 7556844084 git{,-blame}.el: remove old bitrotting Emacs 
code
@@ -20,9 +20,25 @@
 served by hosting these elsewhere (e.g. on ELPA), instead of us
 distributing them with git.
 
+However, since downstream packagers such as Debian are packaging 
this
+as git-el it's less disruptive to still carry these files as Elisp
+code that'll error out with a message suggesting alternatives, 
rather
+than drop the files entirely[2].
+
+Then rather than receive a cryptic load error when they upgrade
+existing users will get an error directing them to the README 
file, or
+to just stop requiring these modes. I think it makes sense to link 
to
+GitHub's hosting of contrib/emacs/README (which'll be updated by 
the
+time users see this) so they don't have to hunt down the packaged
+README on their local system.
+
 1. "Re: [PATCH] git.el: handle default excludesfile
properly" (87muzlwhb0@winehq.org) --
https://public-inbox.org/git/87muzlwhb0@winehq.org/
+
+2. "Re: [PATCH v3] git{,-blame}.el: remove old bitrotting Emacs
+   code" (20180327165751.ga4...@aiede.svl.corp.google.com) --
+   
https://public-inbox.org/git/20180327165751.ga4...@aiede.svl.corp.google.com/
 
 Signed-off-by: Ævar Arnfjörð Bjarmason 
 
@@ -109,9 +125,8 @@
  
 
 diff --git a/contrib/emacs/git-blame.el b/contrib/emacs/git-blame.el
-deleted file mode 100644
 --- a/contrib/emacs/git-blame.el
-+++ /dev/null
 b/contrib/emacs/git-blame.el
 @@
 -;;; git-blame.el --- Minor mode for incremental blame for Git  -*- 
coding: utf-8 -*-
 -;;
@@ -596,11 +611,16 @@
 -(provide 'git-blame)
 -
 -;;; git-blame.el ends here
++(error "git-blame.el no longer ships with git. It's recommended
++to replace its use with Emacs's own vc-annotate. See
++contrib/emacs/README in git's
++sources (https://github.com/git/git/blob/master/contrib/emacs/README)
++for more info on suggested alternatives and for why this
++happened.")
 
 diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
-deleted file mode 100644
 --- a/contrib/emacs/git.el
-+++ /dev/null
 b/contrib/emacs/git.el
 @@
 -;;; git.el --- A user interface for git
 -
@@ -2306,3 +2326,9 @@
 -

Re: [PATCH] specify encoding for sed command

[Matt Coleman]

I found another (possibly better) way to fix this:

> On Apr 10, 2018, at 3:18 AM, Matt Coleman  wrote:
> 
>> 1) What platform OS / version / sed version is this on?
> I'm experiencing this on macOS Sierra (10.12.6). The issue occurs with the 
> OS's native sed, which is FreeBSD sed so the version number is kind of 
> ambiguous.
> 
> The error goes away if I set LANG=C or LC_ALL=C or change it to use GNU sed 
> (installed via homebrew as gsed).

If I change it to use awk instead of sed, it works with mawk, gawk, and macOS 
awk:
unset $(set | awk -F '=' '/^__gitcomp_builtin_[a-zA-Z0-9_][a-zA-Z0-9_]*=/ 
{print $1}') 2>/dev/null

I compared sed vs. awk on Linux and Mac and they all take about the same amount 
of time to run (within 0.002ms).

Re: What's cooking in git.git (Apr 2018, #01; Mon, 9)

[Jakub Narebski]

Junio C Hamano  writes:


> * ab/nuke-emacs-contrib (2018-03-13) 1 commit
>   (merged to 'next' on 2018-03-15 at 13eb4e2d8b)
>  + git{,-blame}.el: remove old bitrotting Emacs code
>
>  The scripts in contrib/emacs/ have outlived their usefulness and
>  have been removed.
>
>  Will kick back to 'pu'.
>
>  There were some noises about better migration strategy that lets
>  git.el to nudge users to magit or something when used.  Is it
>  something we want to pursue further?

Maybe simply delete all files, leaving only README pointing to Magit?

Best,
--
Jakub Narębski

Re: [PATCH v2 0/4] Lazy-load trees when reading commit-graph

[Jakub Narebski]

Derrick Stolee  writes:

> On 4/7/2018 2:40 PM, Jakub Narebski wrote:
>> Derrick Stolee  writes:
>>
>> [...]
>>> On the Linux repository, performance tests were run for the following
>>> command:
>>>
>>>  git log --graph --oneline -1000
>>>
>>>  Before: 0.92s
>>>  After:  0.66s
>>>  Rel %: -28.3%
>>>
>>> Adding '-- kernel/' to the command requires loading the root tree
>>> for every commit that is walked. There was no measureable performance
>>> change as a result of this patch.
>>
>> In the "Git Merge contributor summit notes" [1] one can read that:
>>
>>> - VSTS adds bloom filters to know which paths have changed on the commit
>>> - tree-same check in the bloom filter is fast; speeds up file history checks
>>> - if the file history is _very_ sparse, then bloom filter is useful
>>
>> Could this method speed up also the second case mentioned here?  Can
>> anyone explain how this "path-changed bloom filter" works in VSTS?
>>
>
> The idea is simple: for every commit, store a Bloom filter containing
> the list of paths that are not TREESAME against the first parent. (A
> slight detail: have a max cap on the number of paths, and store simply
> "TOO_BIG" for commits with too many diffs.)

Isn't Bloom filter with all bits set to 1 equivalent of "too big"? It
would answer each query with "possibly in set, check it".

> When performing 'git log -- path' queries, the most important detail
> for considering how to advance the walk is whether the commit is
> TREESAME to its first parent. For a deep path in a large repo, this is
> almost always true. When a Bloom filter says "TREESAME" (i.e. "this
> path is not in my set") it is always correct, so we can set the
> treesame bit and continue without walking any trees. When a Bloom
> filter says "MAYBE NOT TREESAME" (i.e. "this path is probably in my
> set") you only need to do the same work as before: walk the trees to
> compare against your first parent.
>
> If a Bloom filter has a false-positive rate of X%, then you can
> possibly drop your number of tree comparisons by (100-X)%. This is
> very important for large repos where some paths were changed only ten
> times or so, the full graph needs to be walked and it is helpful to
> avoid parsing too many trees.

So this works only for exact file pathnames, or does checking for
subdirectory also work?  I guess it cannot work for globbing patterns
(like "git log -- '*.c'").

I guess that it speeds up "git log -- ", but also "git blame
".


Sidenote: I have stumbled upon https://github.com/efficient/ffbf project
(fast-forward Bllom filters - for exact matching of large number of
patterns), where they invent cache-efficient version of Bloom filter
algorithm.  The paper that the project is based on also might be
interesting, if only because it points to other improvements of classic
Bloom filters.

>> Could we add something like this to the commit-graph file? 
>
> I'm not sure if it is necessary for client-side operations, but it is
> one of the reasons the commit-graph file has the idea of an "optional
> chunk". It could be added to the file format (without changing version
> numbers) and be ignored by clients that don't understand it. I could
> also be gated by a config setting for computing them. My guess is that
> only server-side operations will need the added response time, and can
> bear the cost of computing them when writing the commit-graph
> file. Clients are less likely to be patient waiting for a lot of diff
> calculations.

So the problem is performing large amount of diff calculations.  I
wonder if it would be possible to add Bloom filters incrementally, when
for some reason we need to calculate diff anyway.

>
> If we add commit-graph file downloads to the protocol, then the server
> could do this computation and send the data to all clients. But that
> would be "secondary" information that maybe clients want to verify,
> which is as difficult as computing it themselves.

Can you tell us how much work (how much time) must spend the server to
calculate those per-commit "files changed" Bloom fillters?

Best,
--
Jakub Narębski

Re: [PATCH v6 00/15] rebase -i: offer to recreate commit topology

[Johannes Schindelin]

Hi Sergey,

On Wed, 11 Apr 2018, Sergey Organov wrote:

> The RFC v2 and Phillip's strategy are essentially the same, as has been
> already shown multiple times, both theoretically and by testing.

No, they are not.

I am really tired of repeating myself here, as I have demonstrated it at
length, at least half a dozen times, that they are *not* in practice the
same.

If you had played through the example as I suggested, you would actually
see where the differences are, and that your proposal is simply
impractical.

And you would see that Phillip's strategy is better, but I get the
impression that you want to avoid that insight at all cost.

Ciao,
Johannes

Re: [PATCH v2 04/10] commit-graph: compute generation numbers

[Stefan Beller]

On Wed, Apr 11, 2018 at 6:02 AM, Derrick Stolee  wrote:
> On 4/10/2018 10:51 PM, Junio C Hamano wrote:
>>
>> Derrick Stolee  writes:
>>
>>> +   if ((*list)->generation != GENERATION_NUMBER_INFINITY) {
>>> +   if ((*list)->generation > GENERATION_NUMBER_MAX)
>>> +   die("generation number %u is too large to
>>> store in commit-graph",
>>> +   (*list)->generation);
>>> +   packedDate[0] |= htonl((*list)->generation << 2);
>>> +   }
>>
>>
>> How serious do we want this feature to be?  On one extreme, we could
>> be irresponsible and say it will be a problem for our descendants in
>> the future if their repositories have more than billion pearls on a
>> single strand, and the above certainly is a reasonable way to punt.
>> Those who actually encounter the problem will notice by Git dying
>> somewhere rather deep in the callchain.
>>
>> Or we could say Git actually does support a history that is
>> arbitrarily long, even though such a deep portion of history will
>> not benefit from having generation numbers in commit-graph.
>>
>> I've been assuming that our stance is the latter and that is why I
>> made noises about overflowing 30-bit generation field in my review
>> of the previous step.
>>
>> In case we want to do the "we know this is very large, but we do not
>> know the exact value", we may actually want a mode where we can
>> pretend that GENERATION_NUMBER_MAX is set to quite low (say 256) and
>> make sure that the code to handle overflow behaves sensibly.
>
>
> I agree. I wonder how we can effectively expose this value into a test. It's
> probably not sufficient to manually test using compiler flags ("-D
> GENERATION_NUMBER_MAX=8").

Would using an environment variable for this testing purpose be a good idea?

If we allow a user to pass in an arbitrary maximum, then we'd have to care about
generation numbers that are stored in the commit graph file larger than that
user specific maximum, though.

Looking through the output of "git grep getenv" we only have two instances
with _DEBUG, both in transport.

[PATCH resend] SubmittingPatches: mention the git contacts command

[Thomas Gummerer]

Instead of just mentioning 'git blame' and 'git shortlog', which make it
quite hard for new contributors to pick out the appropriate list of
people to cc on their patch series, mention the 'git contacts' utility,
which makes it much easier to get a reasonable list of contacts for a
change.

This should help new contributors pick out a reasonable cc list by
simply using a single command.

Signed-off-by: Thomas Gummerer 
---

I've originally sent this at <20180316213323.GC2224@hank>, during an
the rc period.  Eric had some comments, which I interpreted as being
okay with the change (hope I'm not mistaken there :)).  As I still
think this would be an improvement for new contributors, I'm resending
it here.

 Documentation/SubmittingPatches | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index a1d0feca36..945f8edb46 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -260,8 +260,8 @@ that starts with `-BEGIN PGP SIGNED MESSAGE-`.  
That is
 not a text/plain, it's something else.
 
 Send your patch with "To:" set to the mailing list, with "cc:" listing
-people who are involved in the area you are touching (the output from
-`git blame $path` and `git shortlog --no-merges $path` would help to
+people who are involved in the area you are touching (the `git
+contacts` command in `contrib/contacts/` can help to
 identify them), to solicit comments and reviews.
 
 :1: footnote:[The current maintainer: gits...@pobox.com]
-- 
2.16.2.804.g6dcf76e11

Re: [PATCH v6 0/6] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

On 04/09, Eric Sunshine wrote:
> On Mon, Apr 9, 2018 at 3:30 PM, Thomas Gummerer  wrote:
> > On 04/08, Eric Sunshine wrote:
> >> As with Junio, I'm fine with this hidden option (for now), however, I
> >> think you can take this a step further. Rather than having a (hidden)
> >> git-reset option which suppresses "HEAD is now at...", instead have a
> >> (hidden) option which augments the message. For example,
> >> --new-head-desc="New worktree" would make it output "New worktree HEAD
> >> is now at...". Changes to builtin/reset.c to support this would hardly
> >> be larger than the changes you already made.
> >
> > Something else I just noticed that may make this a worse solution is
> > that this breaks the sentence in two pieces for translators.  I guess
> > we could somehow get the "New worktree" part of the option translated,
> > but that still means that if some language would require to move parts
> > of the sentence around that would be less than ideal for translation.
> 
> Good point.
> 
> One solution would be to have the new hidden option replace the string
> entirely: --new-head-msg="New worktree HEAD is now at %s", which would
> allow translators to deal with the entire sentence. Even clearer would
> be to drop "now", as in "New worktree HEAD is at %s". (Default in
> reset.c would still be "HEAD is now at %s", of course.)
> 
> Another solution would be not to augment the "HEAD is now at..."
> message at all. I realize that that augmentation was one of the
> original motivations for this patch series, but with the upcoming
> restoration of the "Preparing worktree" message:

My original motivation of the series was to just make the new dwim
work :)  Because that's adding some magic, the secondary motivation
became improving the UI, to help users see which dwim was used.  I
felt like this was going to be one of those improvements, especially
after we get rid of the "Preparing ..." line.

I do however like your suggestion of the "Preparing worktree (_branch
disposition_)", as that doesn't add more lines to the output, while
still giving a good indication of what exactly is happening.  At that
point just showing "HEAD is now at ..." is fine by me, and doesn't
require adding the hidden flag to 'git reset'.  So I'm happy just
dropping the change in the message here, which will simplify things.

Thanks for the suggestion!

> Preparing worktree (_branch disposition_)
> HEAD is now at ...
> 
> it seems clear enough (at least to me) from the context introduced by
> the "Preparing..." message that "HEAD is now at..." refers to HEAD in
> the worktree. (But that's just my opinion.)
> 
> > Would factoring out what we have in 'print_new_head_line()' into some
> > common code, maybe in 'pretty.c', and still doing the printing from
> > here be a reasonable tradeoff?
> 
> Isn't that getting uglier again? Not only would you have to publish
> that function, but you'd still need the hidden git-reset
> --show-new-head-line option.
> 
> Also, you'd end up calling that function from within low-level worker
> worktree.c:add_worktree(), thus making it take on UI duties, which
> would be nice to avoid if possible. (Unfortunately, the alternate idea
> of having worktree.c:add() handle this UI task doesn't quite work
> since add_worktree() doesn't return sufficient information for add()
> to know whether or not it should print "HEAD is now at..."; however,
> add_worktree() could be augmented to return more information.)
> 
> So, I don't presently have a good answer. I'm partial to the idea of
> not augmenting "HEAD is now at...", partly because context makes it
> relatively clear that it applies to the new worktree and partly
> because it's simpler (less implementation work for you) to leave it as
> is. If that choice isn't desirable, then next best would be for
> --new-head-msg= to replace the entire "HEAD is now at..." string
> rather than augmenting it.

[PATCH v2 2/2] fsmonitor: switch to use new fsexcludes logic and remove unused untracked cache based logic

[Ben Peart]

Update fsmonitor to utilize the new fsexcludes based logic for excluding paths
that do not need to be scaned for new or modified files.  Remove the old logic
in dir.c that utilized the untracked cache (if enabled) to accomplish the same
goal.

Signed-off-by: Ben Peart 
---
 dir.c   | 10 +++---
 dir.h   |  2 --
 fsmonitor.c | 21 ++---
 fsmonitor.h | 10 +++---
 t/t7519-status-fsmonitor.sh | 14 +++---
 5 files changed, 11 insertions(+), 46 deletions(-)

diff --git a/dir.c b/dir.c
index 1aa639b9f4..28c2c83f76 100644
--- a/dir.c
+++ b/dir.c
@@ -19,7 +19,6 @@
 #include "varint.h"
 #include "ewah/ewok.h"
 #include "fsexcludes.h"
-#include "fsmonitor.h"
 
 /*
  * Tells read_directory_recursive how a file or directory should be treated.
@@ -1827,12 +1826,9 @@ static int valid_cached_dir(struct dir_struct *dir,
if (!untracked)
return 0;
 
-   /*
-* With fsmonitor, we can trust the untracked cache's valid field.
-*/
-   refresh_fsmonitor(istate);
-   if (!(dir->untracked->use_fsmonitor && untracked->valid)) {
-   if (lstat(path->len ? path->buf : ".", )) {
+   if (!untracked->valid) {
+   if (stat(path->len ? path->buf : ".", )) {
+   invalidate_directory(dir->untracked, untracked);
memset(>stat_data, 0, 
sizeof(untracked->stat_data));
return 0;
}
diff --git a/dir.h b/dir.h
index b0758b82a2..e67ccfbb29 100644
--- a/dir.h
+++ b/dir.h
@@ -139,8 +139,6 @@ struct untracked_cache {
int gitignore_invalidated;
int dir_invalidated;
int dir_opened;
-   /* fsmonitor invalidation data */
-   unsigned int use_fsmonitor : 1;
 };
 
 struct dir_struct {
diff --git a/fsmonitor.c b/fsmonitor.c
index 6d7bcd5d0e..dd67eef851 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "dir.h"
 #include "ewah/ewok.h"
+#include "fsexcludes.h"
 #include "fsmonitor.h"
 #include "run-command.h"
 #include "strbuf.h"
@@ -125,12 +126,7 @@ static void fsmonitor_refresh_callback(struct index_state 
*istate, const char *n
ce->ce_flags &= ~CE_FSMONITOR_VALID;
}
 
-   /*
-* Mark the untracked cache dirty even if it wasn't found in the index
-* as it could be a new untracked file.
-*/
trace_printf_key(_fsmonitor, "fsmonitor_refresh_callback '%s'", 
name);
-   untracked_cache_invalidate_path(istate, name, 0);
 }
 
 void refresh_fsmonitor(struct index_state *istate)
@@ -184,11 +180,8 @@ void refresh_fsmonitor(struct index_state *istate)
/* Mark all entries invalid */
for (i = 0; i < istate->cache_nr; i++)
istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
-
-   if (istate->untracked)
-   istate->untracked->use_fsmonitor = 0;
}
-   strbuf_release(_result);
+   fsexcludes_init(_result);
 
/* Now that we've updated istate, save the last_update time */
istate->fsmonitor_last_update = last_update;
@@ -207,12 +200,6 @@ void add_fsmonitor(struct index_state *istate)
for (i = 0; i < istate->cache_nr; i++)
istate->cache[i]->ce_flags &= ~CE_FSMONITOR_VALID;
 
-   /* reset the untracked cache */
-   if (istate->untracked) {
-   add_untracked_cache(istate);
-   istate->untracked->use_fsmonitor = 1;
-   }
-
/* Update the fsmonitor state */
refresh_fsmonitor(istate);
}
@@ -241,10 +228,6 @@ void tweak_fsmonitor(struct index_state *istate)
 
/* Mark all previously saved entries as dirty */
ewah_each_bit(istate->fsmonitor_dirty, 
fsmonitor_ewah_callback, istate);
-
-   /* Now mark the untracked cache for fsmonitor usage */
-   if (istate->untracked)
-   istate->untracked->use_fsmonitor = 1;
}
 
ewah_free(istate->fsmonitor_dirty);
diff --git a/fsmonitor.h b/fsmonitor.h
index 65f3743636..f7adfc1f7c 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -35,8 +35,7 @@ extern void tweak_fsmonitor(struct index_state *istate);
 
 /*
  * Run the configured fsmonitor integration script and clear the
- * CE_FSMONITOR_VALID bit for any files returned as dirty.  Also invalidate
- * any corresponding untracked cache directory structures. Optimized to only
+ * CE_FSMONITOR_VALID bit for any files returned as dirty. Optimized to only
  * run the first time it is called.
  */
 extern void refresh_fsmonitor(struct index_state *istate);
@@ -55,17 +54,14 @@ static inline void mark_fsmonitor_valid(struct cache_entry 
*ce)
 }
 
 /*
- * Clear the given cache entry's 

[PATCH v2 0/2] fsexcludes: Add programmatic way to exclude files

[Ben Peart]

Updated to incorporate feedback from V1.

I'd really like a close review of the changes in dir.c where I added the calls
to fsexcludes_is_excluded_from().  While they work and pass all the git tests
as well as our internal functional tests, I'd like to be sure I haven't missed
anything.

Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/08442c209d
Checkout: git fetch https://github.com/benpeart/git fsexcludes-v2 && git 
checkout 08442c209d


### Interdiff (v1..v2):

diff --git a/fsexcludes.c b/fsexcludes.c
index 07bfe376a0..0ef57f107b 100644
--- a/fsexcludes.c
+++ b/fsexcludes.c
@@ -33,7 +33,6 @@ static int check_fsexcludes_hashmap(struct hashmap *map, 
const char *pattern, in
char *slash;
 
/* Check straight mapping */
-   strbuf_reset();
strbuf_add(, pattern, patternlen);
fse.pattern = sb.buf;
fse.patternlen = sb.len;
@@ -155,7 +154,6 @@ static int check_directory_hashmap(struct hashmap *map, 
const char *pathname, in
struct fsexcludes fse;
 
/* Check for directory */
-   strbuf_reset();
strbuf_add(, pathname, pathlen);
strbuf_addch(, '/');
fse.pattern = sb.buf;
@@ -198,13 +196,16 @@ int fsexcludes_is_excluded_from(struct index_state 
*istate,
return -1;
 }
 
-void fsexcludes_init(struct strbuf *sb) {
+void fsexcludes_init(struct strbuf *sb)
+{
fsexcludes_initialized = 1;
fsexcludes_data = *sb;
+   strbuf_detach(sb, NULL);
 }
 
-void fsexcludes_free() {
+void fsexcludes_free(void) {
strbuf_release(_data);
hashmap_free(_hashmap, 1);
hashmap_free(_directory_hashmap, 1);
+   fsexcludes_initialized = 0;
 }
diff --git a/fsexcludes.h b/fsexcludes.h
index 1c4101343c..10246daa02 100644
--- a/fsexcludes.h
+++ b/fsexcludes.h
@@ -6,16 +6,18 @@
  * where git will scan for untracked files.  This is used to speed up the
  * scan by avoiding scanning parts of the work directory that do not have
  * any new files.
- *
  */
 
 /*
  * sb should contain a NUL separated list of path names of the files
  * and/or directories that should be checked.  Any path not listed will
  * be excluded from the scan.
+ *
+ * NOTE: fsexcludes_init() will take ownership of the storage passed in
+ * sb and will reset sb to `STRBUF_INIT`
  */
 void fsexcludes_init(struct strbuf *sb);
-void fsexcludes_free();
+void fsexcludes_free(void);
 
 /*
  * Return 1 for exclude, 0 for include and -1 for undecided.


### Patches

Ben Peart (2):
  fsexcludes: add a programmatic way to exclude files from git's working
directory traversal logic
  fsmonitor: switch to use new fsexcludes logic and remove unused
untracked cache based logic

 Makefile|   1 +
 dir.c   |  33 --
 dir.h   |   2 -
 fsexcludes.c| 211 
 fsexcludes.h|  29 +
 fsmonitor.c |  21 +---
 fsmonitor.h |  10 +-
 t/t7519-status-fsmonitor.sh |  14 +--
 8 files changed, 273 insertions(+), 48 deletions(-)
 create mode 100644 fsexcludes.c
 create mode 100644 fsexcludes.h


base-commit: 0b0cc9f86731f894cff8dd25299a9b38c254569e
-- 
2.17.0.windows.1

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

[Eric Sunshine]

On Wed, Apr 11, 2018 at 2:07 PM, Stefan Beller  wrote:
> On Wed, Apr 11, 2018 at 10:57 AM, Harald Nordgren
>  wrote:
>> There have been no new comments for the last few days. I know Jeff
>> King will be away for the next two weeks, but should we still move
>> forward with this? The initial reactions to the idea seemed positive.
>
> Please see the "What's cooking?" email on the mailing list that is
> sent out periodically by Junio.
>
>> * jk/ref-array-push (2018-04-09) 3 commits
>> - ref-filter: factor ref_array pushing into its own function
>> - ref-filter: make ref_array_item allocation more consistent
>> - ref-filter: use "struct object_id" consistently
>> (this branch is used by hn/sort-ls-remote.)
>>
>> API clean-up aournd ref-filter code.
>>
>> Will merge to 'next'.
>
> It will be merged to next and if no people speak up (due to bugs
> observed or such)

I would guess, however, that Harald is wondering more specifically
about the "--sort" patch he added atop Peff's patches. Disposition for
_that_ patch is not specified in the latest "What's cooking":

* hn/sort-ls-remote (2018-04-09) 1 commit
 - ls-remote: create '--sort' option
 (this branch uses jk/ref-array-push.)

 "git ls-remote" learned an option to allow sorting its output based
 on the refnames being shown.

[PATCH v2 1/2] fsexcludes: add a programmatic way to exclude files from git's working directory traversal logic

[Ben Peart]

The File System Excludes module is a new programmatic way to exclude files and
folders from git's traversal of the working directory.  fsexcludes_init() should
be called with a string buffer that contains a NUL separated list of path names
of the files and/or directories that should be included.  Any path not listed
will be excluded. The paths should be relative to the root of the working
directory and be separated by a single NUL.

The excludes logic in dir.c has been updated to honor the results of
fsexcludes_is_excluded_from().  If fsexcludes does not exclude the file, the
normal excludes logic is also checked as it could further reduce the set of
files that should be included.

Signed-off-by: Ben Peart 
---
 Makefile |   1 +
 dir.c|  23 +-
 fsexcludes.c | 211 +++
 fsexcludes.h |  29 +++
 4 files changed, 262 insertions(+), 2 deletions(-)
 create mode 100644 fsexcludes.c
 create mode 100644 fsexcludes.h

diff --git a/Makefile b/Makefile
index 96f6138f63..c102d2f75a 100644
--- a/Makefile
+++ b/Makefile
@@ -819,6 +819,7 @@ LIB_OBJS += exec_cmd.o
 LIB_OBJS += fetch-object.o
 LIB_OBJS += fetch-pack.o
 LIB_OBJS += fsck.o
+LIB_OBJS += fsexcludes.o
 LIB_OBJS += fsmonitor.o
 LIB_OBJS += gettext.o
 LIB_OBJS += gpg-interface.o
diff --git a/dir.c b/dir.c
index 63a917be45..1aa639b9f4 100644
--- a/dir.c
+++ b/dir.c
@@ -18,6 +18,7 @@
 #include "utf8.h"
 #include "varint.h"
 #include "ewah/ewok.h"
+#include "fsexcludes.h"
 #include "fsmonitor.h"
 
 /*
@@ -1102,6 +1103,12 @@ int is_excluded_from_list(const char *pathname,
  struct exclude_list *el, struct index_state *istate)
 {
struct exclude *exclude;
+
+   if (*dtype == DT_UNKNOWN)
+   *dtype = get_dtype(NULL, istate, pathname, pathlen);
+   if (fsexcludes_is_excluded_from(istate, pathname, pathlen, *dtype) > 0)
+   return 1;
+
exclude = last_exclude_matching_from_list(pathname, pathlen, basename,
  dtype, el, istate);
if (exclude)
@@ -1317,8 +1324,15 @@ struct exclude *last_exclude_matching(struct dir_struct 
*dir,
 int is_excluded(struct dir_struct *dir, struct index_state *istate,
const char *pathname, int *dtype_p)
 {
-   struct exclude *exclude =
-   last_exclude_matching(dir, istate, pathname, dtype_p);
+   struct exclude *exclude;
+   int pathlen = strlen(pathname);
+
+   if (*dtype_p == DT_UNKNOWN)
+   *dtype_p = get_dtype(NULL, istate, pathname, pathlen);
+   if (fsexcludes_is_excluded_from(istate, pathname, pathlen, *dtype_p) > 
0)
+   return 1;
+
+   exclude = last_exclude_matching(dir, istate, pathname, dtype_p);
if (exclude)
return exclude->flags & EXC_FLAG_NEGATIVE ? 0 : 1;
return 0;
@@ -1671,6 +1685,9 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
if (dtype != DT_DIR && has_path_in_index)
return path_none;
 
+   if (fsexcludes_is_excluded_from(istate, path->buf, path->len, dtype) > 
0)
+   return path_excluded;
+
/*
 * When we are looking at a directory P in the working tree,
 * there are three cases:
@@ -2011,6 +2028,8 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
/* add the path to the appropriate result list */
switch (state) {
case path_excluded:
+   if (fsexcludes_is_excluded_from(istate, path.buf, 
path.len, DTYPE(cdir.de)) > 0)
+   break;
if (dir->flags & DIR_SHOW_IGNORED)
dir_add_name(dir, istate, path.buf, path.len);
else if ((dir->flags & DIR_SHOW_IGNORED_TOO) ||
diff --git a/fsexcludes.c b/fsexcludes.c
new file mode 100644
index 00..0ef57f107b
--- /dev/null
+++ b/fsexcludes.c
@@ -0,0 +1,211 @@
+#include "cache.h"
+#include "fsexcludes.h"
+#include "hashmap.h"
+#include "strbuf.h"
+
+static int fsexcludes_initialized = 0;
+static struct strbuf fsexcludes_data = STRBUF_INIT;
+static struct hashmap fsexcludes_hashmap;
+static struct hashmap parent_directory_hashmap;
+
+struct fsexcludes {
+   struct hashmap_entry ent; /* must be the first member! */
+   const char *pattern;
+   int patternlen;
+};
+
+static unsigned int(*fsexcludeshash)(const void *buf, size_t len);
+static int(*fsexcludescmp)(const char *a, const char *b, size_t len);
+
+static int fsexcludes_hashmap_cmp(const void *unused_cmp_data,
+   const void *a, const void *b, const void *key)
+{
+   const struct fsexcludes *fse1 = a;
+   const struct fsexcludes *fse2 = b;
+
+   return fsexcludescmp(fse1->pattern, fse2->pattern, fse1->patternlen);
+}
+
+static int check_fsexcludes_hashmap(struct hashmap *map, const char *pattern, 
int 

Re: [PATCH 0/6] Compute and consume generation numbers

[Derrick Stolee]

On 4/11/2018 3:32 PM, Jakub Narebski wrote:

What would you suggest as a good test that could imply performance? The
Google Colab notebook linked to above includes a function to count
number of commits (nodes / vertices in the commit graph) walked,
currently in the worst case scenario.


The two main questions to consider are:

1. Can X reach Y?
2. What is the set of merge-bases between X and Y?

And the thing to measure is a commit count. If possible, it would be 
good to count commits walked (commits whose parent list is enumerated) 
and commits inspected (commits that were listed as a parent of some 
walked commit). Walked commits require a commit parse -- albeit from the 
commit-graph instead of the ODB now -- while inspected commits only 
check the in-memory cache.


For git.git and Linux, I like to use the release tags as tests. They 
provide a realistic view of the linear history, and maintenance releases 
have their own history from the major releases.



I have tried finding number of false positives for level (generation
number) filter and for FELINE index, and number of false negatives for
min-post intervals in the spanning tree (for DFS tree) for 1
randomly selected pairs of commits... but I don't think this is a good
benchmark.


What is a false-positive? A case where gen(X) < gen(Y) but Y cannot 
reach X? I do not think that is a great benchmark, but I guess it is 
something to measure.



I Linux kernel sources 
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
that has 750832 nodes and 811733 edges, and 563747941392 possible
directed pairs, we have for 1 randomly selected pairs of commits:

   level-filter has91 =  0.91% [all] false positives
   FELINE index has78 =  0.78% [all] false positives
   FELINE index has 1.16667 less false positives than level filter

   min-post spanning-tree intervals has  3641 = 36.41% [all] false
   negatives


Perhaps something you can do instead of sampling from N^2 commits in 
total is to select a pair of generations (say, G = 2, G' = 20100) or 
regions of generations ( 2 <= G <= 20050, 20100 <= G' <= 20150) and 
see how many false positives you see by testing all pairs (one from each 
level). The delta between the generations may need to be smaller to 
actually have a large proportion of unreachable pairs. Try different 
levels, since major version releases tend to "pinch" the commit graph to 
a common history.



For git.git repository (https://github.com/git/git.git) that has 52950
nodes and 65887 edges the numbers are slighly more in FELINE index
favor (also out of 1 random pairs):

   level-filter has   504 =  9.11% false positives
   FELINE index has   125 =  2.26% false positives
   FELINE index has 4.032 less false positives than level filter

This is for FELINE which does not use level / generatio-numbers filter.


Thanks,
-Stolee

Re: [PATCH v1 1/2] fsexcludes: add a programmatic way to exclude files from git's working directory traversal logic

[Ben Peart]

On 4/10/2018 6:09 PM, Martin Ågren wrote:

On 10 April 2018 at 23:04, Ben Peart  wrote:

The File System Excludes module is a new programmatic way to exclude files and
folders from git's traversal of the working directory.  fsexcludes_init() should
be called with a string buffer that contains a NUL separated list of path names
of the files and/or directories that should be included.  Any path not listed
will be excluded. The paths should be relative to the root of the working
directory and be separated by a single NUL.

The excludes logic in dir.c has been updated to honor the results of
fsexcludes_is_excluded_from().  If fsexcludes does not exclude the file, the
normal excludes logic is also checked as it could further reduce the set of
files that should be included.


Here you mention a change in dir.c...


  Makefile |   1 +
  fsexcludes.c | 210 +++
  fsexcludes.h |  27 +++
  3 files changed, 238 insertions(+)


... but this patch does not seem to touch dir.c at all.



Oops! Fixed in V2.


+static int check_fsexcludes_hashmap(struct hashmap *map, const char *pattern, 
int patternlen)
+{
+   struct strbuf sb = STRBUF_INIT;
+   struct fsexcludes fse;
+   char *slash;
+
+   /* Check straight mapping */
+   strbuf_reset();


You could drop this strbuf_reset(). Or did you intend to use a static
struct strbuf?



Good point, fixed in V2.


+   /*
+* Check to see if it matches a directory or any path
+* underneath it.  In other words, 'a/b/foo.txt' will match
+* '/', 'a/', and 'a/b/'.
+*/
+   slash = strchr(sb.buf, '/');
+   while (slash) {
+   fse.pattern = sb.buf;
+   fse.patternlen = slash - sb.buf + 1;
+   hashmap_entry_init(, fsexcludeshash(fse.pattern, 
fse.patternlen));
+   if (hashmap_get(map, , NULL)) {
+   strbuf_release();
+   return 0;
+   }
+   slash = strchr(slash + 1, '/');
+   }


Maybe a for-loop would make this slightly more obvious:

for (slash = strchr(sb.buf, '/'); slash; slash = strchr(slash + 1, '/'))

On second thought, maybe not.


+   entry = buf = fsexcludes_data->buf;
+   len = fsexcludes_data->len;
+   for (i = 0; i < len; i++) {
+   if (buf[i] == '\0') {
+   fsexcludes_hashmap_add(map, entry, buf + i - entry);
+   entry = buf + i + 1;
+   }
+   }
+}


Very minor: I would have found "buf - entry + i" clearer here and later,
but I'm sure you'll find someone of the opposing opinion (e.g.,
yourself). ;-)


+static int check_directory_hashmap(struct hashmap *map, const char *pathname, 
int pathlen)
+{
+   struct strbuf sb = STRBUF_INIT;
+   struct fsexcludes fse;
+
+   /* Check for directory */
+   strbuf_reset();


Same comment as above about this spurious reset.


Good point, fixed in V2.




+   if (hashmap_get(map, , NULL)) {
+   strbuf_release();
+   return 0;
+   }
+
+   strbuf_release();
+   return 1;
+}
+
+/*
+ * Return 1 for exclude, 0 for include and -1 for undecided.
+ */
+int fsexcludes_is_excluded_from(struct index_state *istate,
+   const char *pathname, int pathlen, int dtype)
+{


Will we at some point regret not being able to "return negative on
error"? I guess that would be "-2" or "negative other than -1".



This function is modeled after the other is_excluded_from* functions in 
dir.c so that the return value can be handled the same way.  I don't 
anticipate any need for change but you're right, we could return some 
other "negative other than -1" if it was ever needed.



+void fsexcludes_init(struct strbuf *sb) {
+   fsexcludes_initialized = 1;
+   fsexcludes_data = *sb;
+}


Grabbing the strbuf's members looks a bit odd. Is this
performance-sensitive enough that you do not want to make a copy? If a
caller releases its strbuf, which would normally be a good thing to do,
we may be in big trouble later. (Not only may .buf be stale, .len may
indicate we actually have something to read.)

I can understand that you do not want to pass a pointer+len, and that it
is not enough to pass sb.buf, since the string may contain nuls.

Maybe detach the original strbuf? That way, if a caller releases its
buffer, that is a no-op. A caller which goes on to use its buffer should
fail quickly and obviously. Right now, an incorrect caller would
probably fail more subtly and less reproducibly.

In any case, maybe document this in the .h-file?


Great suggestion!  I was looking for a better way to ensure the buffer 
ownership transfer was robust.  I'll do both strbuf_detach() and update 
the header file.  Thank you.




Martin

Re: [PATCH 0/6] Compute and consume generation numbers

[Jakub Narebski]

Derrick Stolee  writes:

> On 4/7/2018 12:55 PM, Jakub Narebski wrote:
>> Currently I am at the stage of reproducing results in FELINE paper:
>> "Reachability Queries in Very Large Graphs: A Fast Refined Online Search
>> Approach" by Renê R. Veloso, Loïc Cerf, Wagner Meira Jr and Mohammed
>> J. Zaki (2014).  This paper is available in the PDF form at
>> https://openproceedings.org/EDBT/2014/paper_166.pdf
>>
>> The Jupyter Notebook (which runs on Google cloud, but can be also run
>> locally) uses Python kernel, NetworkX librabry for graph manipulation,
>> and matplotlib (via NetworkX) for display.
>>
>> Available at:
>> https://colab.research.google.com/drive/1V-U7_slu5Z3s5iEEMFKhLXtaxSu5xyzg
>> https://drive.google.com/file/d/1V-U7_slu5Z3s5iEEMFKhLXtaxSu5xyzg/view?usp=sharing
>>
>> I hope that could be of help, or at least interesting
>
> Let me know when you can give numbers (either raw performance or # of
> commits walked) for real-world Git commit graphs. The Linux repo is a
> good example to use for benchmarking, but I also use the Kotlin repo
> sometimes as it has over a million objects and over 250K commits.

As I am curently converting git repository into commit graph, number of
objects doesn't matter.

Though Kotlin is nicely in largish size set, not as large as Linux
kernel which has 750K commits, but mich larger than git.git with 65K
commits.

> Of course, the only important statistic at the end of the day is the
> end-to-end time of a 'git ...' command. Your investigations should
> inform whether it is worth prototyping the feature in the git
> codebase.

What would you suggest as a good test that could imply performance?  The
Google Colab notebook linked to above includes a function to count
number of commits (nodes / vertices in the commit graph) walked,
currently in the worst case scenario.


I have tried finding number of false positives for level (generation
number) filter and for FELINE index, and number of false negatives for
min-post intervals in the spanning tree (for DFS tree) for 1
randomly selected pairs of commits... but I don't think this is a good
benchmark.

I Linux kernel sources 
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git)
that has 750832 nodes and 811733 edges, and 563747941392 possible
directed pairs, we have for 1 randomly selected pairs of commits:

  level-filter has91 =  0.91% [all] false positives
  FELINE index has78 =  0.78% [all] false positives
  FELINE index has 1.16667 less false positives than level filter

  min-post spanning-tree intervals has  3641 = 36.41% [all] false
  negatives

For git.git repository (https://github.com/git/git.git) that has 52950
nodes and 65887 edges the numbers are slighly more in FELINE index
favor (also out of 1 random pairs):

  level-filter has   504 =  9.11% false positives
  FELINE index has   125 =  2.26% false positives
  FELINE index has 4.032 less false positives than level filter

This is for FELINE which does not use level / generatio-numbers filter.

Regards,
--
Jakub Narębski

Reply Immediately

[Benjamin Joseph]

Good Day
My name is Benjamin Joseph, an accountant with C Bank UK, there is an 
opportunity in our bank that I would like us to talk about so we can join hands 
together to achieve it for our benefit. please get back to me for details.
Thanks you
Benjamin Joseph.

Re: [PATCH v2 04/10] commit-graph: compute generation numbers

[Eric Sunshine]

On Wed, Apr 11, 2018 at 9:02 AM, Derrick Stolee  wrote:
> On 4/10/2018 10:51 PM, Junio C Hamano wrote:
>> In case we want to do the "we know this is very large, but we do not
>> know the exact value", we may actually want a mode where we can
>> pretend that GENERATION_NUMBER_MAX is set to quite low (say 256) and
>> make sure that the code to handle overflow behaves sensibly.
>
> I agree. I wonder how we can effectively expose this value into a test. It's
> probably not sufficient to manually test using compiler flags ("-D
> GENERATION_NUMBER_MAX=8").

A few similar cases of tests needing to tweak some behavior do so by
environment variable. See, for instance, GIT_GETTEXT_POISON and
GIT_FSMONITOR_TEST.

Re: [PATCH v6 15/15] rebase -i --rebase-merges: add a section to the man page

[Eric Sunshine]

On Wed, Apr 11, 2018 at 11:35 AM, Phillip Wood
 wrote:
> On 10/04/18 13:30, Johannes Schindelin wrote:
>> +The `reset` command is essentially a `git reset --hard` to the specified
>> +revision (typically a previously-labeled one).
>
> s/labeled/labelled/

American vs. British English spelling.

CodingGuidelines and SubmittingPatches talk about this. Junio
summarizes the issue well in [1]. The TL;DR is to lean toward the
American English spelling.

[1]: https://public-inbox.org/git/xmqq4m9gpebm@gitster.mtv.corp.google.com/

[PATCH v4 2/3] fast-import: introduce mem_pool type

[Jameson Miller]

Introduce the mem_pool type which encapsulates all the information necessary to
manage a pool of memory. This change moves the existing variables in
fast-import used to support the global memory pool to use this structure. It
also renames variables that are no longer used by memory pools to reflect their
more scoped usage.

These changes allow for the multiple instances of a memory pool to
exist and be reused outside of fast-import. In a future commit the
mem_pool type will be moved to its own file.

Signed-off-by: Jameson Miller 
---
 fast-import.c | 81 ++-
 1 file changed, 52 insertions(+), 29 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 38af0a294b..48d4797ab5 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -218,6 +218,19 @@ struct mp_block {
uintmax_t space[FLEX_ARRAY]; /* more */
 };
 
+struct mem_pool {
+   struct mp_block *mp_block;
+
+   /*
+* The amount of available memory to grow the pool by.
+* This size does not include the overhead for the mp_block.
+*/
+   size_t block_alloc;
+
+   /* The total amount of memory allocated by the pool. */
+   size_t pool_alloc;
+};
+
 struct atom_str {
struct atom_str *next_atom;
unsigned short str_len;
@@ -306,9 +319,8 @@ static int global_argc;
 static const char **global_argv;
 
 /* Memory pools */
-static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mp_block);
-static size_t total_allocd;
-static struct mp_block *mp_block_head;
+static struct mem_pool fi_mem_pool =  {NULL, 2*1024*1024 -
+  sizeof(struct mp_block), 0 };
 
 /* Atom management */
 static unsigned int atom_table_sz = 4451;
@@ -343,6 +355,7 @@ static unsigned int tree_entry_alloc = 1000;
 static void *avail_tree_entry;
 static unsigned int avail_tree_table_sz = 100;
 static struct avail_tree_content **avail_tree_table;
+static size_t tree_entry_allocd;
 static struct strbuf old_tree = STRBUF_INIT;
 static struct strbuf new_tree = STRBUF_INIT;
 
@@ -636,7 +649,21 @@ static unsigned int hc_str(const char *s, size_t len)
return r;
 }
 
-static void *pool_alloc(size_t len)
+static struct mp_block *mem_pool_alloc_block(struct mem_pool *mem_pool, size_t 
block_alloc)
+{
+   struct mp_block *p;
+
+   mem_pool->pool_alloc += sizeof(struct mp_block) + block_alloc;
+   p = xmalloc(st_add(sizeof(struct mp_block), block_alloc));
+   p->next_block = mem_pool->mp_block;
+   p->next_free = (char *)p->space;
+   p->end = p->next_free + block_alloc;
+   mem_pool->mp_block = p;
+
+   return p;
+}
+
+static void *mem_pool_alloc(struct mem_pool *mem_pool, size_t len)
 {
struct mp_block *p;
void *r;
@@ -645,21 +672,17 @@ static void *pool_alloc(size_t len)
if (len & (sizeof(uintmax_t) - 1))
len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
 
-   for (p = mp_block_head; p; p = p->next_block)
-   if ((p->end - p->next_free >= len))
+   for (p = mem_pool->mp_block; p; p = p->next_block)
+   if (p->end - p->next_free >= len)
break;
 
if (!p) {
-   if (len >= (mem_pool_alloc/2)) {
-   total_allocd += len;
+   if (len >= (mem_pool->block_alloc / 2)) {
+   mem_pool->pool_alloc += len;
return xmalloc(len);
}
-   total_allocd += sizeof(struct mp_block) + mem_pool_alloc;
-   p = xmalloc(st_add(sizeof(struct mp_block), mem_pool_alloc));
-   p->next_block = mp_block_head;
-   p->next_free = (char *) p->space;
-   p->end = p->next_free + mem_pool_alloc;
-   mp_block_head = p;
+
+   p = mem_pool_alloc_block(mem_pool, mem_pool->block_alloc);
}
 
r = p->next_free;
@@ -667,10 +690,10 @@ static void *pool_alloc(size_t len)
return r;
 }
 
-static void *pool_calloc(size_t count, size_t size)
+static void *mem_pool_calloc(struct mem_pool *mem_pool, size_t count, size_t 
size)
 {
-   size_t len = count * size;
-   void *r = pool_alloc(len);
+   size_t len = st_mult(count, size);
+   void *r = mem_pool_alloc(mem_pool, len);
memset(r, 0, len);
return r;
 }
@@ -678,7 +701,7 @@ static void *pool_calloc(size_t count, size_t size)
 static char *pool_strdup(const char *s)
 {
size_t len = strlen(s) + 1;
-   char *r = pool_alloc(len);
+   char *r = mem_pool_alloc(_mem_pool, len);
memcpy(r, s, len);
return r;
 }
@@ -687,7 +710,7 @@ static void insert_mark(uintmax_t idnum, struct 
object_entry *oe)
 {
struct mark_set *s = marks;
while ((idnum >> s->shift) >= 1024) {
-   s = pool_calloc(1, sizeof(struct mark_set));
+   s = mem_pool_calloc(_mem_pool, 1, sizeof(struct mark_set));
  

[PATCH v4 3/3] Move reusable parts of memory pool into its own file

[Jameson Miller]

This moves the reusable parts of the memory pool logic used by
fast-import.c into its own file for use by other components.

Signed-off-by: Jameson Miller 
---
 Makefile  |  1 +
 fast-import.c | 70 +--
 mem-pool.c| 55 ++
 mem-pool.h| 34 +
 4 files changed, 91 insertions(+), 69 deletions(-)
 create mode 100644 mem-pool.c
 create mode 100644 mem-pool.h

diff --git a/Makefile b/Makefile
index f181687250..5aa4f8064f 100644
--- a/Makefile
+++ b/Makefile
@@ -844,6 +844,7 @@ LIB_OBJS += log-tree.o
 LIB_OBJS += mailinfo.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
+LIB_OBJS += mem-pool.o
 LIB_OBJS += merge.o
 LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
diff --git a/fast-import.c b/fast-import.c
index 48d4797ab5..05d1079d23 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -170,6 +170,7 @@ Format of STDIN stream:
 #include "run-command.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "mem-pool.h"
 
 #define PACK_ID_BITS 16
 #define MAX_PACK_ID ((1<pool_alloc += sizeof(struct mp_block) + block_alloc;
-   p = xmalloc(st_add(sizeof(struct mp_block), block_alloc));
-   p->next_block = mem_pool->mp_block;
-   p->next_free = (char *)p->space;
-   p->end = p->next_free + block_alloc;
-   mem_pool->mp_block = p;
-
-   return p;
-}
-
-static void *mem_pool_alloc(struct mem_pool *mem_pool, size_t len)
-{
-   struct mp_block *p;
-   void *r;
-
-   /* round up to a 'uintmax_t' alignment */
-   if (len & (sizeof(uintmax_t) - 1))
-   len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
-
-   for (p = mem_pool->mp_block; p; p = p->next_block)
-   if (p->end - p->next_free >= len)
-   break;
-
-   if (!p) {
-   if (len >= (mem_pool->block_alloc / 2)) {
-   mem_pool->pool_alloc += len;
-   return xmalloc(len);
-   }
-
-   p = mem_pool_alloc_block(mem_pool, mem_pool->block_alloc);
-   }
-
-   r = p->next_free;
-   p->next_free += len;
-   return r;
-}
-
-static void *mem_pool_calloc(struct mem_pool *mem_pool, size_t count, size_t 
size)
-{
-   size_t len = st_mult(count, size);
-   void *r = mem_pool_alloc(mem_pool, len);
-   memset(r, 0, len);
-   return r;
-}
-
 static char *pool_strdup(const char *s)
 {
size_t len = strlen(s) + 1;
diff --git a/mem-pool.c b/mem-pool.c
new file mode 100644
index 00..389d7af447
--- /dev/null
+++ b/mem-pool.c
@@ -0,0 +1,55 @@
+/*
+ * Memory Pool implementation logic.
+ */
+
+#include "cache.h"
+#include "mem-pool.h"
+
+static struct mp_block *mem_pool_alloc_block(struct mem_pool *mem_pool, size_t 
block_alloc)
+{
+   struct mp_block *p;
+
+   mem_pool->pool_alloc += sizeof(struct mp_block) + block_alloc;
+   p = xmalloc(st_add(sizeof(struct mp_block), block_alloc));
+   p->next_block = mem_pool->mp_block;
+   p->next_free = (char *)p->space;
+   p->end = p->next_free + block_alloc;
+   mem_pool->mp_block = p;
+
+   return p;
+}
+
+void *mem_pool_alloc(struct mem_pool *mem_pool, size_t len)
+{
+   struct mp_block *p;
+   void *r;
+
+   /* round up to a 'uintmax_t' alignment */
+   if (len & (sizeof(uintmax_t) - 1))
+   len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
+
+   for (p = mem_pool->mp_block; p; p = p->next_block)
+   if (p->end - p->next_free >= len)
+   break;
+
+   if (!p) {
+   if (len >= (mem_pool->block_alloc / 2)) {
+   mem_pool->pool_alloc += len;
+   return xmalloc(len);
+   }
+
+   p = mem_pool_alloc_block(mem_pool, mem_pool->block_alloc);
+   }
+
+   r = p->next_free;
+   p->next_free += len;
+   return r;
+}
+
+void *mem_pool_calloc(struct mem_pool *mem_pool, size_t 

[PATCH v4 0/3] Extract memory pool logic into reusable component

[Jameson Miller]

Thank you everyone for taking the time review and provide feedback on
this patch series.

Changes from v3:

  - Based patch off of new commit, to resolve merge conflict.

  - Updated log message in 2/3 based on feedback.

  - Squashed patch from Ramsay Jones into 2/3 to fix warning from
sparse.

  - Updated variable names in 2/3 to reflect updated usage of
variable.

Jameson Miller (3):
  fast-import: rename mem_pool type to mp_block
  fast-import: introduce mem_pool type
  Move reusable parts of memory pool into its own file

 Makefile  |  1 +
 fast-import.c | 77 +--
 mem-pool.c| 55 ++
 mem-pool.h| 34 ++
 4 files changed, 106 insertions(+), 61 deletions(-)
 create mode 100644 mem-pool.c
 create mode 100644 mem-pool.h

-- 
2.14.3

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

[Todd Zullinger]

Hi Stefan,

Stefan Beller wrote:
> Please see the "What's cooking?" email on the mailing list that is
> sent out periodically by Junio.
> the last one is
> https://public-inbox.org/git/xmqqd0z865pk@gitster-ct.c.googlers.com/
> which says:
> 
>> * jk/ref-array-push (2018-04-09) 3 commits
>> - ref-filter: factor ref_array pushing into its own function
>> - ref-filter: make ref_array_item allocation more consistent
>> - ref-filter: use "struct object_id" consistently
>> (this branch is used by hn/sort-ls-remote.)
>>
>> API clean-up aournd ref-filter code.
>>
>> Will merge to 'next'.
> 
> It will be merged to next and if no people speak up (due to bugs
> observed or such)
> then it will be merged to master eventually, later.
> 
> I am not able to find the documentation for the workflow right now,
> though it is partially covered in Documentation/SubmittingPatches.

Perhaps Documentation/howto/maintain-git.txt is the
documentation you're thinking of?

https://kernel.org/pub/software/scm/git/docs/howto/maintain-git.html

There's also MaintNotes in the todo branch:

https://raw.githubusercontent.com/git/git/todo/MaintNotes

-- 
Todd
~~
Be who you are and say what you feel because those who mind don't
matter and those who matter don't mind.
-- Dr Seuss, "Oh the Places You'll Go"

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

[Stefan Beller]

Hi Harald,

On Wed, Apr 11, 2018 at 10:57 AM, Harald Nordgren
 wrote:
> There have been no new comments for the last few days. I know Jeff
> King will be away for the next two weeks, but should we still move
> forward with this? The initial reactions to the idea seemed positive.

Please see the "What's cooking?" email on the mailing list that is
sent out periodically by Junio.
the last one is
https://public-inbox.org/git/xmqqd0z865pk@gitster-ct.c.googlers.com/
which says:

> * jk/ref-array-push (2018-04-09) 3 commits
> - ref-filter: factor ref_array pushing into its own function
> - ref-filter: make ref_array_item allocation more consistent
> - ref-filter: use "struct object_id" consistently
> (this branch is used by hn/sort-ls-remote.)
>
> API clean-up aournd ref-filter code.
>
> Will merge to 'next'.

It will be merged to next and if no people speak up (due to bugs
observed or such)
then it will be merged to master eventually, later.

I am not able to find the documentation for the workflow right now,
though it is partially covered in Documentation/SubmittingPatches.

Hey

[Financial Services]

Do you need a loan

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

[Harald Nordgren]

There have been no new comments for the last few days. I know Jeff
King will be away for the next two weeks, but should we still move
forward with this? The initial reactions to the idea seemed positive.

Best regards
Harald

On Mon, Apr 9, 2018 at 3:42 AM, Harald Nordgren
 wrote:
> From: Jeff King 
>
> We have a helper function to allocate ref_array_item
> structs, but it only takes a subset of the possible fields
> in the struct as initializers. We could have it accept an
> argument for _every_ field, but that becomes a pain for the
> fields which some callers don't want to set initially.
>
> Instead, let's be explicit that it takes only the minimum
> required to create the ref, and that callers should then
> fill in the rest themselves.
>
> Signed-off-by: Jeff King 
> Signed-off-by: Harald Nordgren 
> ---
>  ref-filter.c | 18 +++---
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ade97a848..c1c3cc948 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct 
> oid_array *points_at,
> return NULL;
>  }
>
> -/* Allocate space for a new ref_array_item and copy the objectname and flag 
> to it */
> +/*
> + * Allocate space for a new ref_array_item and copy the name and oid to it.
> + *
> + * Callers can then fill in other struct members at their leisure.
> + */
>  static struct ref_array_item *new_ref_array_item(const char *refname,
> -const struct object_id *oid,
> -int flag)
> +const struct object_id *oid)
>  {
> struct ref_array_item *ref;
> +
> FLEX_ALLOC_STR(ref, refname, refname);
> oidcpy(>objectname, oid);
> -   ref->flag = flag;
>
> return ref;
>  }
> @@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, 
> const struct object_id *oid,
>  * to do its job and the resulting list may yet to be pruned
>  * by maxcount logic.
>  */
> -   ref = new_ref_array_item(refname, oid, flag);
> +   ref = new_ref_array_item(refname, oid);
> ref->commit = commit;
> +   ref->flag = flag;
> +   ref->kind = kind;
>
> REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
> ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
> -   ref->kind = kind;
> return 0;
>  }
>
> @@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct 
> object_id *oid,
>   const struct ref_format *format)
>  {
> struct ref_array_item *ref_item;
> -   ref_item = new_ref_array_item(name, oid, 0);
> +   ref_item = new_ref_array_item(name, oid);
> ref_item->kind = ref_kind_from_refname(name);
> show_ref_array_item(ref_item, format);
> free_array_item(ref_item);
> --
> 2.14.3 (Apple Git-98)
>

Re: Is offloading to GPU a worthwhile feature?

[Jakub Narebski]

Konstantin Ryabitsev  writes:
> On 04/08/18 09:59, Jakub Narebski wrote:

>>> This is an entirely idle pondering kind of question, but I wanted to
>>> ask. I recently discovered that some edge providers are starting to
>>> offer systems with GPU cards in them -- primarily for clients that need
>>> to provide streaming video content, I guess. As someone who needs to run
>>> a distributed network of edge nodes for a fairly popular git server, I
>>> wondered if git could at all benefit from utilizing a GPU card for
>>> something like delta calculations or compression offload, or if benefits
>>> would be negligible.
>> 
>> The problem is that you need to transfer the data from the main memory
>> (host memory) geared towards low-latency thanks to cache hierarchy, to
>> the GPU memory (device memory) geared towards bandwidth and parallel
>> access, and back again.  So to make sense the time for copying data plus
>> the time to perform calculations on GPU (and not all kinds of
>> computations can be speed up on GPU -- you need fine-grained massively
>> data-parallel task) must be less than time to perform calculations on
>> CPU (with multi-threading).
>
> Would something like this be well-suited for tasks like routine fsck,
> repacking and bitmap generation? That's the kind of workloads I was
> imagining it would be most well-suited for.

All of those, I think, would need to use some graph algorithms.  While
there are here ready graph libraries on GPU (like nVidia's nvGRAPH),
graphs are irregular structures not that well souted to the SIMD type of
parallelism that GPU is best for.

I also wonder if the amound of memory on GPU would be enough (and if
not, would be it possible to perform calculations in batches).

>> Also you would need to keep non-GPU and GPGPU code in sync.  Some parts
>> of code do not change much; and there also solutions to generate dual
>> code from one source.
>> 
>> Still, it might be good idea,
>
> I'm still totally the wrong person to be implementing this, but I do
> have access to Packet.net's edge systems which carry powerful GPUs for
> projects that might be needing these for video streaming services. It
> seems a shame to have them sitting idle if I can offload some of the
> RAM- and CPU-hungry tasks like repacking to be running there.

Happily, GPGPU programming (in CUDA C mainly, which limits use to nVidia
hardware) is one of my areas if interests...

Best regards,
--
Jakub Narębski

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:

> Hi Sergey,
>
> On Wed, 11 Apr 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  writes:
>> 
>> [...]
>> 
>> > We disallow '#' as label because that character will be used as
>> > separator in the upcoming `merge` command.
>> 
>> Please consider to use # not only in `merge` and `reset`, but in the
>> rest of the commands as well, to unify this new syntax. I.e., right now
>> it seems to be:
>> 
>> pick  abcd A commit message
>> merge beaf # B commit message
>> 
>> I suggest to turn it to:
>> 
>> pick  abcd # A commit message
>> merge beaf # B commit message
>
> First of all, that alignment of pick's and merge's first arguments?

As if it has anything to do with the topic of the issue!

Just a nice look. Let it be:

pick abcd # A commit message
merge beaf # B commit message

if it's that essential indeed.

> That does not exist. If you want aligned arguments, you have to use the
> rebase.abbreviateCommands feature.

It's changing the subject.

> Second: this change would break backwards-compatibility. For almost eleven
> years, we generated `pick abcdef0123 A commit message`.

I thought we already agreed that you have no backward compatibility
issues with this new feature, as it's a new feature, complete re-design,
as you put it yourself:

"This design flaw cannot be fixed. Not without a complete re-design, at
least. This patch series offers such a re-design."

At least could you please answer plain yes/no to this simple question: is
this feature a complete re-design or not? yes/no, please!

> Even if there are no scripts that rely on this form, power users have
> gotten used to it, and I can tell you from experience how unsettling
> even minor visual changes are in everyday operations.
> In short: no, we cannot do that.

You can do that, provided it's complete re-design indeed. You don't wish
to, but you can. Nothing will break and things will be at least a little
bit cleaner.

Each directive having its own dedicated syntax... gosh! No luck getting
syntax description, I'm afraid.

> Just like your proposal to conflate the `merge` and `pick` commands

There was never such proposal. The proposal was not to introduce new
`merge` command when there is already `pick` that could simply be
extended to pick any commit, whatever number of parents it happens to
have.

But provided you decline to even put a # before the commit message...
that proposal is simply a pie in the sky.

> for some perception of consistency: The user experience is more
> important than individual persons' sense of elegance (that might not
> even be shared with the majority).

It's about consistency indeed. Consistent handling of commits is
essential. Consistency is one of the things that bring positive user
experience. You disagree?

Besides, it was bad user experience that forced you to re-design, isn't
it? I'm afraid you miss good opportunity to fix some of your former
mistakes and you make some new. As the discussion goes, it seems you'd
never admit it, the design is set in stone, and my attempts are in fact
pointless.

Overall, I hereby withdraw all my pending suggestions to improve this
patch series.

-- Sergey

Re: [Git] recursive merge on 'master' severely broken?

[Elijah Newren]

On Wed, Apr 11, 2018 at 12:19 AM, Junio C Hamano  wrote:
> It appears that a topic recently graduated to 'master' introduces a
> severe regression to "git merge" when it merges a side branch that
> renames paths while the trunk has further updates.
>
> The symptom can easily be seen by trying to recreate the merge I
> made at the tip of 'pu' 29dea678 ("Merge branch
> 'sb/filenames-with-dashes' into pu", 2018-04-11) that I'll be.
> pushing out shortly.  The side branch renames a file exec_cmd.h to
> exec-cmd.h (an underscore changed to a dash) without changing any
> contents, while the branch being merged to has made some changes to
> the contents while keeping the original pathname.
>
> A clean automerged result should leave the identical contents from
> HEAD:exec_cmd.h in :exec-cmd.h in the index, which is what happens
> when using Git v2.17.0 proper, but with today's master', there are
> content changes that cannot be explained--the merge is simply broken
> and worse yet, the command pretends that everything went well and
> merged cleanly in that path.  Overly clever tool that behaves in a
> buggy and unexplainable way is bad enough, doing so silently is
> unexcusable.

I agree, that is _really_ bad.  My sincerest apologies.  I'll dig into it.

Targeted Global B2B Companies Emails List

[larry . clinton]

Hi,

Did you get a chance to review my below email? Kindly let me know your  
requirements and I will get back with detailed information on the same.


Warm Regards,
Larry Clinton

Re: [PATCH v6 15/15] rebase -i --rebase-merges: add a section to the man page

[Phillip Wood]

On 10/04/18 13:30, Johannes Schindelin wrote:

Firstly let me say that I think expanding the documentation and having 
an example is an excellent idea.



+
+
+label onto
+
+# Branch: refactor-button
+reset onto
+pick 123456 Extract a generic Button class from the DownloadButton one
+pick 654321 Use the Button class for all buttons
+label refactor-button
+
+# Branch: report-a-bug
+reset refactor-button # Use the Button class for all buttons
+pick abcdef Add the feedback button
+label report-a-bug
+
+reset onto
+merge -C a1b2c3 refactor-button # Merge 'refactor-button'
+merge -C 6f5e4d report-a-bug # Merge 'report-a-bug'
+
+
+In contrast to a regular interactive rebase, there are `label`, `reset` and
+`merge` commands in addition to `pick` ones.
+
+The `label` command puts a label to whatever will be the current


s/puts a label to/associates a label with/ would be clearer I think. 
Maybe s/whatever will be the current revision/the current HEAD/ an well?



+revision when that command is executed. Internally, these labels are
+worktree-local refs that will be deleted when the rebase finishes or
+when it is aborted.


I agree they should be deleted when the rebase is aborted but I cannot 
see any changes to git-rebase.sh to make that happen. I think they 
should also be deleted by 'rebase --quit'.



That way, rebase operations in multiple worktrees
+linked to the same repository do not interfere with one another.
+
+The `reset` command is essentially a `git reset --hard` to the specified
+revision (typically a previously-labeled one).


s/labeled/labelled/

I think it would be worthwhile to point out that unlike the other 
commands this will not preserve untracked files. Maybe something like
"Note that unlike the `pick` or `merge` commands or initial checkout 
when the rebase starts the `reset` command will overwrite any untracked 
files."



Best Wishes

Phillip

Re: [PATCH 6/8] gpg-interface: find the last gpg signature line

[Ben Toews]

On Tue, Apr 10, 2018 at 4:17 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>>> That test was fixed a week ago:
>>> https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd
>>
>> Well, you cannot expect any reviewer to know about a change that has
>> never been sent to the list and has never been part of even 'pu'
>> branch, no matter how old such a private "fix" is.
>>
>> What other unpublished things that are not even on 'pu' do these
>> patches depend on?
>
> Answering my own question after digging a bit more, now I know that
> a99d903 comes from the 'private' branch in peff/git/ repository
> hosted at GitHub [*1*].  The branch builds on 'next' by merging many
> private branches, and 'jk/non-pgp-signatures' branch has that commit.
>
> peff.git/private$ git log --oneline --boundary 0c8726318..7a526834e^2
> c9ce7c5b7 gpg-interface: handle multiple signing tools
> 914951682 gpg-interface: handle bool user.signingkey
> 1f2ea84b3 gpg-interface: return signature type from parse_signature()
> 6d2ce6547 gpg-interface: prepare for parsing arbitrary PEM blocks
> fb1d173db gpg-interface: find the last gpg signature line
> 6bc4e7e17 gpg-interface: extract gpg line matching helper
> 4f883ac49 gpg-interface: fix const-correctness of "eol" pointer
> ae6529fdb gpg-interface: use size_t for signature buffer size
> 1bca4296b gpg-interface: modernize function declarations
> a99d903f2 t7004: fix mistaken tag name
> - 468165c1d Git 2.17
>
> It seems to me that Peff did the t7004 change as the earliest
> preliminary step of the series, but it caused confusion when it was
> not sent as part of the series by mistake.  Judging from the shape
> of the topic, I do not think this topic has any other hidden
> dependencies as it builds directly on top of v2.17.0.
>
> For those who are reading and reviewing from the sideline, I've
> attached that missing 0.9/8 patch at the end of this message.

Sorry for the confusion Junio. I'm not used to the mailing list
workflow and it seems that I missed a patch. I'll make sure to include
that patch in v2.

>
> [Footnote]
>
> *1* I do not know if it deserves to be called a bug, but it
> certainly is an anomaly GitHub exhibits that a99d903f can be
> viewed at https://github.com/git/git/commit/a99d903f..., as if
> it is part of the official git/git history, when it only exists
> in a fork of that repository.  I can understand why it happens
> but it certainly is unexpected.
>
> -- >8 --
> From: Jeff King 
> Date: Tue, 3 Apr 2018 17:10:30 -0400
> Subject: [PATCH 0.9/8] t7004: fix mistaken tag name
>
> We have a series of tests which create signed tags with
> various properties, but one test accidentally verifies a tag
> from much earlier in the series.
> ---
>  t/t7004-tag.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 2aac77af7..ee093b393 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1056,7 +1056,7 @@ test_expect_success GPG \
> git tag -s -F sigblanknonlfile blanknonlfile-signed-tag &&
> get_tag_msg blanknonlfile-signed-tag >actual &&
> test_cmp expect actual &&
> -   git tag -v signed-tag
> +   git tag -v blanknonlfile-signed-tag
>  '
>
>  # messages with commented lines for signed tags:
> --
> 2.17.0-140-g0b0cc9f867
>



-- 
-Ben Toews

Re: Great Investment Offer

[Gagum Melvin Sikze Kakha]

Hello

In my search for a business partner i got your contact in google 
search. My client is willing to invest $10 Million to $500 
million but my client said he need a trusted partner who he can 
have a meeting at the point of releasing his funds. 

I told my client that you have a good profile with your company 
which i got details about you on my search on google lookup. Can 
we trust you. 

Can we make a plan for a long term business relationship.

Please reply. 

Regards,
Gagum Melvin Sikze Kakha
Tel: 703197576

Re: [PATCH v2 06/10] commit.c: use generation to halt paint walk

[Derrick Stolee]

On 4/10/2018 11:02 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


@@ -800,17 +810,26 @@ static struct commit_list *paint_down_to_common(struct 
commit *one, int n, struc
return result;
}
prio_queue_put(, one);
+   if (one->generation < min_nonstale_gen)
+   min_nonstale_gen = one->generation;
  
  	for (i = 0; i < n; i++) {

twos[i]->object.flags |= PARENT2;
prio_queue_put(, twos[i]);
+   if (twos[i]->generation < min_nonstale_gen)
+   min_nonstale_gen = twos[i]->generation;
}
  
-	while (queue_has_nonstale()) {

+   while (queue_has_nonstale(, min_nonstale_gen)) {
struct commit *commit = prio_queue_get();
struct commit_list *parents;
int flags;
  
+		if (commit->generation > last_gen)

+   BUG("bad generation skip");
+
+   last_gen = commit->generation;
+
flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
if (flags == (PARENT1 | PARENT2)) {
if (!(commit->object.flags & RESULT)) {
@@ -830,6 +849,10 @@ static struct commit_list *paint_down_to_common(struct 
commit *one, int n, struc
return NULL;
p->object.flags |= flags;

Hmph.  Can a commit that used to be not stale (and contributed to
the current value of min_nonstale_gen) become stale here by getting
visited twice, invalidating the value in min_nonstale_gen?


min_nonstale_gen can be "wrong" in the way you say, but fits the 
definition from the commit message:


"To properly take advantage of this condition, track the minimum 
generation number of a commit that **enters the queue** with nonstale 
status." (Emphasis added)


You make an excellent point about how this can be problematic. I was 
confused by the lack of clear performance benefits here, but I think 
that whatever benefits making queue_has_nonstale() be O(1) were removed 
by walking more commits than necessary.


Consider the following commit graph, where M is a parent of both A and 
B, S is a parent of M and B, and there is a large set of commits 
reachable from M with generation number larger than gen(S).


A    B
| __/|
|/   |
M    |
|\   |
. |  |
. |  |
. |_/
|/
S

Between A and B, the true merge base is M. Anything reachable from M is 
marked as stale. When S is added to the queue, it is only reachable from 
B, so it is non-stale. However, it is marked stale after M is walked. 
The old code would detect this as a termination condition, but the new 
code would not.


I think this data shape is actually common (not exactly, as it may be 
that some ancestor of M provides a second path to S) especially in the 
world of pull requests and users merging master into their topic branches.


I'll remove this commit in the next version, but use the new prototype 
for queue_has_nonstale() in "commit: add short-circuit to 
paint_down_to_common()" using the given 'min_generation' instead of 
'min_nonstale_gen'.


Thanks,
-Stolee

Re: [PATCH v6 00/15] rebase -i: offer to recreate commit topology

[Sergey Organov]

Johannes Schindelin  writes:

> Hi Sergey,
>
> On Wed, 11 Apr 2018, Sergey Organov wrote:
>
>> Johannes Schindelin  writes:
>>
>> > On Tue, 10 Apr 2018, Sergey Organov wrote:
>> >
>> >> Johannes Schindelin  writes:
>> >>
>> >> > Once upon a time, I dreamt of an interactive rebase that would not
>> >> > flatten branch structure, but instead recreate the commit topology
>> >> > faithfully.
>> >>
>> >> [...]
>> >>
>> >> > Think of --rebase-merges as "--preserve-merges done right".
>> >>
>> >> Both option names seem to miss the primary point of the mode of
>> >> operation that you've formulated in the first sentence. I suggest to
>> >> rather call the new option in accordance to your description, say,
>> >> --no-flatten, --keep-topology, or --preserve-shape.
>> >
>> > A very quick A/B test shows that neither --no-flatten nor --keep-topology
>> > and certainly not --preserve-shape conveys to Git users what those options
>> > are supposed to do.
>>
>> In fact, my preference would be --[no-]flatten, exactly because the
>> default mode of rebase operation flattens the history, and thus what I'm
>> talking about is:
>>
>> git rebase --no-flatten
>
> And this is the option out of the four that fared *worst* in the A/B
> testing. Not even experts in Git internals were able to figure out what
> the heck you are talking about.

It was you who introduced the "flatten" term, not me. I took it from
your descriptions.

So they are able to make sense of your own:

>>> Once upon a time, I dreamt of an interactive rebase that would not
>>> flatten branch structure, but instead recreate the commit topology
>>> faithfully.

Yet they can't get:

--no-flatten::
Instead of flattening branch structure, recreate the commit
topology faithfully

Are you kidding?

Well, suppose for a moment that nobody could even guess what "flatten"
means indeed. Then are you willing to remove the "flatten" from both the
description of our patch series and from the proposed patch to the Git
manual:

-r::
--rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
Rebase merge commits instead of _flattening_ the history by replaying
merges.

???

>
> Now, you can beat that dead horse until it is pulp. Your choice. I'd
> rather go on to more interesting things, because as far as I am concerned,
> the naming issue has been settled, with you being the only person in
> disfavor of --rebase-merges.

It was rather --recreate-merges just a few weeks ago, and I've seen
nobody actually commented either in favor or against the
--rebase-merges.

git rebase --rebase-merges

_is_ plain simple ugly.

>
> What you *could* do is finally take your RFC to the test. Run it with the
> concrete example I showed you in
> https://public-inbox.org/git/nycvar.qro.7.76.6.1803261405170...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/
>
> It is high time that you demonstrated on this concrete case study how your
> proposed solution performs. And then tally that up with Phillip's
> strategy.

What you could do is to stop shifting the subject of discussion.

The RFC v2 and Phillip's strategy are essentially the same, as has been
already shown multiple times, both theoretically and by testing. Ask
Bugga for details.

One way or another, this doesn't make

git rebase --rebase-merges

even a bit less ugly.

-- Sergey

Re: [PATCH v2 04/10] commit-graph: compute generation numbers

[Derrick Stolee]

On 4/10/2018 10:51 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


+   if ((*list)->generation != GENERATION_NUMBER_INFINITY) {
+   if ((*list)->generation > GENERATION_NUMBER_MAX)
+   die("generation number %u is too large to store in 
commit-graph",
+   (*list)->generation);
+   packedDate[0] |= htonl((*list)->generation << 2);
+   }


How serious do we want this feature to be?  On one extreme, we could
be irresponsible and say it will be a problem for our descendants in
the future if their repositories have more than billion pearls on a
single strand, and the above certainly is a reasonable way to punt.
Those who actually encounter the problem will notice by Git dying
somewhere rather deep in the callchain.

Or we could say Git actually does support a history that is
arbitrarily long, even though such a deep portion of history will
not benefit from having generation numbers in commit-graph.

I've been assuming that our stance is the latter and that is why I
made noises about overflowing 30-bit generation field in my review
of the previous step.

In case we want to do the "we know this is very large, but we do not
know the exact value", we may actually want a mode where we can
pretend that GENERATION_NUMBER_MAX is set to quite low (say 256) and
make sure that the code to handle overflow behaves sensibly.


I agree. I wonder how we can effectively expose this value into a test. 
It's probably not sufficient to manually test using compiler flags ("-D 
GENERATION_NUMBER_MAX=8").





+   for (i = 0; i < nr_commits; i++) {
+   if (commits[i]->generation != GENERATION_NUMBER_INFINITY &&
+   commits[i]->generation != GENERATION_NUMBER_ZERO)
+   continue;
+
+   commit_list_insert(commits[i], );
+   while (list) {
+...
+   }
+   }

So we go over the list of commits just _once_ and make sure each of
them gets the generation assigned correctly by (conceptually
recursively but iteratively in implementation by using a commit
list) making sure that all its parents have generation assigned and
compute the generation for the commit, before moving to the next
one.  Which sounds correct.


Yes, we compute the generation number of a commit exactly once. We use 
the list as a stack so we do not have recursion limits during our 
depth-first search (DFS). We rely on the object cache to ensure we store 
the computed generation numbers, and computed generation numbers provide 
termination conditions to the DFS.


Thanks,
-Stolee

Re: [PATCH v2 03/10] commit: add generation number to struct commmit

[Derrick Stolee]

On 4/10/2018 10:31 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


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

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

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

GENERATION_NUMBER_ININITY 0x
GENERATION_NUMBER_ZERO 0

The first (_INFINITY) means the generation number has not been loaded or
computed. The second (_ZERO) means the generation number was loaded
from a commit graph file that was stored before generation numbers
were computed.

Should it also be possible for a caller to tell if a given commit
has too deep a history, i.e. we do not know its generation number
exactly, but we know it is larger than 1<<30?

It seems that we only have a 30-bit field in the file, so wouldn't
we need a special value defined in (e.g. "0") so that we can tell
that the commit has such a large generation number?  E.g.


+   item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;

if (!item->generation)
item->generation = GENERATION_NUMBER_OVERFLOW;

when we read it from the file?

We obviously need to do something similar when assigning a
generation number to a child commit, perhaps like

#define GENERATION_NUMBER_OVERFLOW (GENERATION_NUMBER_MAX + 1)

commit->generation = 1; /* assume no parent */
for (p = commit->parents; p; p++) {
uint32_t gen = p->item->generation + 1;

if (gen >= GENERATION_NUMBER_OVERFLOW) {
commit->generation = GENERATION_NUMBER_OVERFLOW;
break;
} else if (commit->generation < gen)
commit->generation = gen;
}
 
or something?  And then on the writing side you'd encode too large a

generation as '0'.


You raise a very good point. How about we do a slightly different 
arrangement for these overflow commits?


Instead of storing the commits in the commit-graph file as "0" (which 
currently means "written by a version of git that did not compute 
generation numbers") we could let GENERATION_NUMBER_MAX be the maximum 
generation of a commit in the commit-graph, and if a commit would have 
larger generation, we collapse it down to that value.


It slightly complicates the diagram I made in 
Documentation/technical/commit-graph.txt, but it was already a bit of a 
simplification. Here is an updated diagram, but likely we will want to 
limit discussion of the special-case GENERATION_NUMBER_MAX to the prose, 
since it is not a practical situation at the moment.


    +-+
    | GENERATION_NUMBER_INFINITY = 0x |
    +-+
  |    |    |  ^
  |    |    |  |
  |    |    +--+
  |    | [gen(A) = gen(B)]
  |    V
  |  ++
      |  | GENERATION_NUMBER_MAX = 0x3FFF |
  |  ++
  |    |    |  ^
  |    |    |  |
  |    |    +--+
  |    | [gen(A) = gen(B)]
  V    V
    +-+
    | 0 < commit->generation < 0x3FFF |
    +-+
        |    |  ^
        |    |  |
        |    +--+
        |    [gen(A) > gen(B)]
        V
    +-+
    | GENERATION_NUMBER_ZERO = 0  |
    +-+
             |  ^
             |  |
             +--+
         [gen(A) = gen(B)]

Thanks,
-Stolee

Re: [PATCH v2 02/10] merge: check config before loading commits

[Derrick Stolee]

On 4/10/2018 10:12 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


diff --git a/builtin/merge.c b/builtin/merge.c
index ee050a47f3..20897f8223 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1183,13 +1183,14 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
branch = branch_to_free = resolve_refdup("HEAD", 0, _oid, NULL);
if (branch)
skip_prefix(branch, "refs/heads/", );
+   init_diff_ui_defaults();
+   git_config(git_merge_config, NULL);
+
if (!branch || is_null_oid(_oid))
head_commit = NULL;
else
head_commit = lookup_commit_or_die(_oid, "HEAD");
  
-	init_diff_ui_defaults();

-   git_config(git_merge_config, NULL);

Wow, that's tricky.  git_merge_config() wants to know which "branch"
we are on, and this place is as early as we can move the call to
without breaking things.  Is this to allow parse_object() called
in lookup_commit_reference_gently() to know if we can rely on the
data cached in the commit-graph data?


When I saw the bug on my machine, I tracked the issue down to a call to 
parse_commit_in_graph() that skipped the graph check since 
core_commit_graph was not set. The call stack from this call is as follows:


* lookup_commit_or_die()
* lookup_commit_reference()
* lookup_commit_reference_gently()
* parse_object()
* parse_object_buffer()
* parse_commit_in_graph() [as introduced in PATCH 01/10]




Move the config load to be between the initialization of 'branch'
and the commit lookup. Also add a test to t5318-commit-graph.sh
that exercises this code path to prevent a regression.

It is not clear to me how a successful merge of commits/8
demonstrates that reading the config earlier than before is
regression free.


I didn't want to introduce commits in an order that led to a commit 
failing tests, but if you drop the change to builtin/merge.c from this 
series, the tip commit will fail this test with "BUG: bad generation skip".


The reason for this failure is that commits/5 is loaded from HEAD from 
the object database, so its generation is marked as 
GENERATION_NUMBER_INFINITY, and the commit is marked as parsed. Later, 
the commit at merges/3 is loaded from the graph with generation 4. This 
triggers the BUG statement in paint_down_to_common(). That is why it is 
important to check a fast-forward merge.


In the 'graph_git_behavior' steps of t5318-commit-graph.sh, we were 
already testing 'git merge-base' to check the commit walk logic.


Thanks,
-Stolee

Great News!!!

[Amnesty International]

You Won  $950,500.00 USD on Amnesty International 
Kenya online E-mail Promotion. For more details 
about your prize claims, file with the following;

Names: 
Country: 
Tel:

Regards,
Mr. David Ford

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Johannes Schindelin]

Hi Sergey,

On Wed, 11 Apr 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> 
> [...]
> 
> > We disallow '#' as label because that character will be used as
> > separator in the upcoming `merge` command.
> 
> Please consider to use # not only in `merge` and `reset`, but in the
> rest of the commands as well, to unify this new syntax. I.e., right now
> it seems to be:
> 
> pick  abcd A commit message
> merge beaf # B commit message
> 
> I suggest to turn it to:
> 
> pick  abcd # A commit message
> merge beaf # B commit message

First of all, that alignment of pick's and merge's first arguments? That
does not exist. If you want aligned arguments, you have to use the
rebase.abbreviateCommands feature.

Second: this change would break backwards-compatibility. For almost eleven
years, we generated `pick abcdef0123 A commit message`. Even if there are
no scripts that rely on this form, power users have gotten used to it, and
I can tell you from experience how unsettling even minor visual changes
are in everyday operations.

In short: no, we cannot do that. Just like your proposal to conflate the
`merge` and `pick` commands for some perception of consistency: The user
experience is more important than individual persons' sense of elegance
(that might not even be shared with the majority).

Ciao,
Johannes

Re: [PATCH v6 00/15] rebase -i: offer to recreate commit topology

[Johannes Schindelin]

Hi Sergey,

On Wed, 11 Apr 2018, Sergey Organov wrote:

> Johannes Schindelin  writes:
> 
> > On Tue, 10 Apr 2018, Sergey Organov wrote:
> >
> >> Johannes Schindelin  writes:
> >> 
> >> > Once upon a time, I dreamt of an interactive rebase that would not
> >> > flatten branch structure, but instead recreate the commit topology
> >> > faithfully.
> >> 
> >> [...]
> >> 
> >> > Think of --rebase-merges as "--preserve-merges done right".
> >> 
> >> Both option names seem to miss the primary point of the mode of
> >> operation that you've formulated in the first sentence. I suggest to
> >> rather call the new option in accordance to your description, say,
> >> --no-flatten, --keep-topology, or --preserve-shape.
> >
> > A very quick A/B test shows that neither --no-flatten nor --keep-topology
> > and certainly not --preserve-shape conveys to Git users what those options
> > are supposed to do.
> 
> In fact, my preference would be --[no-]flatten, exactly because the
> default mode of rebase operation flattens the history, and thus what I'm
> talking about is:
> 
> git rebase --no-flatten

And this is the option out of the four that fared *worst* in the A/B
testing. Not even experts in Git internals were able to figure out what
the heck you are talking about.

Now, you can beat that dead horse until it is pulp. Your choice. I'd
rather go on to more interesting things, because as far as I am concerned,
the naming issue has been settled, with you being the only person in
disfavor of --rebase-merges.

What you *could* do is finally take your RFC to the test. Run it with the
concrete example I showed you in
https://public-inbox.org/git/nycvar.qro.7.76.6.1803261405170...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/

It is high time that you demonstrated on this concrete case study how your
proposed solution performs. And then tally that up with Phillip's
strategy.

Ciao,
Johannes

Re: What's cooking in git.git (Apr 2018, #01; Mon, 9)

[Phillip Wood]

On 09/04/18 11:21, Junio C Hamano wrote:

* pw/add-p-select (2018-03-16) 3 commits
   (merged to 'next' on 2018-03-30 at eae69f5ded)
  + add -p: optimize line selection for short hunks
  + add -p: allow line selection to be inverted
  + add -p: select individual hunk lines

  "git add -p" interactive interface learned to let users choose
  individual added/removed lines to be used in the operation, instead
  of accepting or rejecting a whole hunk.

  Will kick back to 'pu'.

  There was a brief discussion about this topic not doing as good a
  job as it is advertised as---has it been resolved, or do we want to
  run with what we have for now?
  cf. <878ta8vyqe@evledraar.gmail.com>


I've been working on a re-roll that has handles modified lines better. 
I'm not sure what to do about the cases where it cannot pair up the 
insertions and deletions automatically - I think I'll clean up what I've 
got, post it and see where the discussion goes from there.


Thanks

Phillip

Re: [PATCH 8/8] gpg-interface: handle alternative signature types

[SZEDER Gábor]

> diff --git a/gpg-interface.h b/gpg-interface.h
> index a5e6517ae6..cee0dfe401 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -23,15 +23,27 @@ struct signature_check {
>   char *key;
>  };
>  
> +struct signing_tool {
> + char *name;
> + char *program;
> + struct string_list pemtype;
> + struct signing_tool *next;
> +};
> +
>  void signature_check_clear(struct signature_check *sigc);
>  
>  /*
> - * Look at GPG signed content (e.g. a signed tag object), whose
> + * Look for signed content (e.g. a signed tag object), whose
>   * payload is followed by a detached signature on it.  Return the
>   * offset where the embedded detached signature begins, or the end of
>   * the data when there is no such signature.
> + *
> + * If out_tool is non-NULL and a signature is found, it will be
> + * pointed at the signing_tool that corresponds to the found
> + * signature type.
>   */
> -size_t parse_signature(const char *buf, size_t size);
> +size_t parse_signature(const char *buf, unsigned long size,
> +const struct signing_tool **out_tool);

This hunk changes the type of the 'size' argument from size_t to
unsigned long, but leaves the function's signature in
'gpg-interface.c' unchanged.  This breaks the build on 32 bit systems
(and Windows?), where unsigned long is only 32 bits, and I presume is
unintended, as it goes against the earlier patch 3/8 "gpg-interface:
use size_t for signature buffer size".

Great News!!!

[Amnesty International]

You Won  $950,500.00 USD on Amnesty International 
Kenya online E-mail Promotion. For more details 
about your prize claims, file with the following;

Names: 
Country: 
Tel:

Regards,
Mr. David Ford

Re: [Git] recursive merge on 'master' severely broken?

[Junio C Hamano]

Junio C Hamano  writes:

> Another interim report.
>
> I do not yet know which one of the ~30 individual commmits is the
> real culprit, ...

It bisects down to c5b761fb ("merge-recursive: ensure we write
updates for directory-renamed file", 2018-02-14); given that a part
of the symptom I saw was a few messages like these:

...
CONFLICT (content): Merge conflict in upload-pack.c
error: addinfo_cache failed for path 'sha1-name.c'
error: addinfo_cache failed for path 'sha1-file.c'
Auto-merging sequencer.c
...

and the patch does change code around the callsites to
add_cacheinfo(), it does look like a plausible culprit.

Re: Is support for 10.8 dropped?

[Eric Sunshine]

On Wed, Apr 11, 2018 at 12:28 AM, Igor Korot  wrote:
>>> Is there a way to check for OpenSSL presence?
>>
>> Not sure what you're asking. Are you asking how to determine if you
>> already have OpenSSL built on your machine?
>
> Yes, that's what I was asking...

Easiest way to determine it is to try to compile Git without setting NO_OPENSSL.

> This is what I got trying to do just "make":
>
> MyMac:git-2.17.0 igorkorot$ make
> * new build flags
> CC credential-store.o
> In file included from credential-store.c:1:
> In file included from ./cache.h:9:
> ./gettext.h:17:11: fatal error: 'libintl.h' file not found
> #   include 
> ^
> 1 error generated.

This is because you don't have gettext installed. You should be able
to set NO_GETTEXT to work around this.

> And I am also confused. Which file am I suppose to modify here?
>
> MyMac:git-2.17.0 igorkorot$ ls -la conf*
> -rwxr-xr-x@ 1 igorkorot  staff  74461 Apr  2 10:13 config.c
> -rwxr-xr-x@ 1 igorkorot  staff   9888 Apr  2 10:13 config.h
> -rwxr-xr-x@ 1 igorkorot  staff540 Apr  2 10:13 config.mak.in
> -rwxr-xr-x@ 1 igorkorot  staff  16940 Apr  2 10:13 config.mak.uname
> -rwxr-xr-x@ 1 igorkorot  staff  37509 Apr  2 10:13 configure.ac
> -rw-r--r--  1 igorkorot  staff  37500 Apr  8 18:52 configure.ac+

You should create a new file named "config.mak". For instance, the
content of your config.mak file might be:

NO_GETTEXT=Yes

Re: [Git] recursive merge on 'master' severely broken?

[Junio C Hamano]

Junio C Hamano  writes:

> It appears that a topic recently graduated to 'master' introduces a
> severe regression to "git merge" when it merges a side branch that
> renames paths while the trunk has further updates.
> ...
> I suspect that the culprit is the merge e4bb62fa ("Merge branch
> 'en/rename-directory-detection'", 2018-04-10), and I am planning to
> revert it out of 'master' as soon as possible, but it is already
> buried deep in other topics, so I may not be able to get to it until
> later this week.  Sorry about that.

An interim report.

It indeed is that merge with the topic.  I rebuilt an equivalent of
'master' without that topic on top of v2.17.0 and tried the same
merge of sb/filenames-with-dashes fd055186 ("replace_object.c:
rename to use dash in file name", 2018-04-10) topic into pu^
35f7512e ("Merge branch 'fg/completion-external' into pu",
2018-04-11), and that version of Git does not exhibit the problem.

I do not yet know which one of the ~30 individual commmits is the
real culprit, but in the meantime, I'll revert the merge out of
'master' and push the result out, so that real Git-using projects
won't get silent mismerges.  Those who run in-house version of Git
that is based on anything newer than the released version may want
to do the same.

Sorry for the crappy 'master' X-<.

Re: core.fsmonitor always perform rescans

[Tatsuyuki Ishi]

2018-04-10 23:34 GMT+09:00 Ben Peart :
> As a performance optimization, the fsmonitor code doesn't flag the index as
> dirty and force it to be written out with every command.  Can you try
> performing a git operation (add, rm, commit, etc) that will write out an
> updated index and see if that fixes the issue you're seeing?

Yeah, that resolves the issue. Though the repo I'm working on uses
submodules, so doing this in each of them isn't a easy work.

> I'm considering adding a special case to force the index to be written out
> the first time fsmonitor is invoked and am interested to know if this would
> have avoided the issue you are seeing.

Yes please. And maybe we should also flush the index when the script
returns '/',
which means all files?

[Git] recursive merge on 'master' severely broken?

[Junio C Hamano]

It appears that a topic recently graduated to 'master' introduces a
severe regression to "git merge" when it merges a side branch that
renames paths while the trunk has further updates.

The symptom can easily be seen by trying to recreate the merge I
made at the tip of 'pu' 29dea678 ("Merge branch
'sb/filenames-with-dashes' into pu", 2018-04-11) that I'll be.
pushing out shortly.  The side branch renames a file exec_cmd.h to
exec-cmd.h (an underscore changed to a dash) without changing any
contents, while the branch being merged to has made some changes to
the contents while keeping the original pathname.

A clean automerged result should leave the identical contents from
HEAD:exec_cmd.h in :exec-cmd.h in the index, which is what happens
when using Git v2.17.0 proper, but with today's master', there are
content changes that cannot be explained--the merge is simply broken
and worse yet, the command pretends that everything went well and
merged cleanly in that path.  Overly clever tool that behaves in a
buggy and unexplainable way is bad enough, doing so silently is
unexcusable.

I suspect that the culprit is the merge e4bb62fa ("Merge branch
'en/rename-directory-detection'", 2018-04-10), and I am planning to
revert it out of 'master' as soon as possible, but it is already
buried deep in other topics, so I may not be able to get to it until
later this week.  Sorry about that.

Re: [PATCH v8 0/5] RUNTIME_PREFIX relocatable Git

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 10 2018, Dan Jacques wrote:

> This is a minor update based on comments from the v6 series.
> I'm hoping this set is good to go!

This looks to me. Thanks!

> - Rebased on top of "master".

Probably useful for others to have the interdiff between this and
v7. Here it is:

$ git tbdiff origin/master b329fde5a6 2b1ed6a79a
1: 776c7d6083 = 1: 4943ec6581 Makefile: generate Perl header from template 
file
2: 0cded81572 ! 2: 29223250da Makefile: add Perl runtime prefix support
@@ -52,10 +52,9 @@

  prefix = $(HOME)
 @@
- 
  mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
  infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
-+gitexecdir_relative = $(patsubst $(prefix)/%,%,$(gitexecdir))
+ gitexecdir_relative = $(patsubst $(prefix)/%,%,$(gitexecdir))
 +localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))
  htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 +perllibdir_relative = $(patsubst $(prefix)/%,%,$(perllibdir))
@@ -68,7 +67,7 @@
  localedir_SQ = $(subst ','\'',$(localedir))
 +localedir_relative_SQ = $(subst ','\'',$(localedir_relative))
  gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
-+gitexecdir_relative_SQ = $(subst ','\'',$(gitexecdir_relative))
+ gitexecdir_relative_SQ = $(subst ','\'',$(gitexecdir_relative))
  template_dir_SQ = $(subst ','\'',$(template_dir))
  htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
  prefix_SQ = $(subst ','\'',$(prefix))
3: a0e41f5f41 ! 3: 5220baf80e exec_cmd: RUNTIME_PREFIX on some POSIX systems
@@ -109,6 +109,8 @@
 +  
git_setup_gettext();

+   initialize_the_repository();
+ 
attr_start();

 -  git_extract_argv0_path(argv[0]);
4: b25be6e56d = 4: 57dcc5203e exec_cmd: provide a new-style RUNTIME_PREFIX 
helper for Windows
5: b329fde5a6 = 5: 2b1ed6a79a mingw/msvc: use the new-style RUNTIME_PREFIX 
helper

Re: [PATCH v1 1/2] fsexcludes: add a programmatic way to exclude files from git's working directory traversal logic

[Junio C Hamano]

Ben Peart  writes:

> +void fsexcludes_free() {

Write this line like so:

void fsexcludes_free(void)
{

> +void fsexcludes_free();

void fsexcludes_free(void);

[PATCH v3 1/1] perl: fix installing modules from contrib

[Christian Hesse]

Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
removed a target that allowed Makefiles from contrib/ to get the correct
install path. This introduces a new target for main Makefile and fixes
installation for Mediawiki module.

v2: Pass prefix as that can have influence as well, add single quotes
for _SQ variant.
v3: Rename target, add to .PHONY.

Signed-off-by: Christian Hesse 
---
 Makefile   | 3 +++
 contrib/mw-to-git/Makefile | 5 +++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f18168725..5a06eddf2 100644
--- a/Makefile
+++ b/Makefile
@@ -2014,6 +2014,9 @@ GIT-PERL-DEFINES: FORCE
echo "$$FLAGS" >$@; \
fi
 
+.PHONY: say-perllibdir
+say-perllibdir:
+   @echo '$(perllibdir_SQ)'
 
 .PHONY: gitweb
 gitweb:
diff --git a/contrib/mw-to-git/Makefile b/contrib/mw-to-git/Makefile
index a4b6f7a2c..e301a5b4e 100644
--- a/contrib/mw-to-git/Makefile
+++ b/contrib/mw-to-git/Makefile
@@ -21,8 +21,9 @@ HERE=contrib/mw-to-git/
 INSTALL = install
 
 SCRIPT_PERL_FULL=$(patsubst %,$(HERE)/%,$(SCRIPT_PERL))
-INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/perl \
--s --no-print-directory instlibdir)
+INSTLIBDIR=$(shell $(MAKE) -C $(GIT_ROOT_DIR)/ \
+-s --no-print-directory prefix=$(prefix) \
+perllibdir=$(perllibdir) say-perllibdir)
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
 INSTLIBDIR_SQ = $(subst ','\'',$(INSTLIBDIR))
 

Re: [PATCH v8 0/5] RUNTIME_PREFIX relocatable Git

[Junio C Hamano]

Dan Jacques  writes:

> This is a minor update based on comments from the v6 series.
> I'm hoping this set is good to go!
>
> This patch set expands support for the RUNTIME_PREFIX configuration flag,
> currently only used on Windows builds, to include Linux, Darwin, and
> FreeBSD. When Git is built with RUNTIME_PREFIX enabled, it resolves its
> ancillary paths relative to the runtime location of its executable
> rather than hard-coding them at compile-time, allowing a Git
> installation to be deployed to a path other than the one in which it
> was built/installed.
>
> Note that RUNTIME_PREFIX is not currently used outside of Windows.
> This patch set should not have an impact on default Git builds.
>
> Previous threads:
> v1: https://public-inbox.org/git/20171116170523.28696-1-...@google.com/
> v2: https://public-inbox.org/git/20171119173141.4896-1-...@google.com/
> v3: https://public-inbox.org/git/20171127164055.93283-1-...@google.com/
> v4: https://public-inbox.org/git/20171129223807.91343-1-...@google.com/
> v5: https://public-inbox.org/git/20180108030239.92036-1-...@google.com/
> v5: https://public-inbox.org/git/20180108030239.92036-1-...@google.com/
> v6: https://public-inbox.org/git/20180319025046.58052-1-...@google.com/
> v7: https://public-inbox.org/git/20180325205120.17730-1-...@google.com/
>
> Changes in v8 from v7:
>
> - Add Johannes's Windows patch series to the end (see v7 thread).

Wonderful.  That gives me one less separate topic to worry about ;-)

> - Fix more typos and formatting nits.
> - Rebased on top of "master".

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Sergey Organov]

Hi Johannes,

Johannes Schindelin  writes:

[...]

> We disallow '#' as label because that character will be used as separator
> in the upcoming `merge` command.

Please consider to use # not only in `merge` and `reset`, but in the rest
of the commands as well, to unify this new syntax. I.e., right now it
seems to be:

pick  abcd A commit message
merge beaf # B commit message

I suggest to turn it to:

pick  abcd # A commit message
merge beaf # B commit message

So that the # is finally universally the start of comment.

While we are at this, I couldn't find any even semi-formal syntax
description of the entire todo list. Is there one already? Are you going
to provide one?

-- Sergey