[PATCH v2 10/37] help: rename 'new' variables

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 builtin/help.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index d3c8fc408..598867cfe 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -194,11 +194,11 @@ static void do_add_man_viewer_info(const char *name,
   size_t len,
   const char *value)
 {
-   struct man_viewer_info_list *new;
-   FLEX_ALLOC_MEM(new, name, name, len);
-   new->info = xstrdup(value);
-   new->next = man_viewer_info_list;
-   man_viewer_info_list = new;
+   struct man_viewer_info_list *new_man_viewer;
+   FLEX_ALLOC_MEM(new_man_viewer, name, name, len);
+   new_man_viewer->info = xstrdup(value);
+   new_man_viewer->next = man_viewer_info_list;
+   man_viewer_info_list = new_man_viewer;
 }
 
 static int add_man_viewer_path(const char *name,
-- 
2.16.1.291.g4437f3f132-goog

[PATCH v2 11/37] pack-redundant: rename 'new' variables

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 builtin/pack-redundant.c | 54 
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index aaa813632..991e1bb76 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -48,17 +48,17 @@ static inline void llist_item_put(struct llist_item *item)
 
 static inline struct llist_item *llist_item_get(void)
 {
-   struct llist_item *new;
+   struct llist_item *new_item;
if ( free_nodes ) {
-   new = free_nodes;
+   new_item = free_nodes;
free_nodes = free_nodes->next;
} else {
int i = 1;
-   ALLOC_ARRAY(new, BLKSIZE);
+   ALLOC_ARRAY(new_item, BLKSIZE);
for (; i < BLKSIZE; i++)
-   llist_item_put([i]);
+   llist_item_put(_item[i]);
}
-   return new;
+   return new_item;
 }
 
 static void llist_free(struct llist *list)
@@ -80,26 +80,26 @@ static inline void llist_init(struct llist **list)
 static struct llist * llist_copy(struct llist *list)
 {
struct llist *ret;
-   struct llist_item *new, *old, *prev;
+   struct llist_item *new_item, *old_item, *prev;
 
llist_init();
 
if ((ret->size = list->size) == 0)
return ret;
 
-   new = ret->front = llist_item_get();
-   new->sha1 = list->front->sha1;
+   new_item = ret->front = llist_item_get();
+   new_item->sha1 = list->front->sha1;
 
-   old = list->front->next;
-   while (old) {
-   prev = new;
-   new = llist_item_get();
-   prev->next = new;
-   new->sha1 = old->sha1;
-   old = old->next;
+   old_item = list->front->next;
+   while (old_item) {
+   prev = new_item;
+   new_item = llist_item_get();
+   prev->next = new_item;
+   new_item->sha1 = old_item->sha1;
+   old_item = old_item->next;
}
-   new->next = NULL;
-   ret->back = new;
+   new_item->next = NULL;
+   ret->back = new_item;
 
return ret;
 }
@@ -108,24 +108,24 @@ static inline struct llist_item *llist_insert(struct 
llist *list,
  struct llist_item *after,
   const unsigned char *sha1)
 {
-   struct llist_item *new = llist_item_get();
-   new->sha1 = sha1;
-   new->next = NULL;
+   struct llist_item *new_item = llist_item_get();
+   new_item->sha1 = sha1;
+   new_item->next = NULL;
 
if (after != NULL) {
-   new->next = after->next;
-   after->next = new;
+   new_item->next = after->next;
+   after->next = new_item;
if (after == list->back)
-   list->back = new;
+   list->back = new_item;
} else {/* insert in front */
if (list->size == 0)
-   list->back = new;
+   list->back = new_item;
else
-   new->next = list->front;
-   list->front = new;
+   new_item->next = list->front;
+   list->front = new_item;
}
list->size++;
-   return new;
+   return new_item;
 }
 
 static inline struct llist_item *llist_insert_back(struct llist *list,
-- 
2.16.1.291.g4437f3f132-goog

[PATCH v2 12/37] reflog: rename 'new' variables

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 builtin/reflog.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 223372531..ac3dcd7a5 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -289,20 +289,20 @@ static int should_expire_reflog_ent(struct object_id 
*ooid, struct object_id *no
const char *message, void *cb_data)
 {
struct expire_reflog_policy_cb *cb = cb_data;
-   struct commit *old, *new;
+   struct commit *old_commit, *new_commit;
 
if (timestamp < cb->cmd.expire_total)
return 1;
 
-   old = new = NULL;
+   old_commit = new_commit = NULL;
if (cb->cmd.stalefix &&
-   (!keep_entry(, ooid) || !keep_entry(, noid)))
+   (!keep_entry(_commit, ooid) || !keep_entry(_commit, noid)))
return 1;
 
if (timestamp < cb->cmd.expire_unreachable) {
if (cb->unreachable_expire_kind == UE_ALWAYS)
return 1;
-   if (unreachable(cb, old, ooid) || unreachable(cb, new, noid))
+   if (unreachable(cb, old_commit, ooid) || unreachable(cb, 
new_commit, noid))
return 1;
}
 
-- 
2.16.1.291.g4437f3f132-goog

[PATCH v2 03/37] blame: rename 'this' variables

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 blame.c | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/blame.c b/blame.c
index 2893f3c10..21c867664 100644
--- a/blame.c
+++ b/blame.c
@@ -998,28 +998,29 @@ unsigned blame_entry_score(struct blame_scoreboard *sb, 
struct blame_entry *e)
 }
 
 /*
- * best_so_far[] and this[] are both a split of an existing blame_entry
- * that passes blame to the parent.  Maintain best_so_far the best split
- * so far, by comparing this and best_so_far and copying this into
+ * best_so_far[] and potential[] are both a split of an existing blame_entry
+ * that passes blame to the parent.  Maintain best_so_far the best split so
+ * far, by comparing potential and best_so_far and copying potential into
  * bst_so_far as needed.
  */
 static void copy_split_if_better(struct blame_scoreboard *sb,
 struct blame_entry *best_so_far,
-struct blame_entry *this)
+struct blame_entry *potential)
 {
int i;
 
-   if (!this[1].suspect)
+   if (!potential[1].suspect)
return;
if (best_so_far[1].suspect) {
-   if (blame_entry_score(sb, [1]) < blame_entry_score(sb, 
_so_far[1]))
+   if (blame_entry_score(sb, [1]) <
+   blame_entry_score(sb, _so_far[1]))
return;
}
 
for (i = 0; i < 3; i++)
-   blame_origin_incref(this[i].suspect);
+   blame_origin_incref(potential[i].suspect);
decref_split(best_so_far);
-   memcpy(best_so_far, this, sizeof(struct blame_entry [3]));
+   memcpy(best_so_far, potential, sizeof(struct blame_entry[3]));
 }
 
 /*
@@ -1046,12 +1047,12 @@ static void handle_split(struct blame_scoreboard *sb,
if (ent->num_lines <= tlno)
return;
if (tlno < same) {
-   struct blame_entry this[3];
+   struct blame_entry potential[3];
tlno += ent->s_lno;
same += ent->s_lno;
-   split_overlap(this, ent, tlno, plno, same, parent);
-   copy_split_if_better(sb, split, this);
-   decref_split(this);
+   split_overlap(potential, ent, tlno, plno, same, parent);
+   copy_split_if_better(sb, split, potential);
+   decref_split(potential);
}
 }
 
@@ -1273,7 +1274,7 @@ static void find_copy_in_parent(struct blame_scoreboard 
*sb,
struct diff_filepair *p = diff_queued_diff.queue[i];
struct blame_origin *norigin;
mmfile_t file_p;
-   struct blame_entry this[3];
+   struct blame_entry potential[3];
 
if (!DIFF_FILE_VALID(p->one))
continue; /* does not exist in parent */
@@ -1292,10 +1293,10 @@ static void find_copy_in_parent(struct blame_scoreboard 
*sb,
 
for (j = 0; j < num_ents; j++) {
find_copy_in_blob(sb, blame_list[j].ent,
- norigin, this, _p);
+ norigin, potential, _p);
copy_split_if_better(sb, blame_list[j].split,
-this);
-   decref_split(this);
+potential);
+   decref_split(potential);
}
blame_origin_decref(norigin);
}
-- 
2.16.1.291.g4437f3f132-goog

[PATCH v2 05/37] rev-parse: rename 'this' variable

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 builtin/rev-parse.c | 34 +-
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 74aa644cb..171c7a2b4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -243,28 +243,28 @@ static int show_file(const char *arg, int output_prefix)
 static int try_difference(const char *arg)
 {
char *dotdot;
-   struct object_id oid;
-   struct object_id end;
-   const char *next;
-   const char *this;
+   struct object_id start_oid;
+   struct object_id end_oid;
+   const char *end;
+   const char *start;
int symmetric;
static const char head_by_default[] = "HEAD";
 
if (!(dotdot = strstr(arg, "..")))
return 0;
-   next = dotdot + 2;
-   this = arg;
-   symmetric = (*next == '.');
+   end = dotdot + 2;
+   start = arg;
+   symmetric = (*end == '.');
 
*dotdot = 0;
-   next += symmetric;
+   end += symmetric;
 
-   if (!*next)
-   next = head_by_default;
+   if (!*end)
+   end = head_by_default;
if (dotdot == arg)
-   this = head_by_default;
+   start = head_by_default;
 
-   if (this == head_by_default && next == head_by_default &&
+   if (start == head_by_default && end == head_by_default &&
!symmetric) {
/*
 * Just ".."?  That is not a range but the
@@ -274,14 +274,14 @@ static int try_difference(const char *arg)
return 0;
}
 
-   if (!get_oid_committish(this, ) && !get_oid_committish(next, )) 
{
-   show_rev(NORMAL, , next);
-   show_rev(symmetric ? NORMAL : REVERSED, , this);
+   if (!get_oid_committish(start, _oid) && !get_oid_committish(end, 
_oid)) {
+   show_rev(NORMAL, _oid, end);
+   show_rev(symmetric ? NORMAL : REVERSED, _oid, start);
if (symmetric) {
struct commit_list *exclude;
struct commit *a, *b;
-   a = lookup_commit_reference();
-   b = lookup_commit_reference();
+   a = lookup_commit_reference(_oid);
+   b = lookup_commit_reference(_oid);
exclude = get_merge_bases(a, b);
while (exclude) {
struct commit *commit = pop_commit();
-- 
2.16.1.291.g4437f3f132-goog

[PATCH v2 02/37] object: rename function 'typename' to 'type_name'

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 builtin/cat-file.c |  2 +-
 builtin/diff-tree.c|  2 +-
 builtin/fast-export.c  |  8 
 builtin/fsck.c |  4 ++--
 builtin/grep.c |  2 +-
 builtin/index-pack.c   | 12 ++--
 builtin/merge.c|  2 +-
 builtin/mktree.c   |  4 ++--
 builtin/prune.c|  2 +-
 builtin/replace.c  | 12 ++--
 builtin/tag.c  |  2 +-
 builtin/unpack-objects.c   | 10 +-
 builtin/verify-commit.c|  2 +-
 bulk-checkin.c |  2 +-
 commit.c   |  2 +-
 contrib/examples/builtin-fetch--tool.c |  2 +-
 fast-import.c  | 16 
 fsck.c |  2 +-
 http-push.c|  2 +-
 log-tree.c |  2 +-
 object.c   |  6 +++---
 object.h   |  2 +-
 pack-check.c   |  2 +-
 packfile.c |  2 +-
 reachable.c|  2 +-
 ref-filter.c   |  4 ++--
 sequencer.c|  2 +-
 sha1_file.c| 20 ++--
 sha1_name.c|  6 +++---
 submodule.c|  2 +-
 tag.c  |  2 +-
 walker.c   |  4 ++--
 32 files changed, 73 insertions(+), 73 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index d06c66c77..c6b3b1bfb 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -229,7 +229,7 @@ static void expand_atom(struct strbuf *sb, const char 
*atom, int len,
if (data->mark_query)
data->info.typep = >type;
else
-   strbuf_addstr(sb, typename(data->type));
+   strbuf_addstr(sb, type_name(data->type));
} else if (is_atom("objectsize", atom, len)) {
if (data->mark_query)
data->info.sizep = >size;
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index b775a7564..473615117 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -76,7 +76,7 @@ static int diff_tree_stdin(char *line)
if (obj->type == OBJ_TREE)
return stdin_diff_trees((struct tree *)obj, p);
error("Object %s is a %s, not a commit or tree",
- oid_to_hex(), typename(obj->type));
+ oid_to_hex(), type_name(obj->type));
return -1;
 }
 
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 796d0cd66..27b2cc138 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -240,7 +240,7 @@ static void export_blob(const struct object_id *oid)
buf = read_sha1_file(oid->hash, , );
if (!buf)
die ("Could not read blob %s", oid_to_hex(oid));
-   if (check_sha1_signature(oid->hash, buf, size, typename(type)) 
< 0)
+   if (check_sha1_signature(oid->hash, buf, size, type_name(type)) 
< 0)
die("sha1 mismatch in blob %s", oid_to_hex(oid));
object = parse_object_buffer(oid, type, size, buf, );
}
@@ -757,7 +757,7 @@ static void handle_tag(const char *name, struct tag *tag)
if (tagged->type != OBJ_COMMIT) {
die ("Tag %s tags unexported %s!",
 oid_to_hex(>object.oid),
-typename(tagged->type));
+type_name(tagged->type));
}
p = (struct commit *)tagged;
for (;;) {
@@ -839,7 +839,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info 
*info)
if (!commit) {
warning("%s: Unexpected object of type %s, skipping.",
e->name,
-   typename(e->item->type));
+   type_name(e->item->type));
continue;
}
 
@@ -851,7 +851,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info 
*info)
continue;
default: /* OBJ_TAG (nested tags) is already handled */
warning("Tag points to object of unexpected type %s, 
skipping.",
-   typename(commit->object.type));
+   type_name(commit->object.type));
continue;
 

[PATCH v2 04/37] pack-objects: rename 'this' variables

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 builtin/pack-objects.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6b9cfc289..bfda8602c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1376,10 +1376,10 @@ static void cleanup_preferred_base(void)
it = pbase_tree;
pbase_tree = NULL;
while (it) {
-   struct pbase_tree *this = it;
-   it = this->next;
-   free(this->pcache.tree_data);
-   free(this);
+   struct pbase_tree *tmp = it;
+   it = tmp->next;
+   free(tmp->pcache.tree_data);
+   free(tmp);
}
 
for (i = 0; i < ARRAY_SIZE(pbase_tree_cache); i++) {
-- 
2.16.1.291.g4437f3f132-goog

[PATCH v2 01/37] object_info: change member name from 'typename' to 'type_name'

[Brandon Williams]

Rename C++ keyword in order to bring the codebase closer to being able
to be compiled with a C++ compiler.

Signed-off-by: Brandon Williams 
---
 builtin/cat-file.c |  2 +-
 cache.h|  2 +-
 packfile.c |  6 +++---
 sha1_file.c| 10 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75..d06c66c77 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -76,7 +76,7 @@ static int cat_one_file(int opt, const char *exp_type, const 
char *obj_name,
buf = NULL;
switch (opt) {
case 't':
-   oi.typename = 
+   oi.type_name = 
if (sha1_object_info_extended(oid.hash, , flags) < 0)
die("git cat-file: could not get object info");
if (sb.len) {
diff --git a/cache.h b/cache.h
index d8b975a57..69b5a3bf8 100644
--- a/cache.h
+++ b/cache.h
@@ -1744,7 +1744,7 @@ struct object_info {
unsigned long *sizep;
off_t *disk_sizep;
unsigned char *delta_base_sha1;
-   struct strbuf *typename;
+   struct strbuf *type_name;
void **contentp;
 
/* Response */
diff --git a/packfile.c b/packfile.c
index 4a5fe7ab1..6657a0a49 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1350,16 +1350,16 @@ int packed_object_info(struct packed_git *p, off_t 
obj_offset,
*oi->disk_sizep = revidx[1].offset - obj_offset;
}
 
-   if (oi->typep || oi->typename) {
+   if (oi->typep || oi->type_name) {
enum object_type ptot;
ptot = packed_to_object_type(p, obj_offset, type, _curs,
 curpos);
if (oi->typep)
*oi->typep = ptot;
-   if (oi->typename) {
+   if (oi->type_name) {
const char *tn = typename(ptot);
if (tn)
-   strbuf_addstr(oi->typename, tn);
+   strbuf_addstr(oi->type_name, tn);
}
if (ptot < 0) {
type = OBJ_BAD;
diff --git a/sha1_file.c b/sha1_file.c
index 3da70ac65..2c03458ea 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1087,8 +1087,8 @@ static int parse_sha1_header_extended(const char *hdr, 
struct object_info *oi,
}
 
type = type_from_string_gently(type_buf, type_len, 1);
-   if (oi->typename)
-   strbuf_add(oi->typename, type_buf, type_len);
+   if (oi->type_name)
+   strbuf_add(oi->type_name, type_buf, type_len);
/*
 * Set type to 0 if its an unknown object and
 * we're obtaining the type using '--allow-unknown-type'
@@ -1158,7 +1158,7 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
 * return value implicitly indicates whether the
 * object even exists.
 */
-   if (!oi->typep && !oi->typename && !oi->sizep && !oi->contentp) {
+   if (!oi->typep && !oi->type_name && !oi->sizep && !oi->contentp) {
const char *path;
struct stat st;
if (stat_sha1_file(sha1, , ) < 0)
@@ -1239,8 +1239,8 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
*(oi->disk_sizep) = 0;
if (oi->delta_base_sha1)
hashclr(oi->delta_base_sha1);
-   if (oi->typename)
-   strbuf_addstr(oi->typename, typename(co->type));
+   if (oi->type_name)
+   strbuf_addstr(oi->type_name, 
typename(co->type));
if (oi->contentp)
*oi->contentp = xmemdupz(co->buf, co->size);
oi->whence = OI_CACHED;
-- 
2.16.1.291.g4437f3f132-goog

[PATCH v2 00/37] removal of some c++ keywords

[Brandon Williams]

One person was interested enough for me to go back through and also
rename all the paired 'old' variables to match the new names for the
variables which were named 'new'.

Brandon Williams (37):
  object_info: change member name from 'typename' to 'type_name'
  object: rename function 'typename' to 'type_name'
  blame: rename 'this' variables
  pack-objects: rename 'this' variables
  rev-parse: rename 'this' variable
  diff: rename 'this' variables
  apply: rename 'try' variables
  apply: rename 'new' variables
  checkout: rename 'new' variables
  help: rename 'new' variables
  pack-redundant: rename 'new' variables
  reflog: rename 'new' variables
  remote: rename 'new' variables
  combine-diff: rename 'new' variables
  commit: rename 'new' variables
  diff-lib: rename 'new' variable
  diff: rename 'new' variables
  diffcore-delta: rename 'new' variables
  entry: rename 'new' variables
  http: rename 'new' variables
  imap-send: rename 'new' variables
  line-log: rename 'new' variables
  read-cache: rename 'new' variables
  ref-filter: rename 'new' variables
  remote: rename 'new' variables
  split-index: rename 'new' variables
  submodule: rename 'new' variables
  trailer: rename 'new' variables
  unpack-trees: rename 'new' variables
  init-db: rename 'template' variables
  environment: rename 'template' variables
  diff: rename 'template' variables
  environment: rename 'namespace' variables
  wrapper: rename 'template' variables
  tempfile: rename 'template' variables
  trailer: rename 'template' variables
  replace: rename 'new' variables

 apply.c| 122 +++
 blame.c|  33 +++--
 builtin/cat-file.c |   4 +-
 builtin/checkout.c | 196 -
 builtin/diff-tree.c|   2 +-
 builtin/fast-export.c  |   8 +-
 builtin/fsck.c |   4 +-
 builtin/grep.c |   2 +-
 builtin/help.c |  10 +-
 builtin/index-pack.c   |  12 +-
 builtin/init-db.c  |  30 ++--
 builtin/merge.c|   2 +-
 builtin/mktree.c   |   4 +-
 builtin/pack-objects.c |   8 +-
 builtin/pack-redundant.c   |  54 +++
 builtin/prune.c|   2 +-
 builtin/reflog.c   |   8 +-
 builtin/remote.c   |  66 -
 builtin/replace.c  |  46 +++---
 builtin/rev-parse.c|  34 ++---
 builtin/tag.c  |   2 +-
 builtin/unpack-objects.c   |  10 +-
 builtin/verify-commit.c|   2 +-
 bulk-checkin.c |   2 +-
 cache.h|   4 +-
 combine-diff.c |  12 +-
 commit.c   |  20 +--
 contrib/examples/builtin-fetch--tool.c |   2 +-
 diff-lib.c |  38 ++---
 diff.c |  62 
 diffcore-delta.c   |  16 +-
 entry.c|  40 ++---
 environment.c  |  24 +--
 fast-import.c  |  16 +-
 fsck.c |   2 +-
 git-compat-util.h  |   4 +-
 http-push.c|   2 +-
 http.c |  10 +-
 imap-send.c|  14 +-
 line-log.c |  56 +++
 log-tree.c |   2 +-
 object.c   |   6 +-
 object.h   |   2 +-
 pack-check.c   |   2 +-
 packfile.c |   8 +-
 reachable.c|   2 +-
 read-cache.c   |  40 ++---
 ref-filter.c   |  20 +--
 remote.c   |  20 +--
 sequencer.c|   2 +-
 sha1_file.c|  28 ++--
 sha1_name.c|   6 +-
 split-index.c  |  16 +-
 split-index.h  |   2 +-
 submodule.c|  32 ++--
 submodule.h|   2 +-
 tag.c  |   2 +-
 tempfile.c |  12 +-
 tempfile.h |  34 ++---
 trailer.c  |  44 +++---
 unpack-trees.c |   6 +-
 walker.c   |   4 +-
 wrapper.c  |  40 ++---
 63 files changed, 659 insertions(+), 658 deletions(-)

-- 
2.16.1.291.g4437f3f132-goog

[PATCH v8 25/29] merge-recursive: fix overwriting dirty files involved in renames

[Elijah Newren]

This fixes an issue that existed before my directory rename detection
patches that affects both normal renames and renames implied by
directory rename detection.  Additional codepaths that only affect
overwriting of dirty files that are involved in directory rename
detection will be added in a subsequent commit.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 85 -
 merge-recursive.h   |  2 +
 t/t3501-revert-cherry-pick.sh   |  2 +-
 t/t6043-merge-rename-directories.sh |  2 +-
 t/t7607-merge-overwrite.sh  |  2 +-
 unpack-trees.c  |  4 +-
 unpack-trees.h  |  4 ++
 7 files changed, 77 insertions(+), 24 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index cd91eef805..4532b982a4 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,32 +337,37 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(int index_only,
+static int git_merge_trees(struct merge_options *o,
   struct tree *common,
   struct tree *head,
   struct tree *merge)
 {
int rc;
struct tree_desc t[3];
-   struct unpack_trees_options opts;
 
-   memset(, 0, sizeof(opts));
-   if (index_only)
-   opts.index_only = 1;
+   memset(>unpack_opts, 0, sizeof(o->unpack_opts));
+   if (o->call_depth)
+   o->unpack_opts.index_only = 1;
else
-   opts.update = 1;
-   opts.merge = 1;
-   opts.head_idx = 2;
-   opts.fn = threeway_merge;
-   opts.src_index = _index;
-   opts.dst_index = _index;
-   setup_unpack_trees_porcelain(, "merge");
+   o->unpack_opts.update = 1;
+   o->unpack_opts.merge = 1;
+   o->unpack_opts.head_idx = 2;
+   o->unpack_opts.fn = threeway_merge;
+   o->unpack_opts.src_index = _index;
+   o->unpack_opts.dst_index = _index;
+   setup_unpack_trees_porcelain(>unpack_opts, "merge");
 
init_tree_desc_from_tree(t+0, common);
init_tree_desc_from_tree(t+1, head);
init_tree_desc_from_tree(t+2, merge);
 
-   rc = unpack_trees(3, t, );
+   rc = unpack_trees(3, t, >unpack_opts);
+   /*
+* unpack_trees NULLifies src_index, but it's used in verify_uptodate,
+* so set to the new index which will usually have modification
+* timestamp info copied over.
+*/
+   o->unpack_opts.src_index = _index;
cache_tree_free(_cache_tree);
return rc;
 }
@@ -795,6 +800,20 @@ static int would_lose_untracked(const char *path)
return !was_tracked(path) && file_exists(path);
 }
 
+static int was_dirty(struct merge_options *o, const char *path)
+{
+   struct cache_entry *ce;
+   int dirty = 1;
+
+   if (o->call_depth || !was_tracked(path))
+   return !dirty;
+
+   ce = cache_file_exists(path, strlen(path), ignore_case);
+   dirty = (ce->ce_stat_data.sd_mtime.sec > 0 &&
+verify_uptodate(ce, >unpack_opts) != 0);
+   return dirty;
+}
+
 static int make_room_for_path(struct merge_options *o, const char *path)
 {
int status, i;
@@ -2686,6 +2705,7 @@ static int handle_modify_delete(struct merge_options *o,
 
 static int merge_content(struct merge_options *o,
 const char *path,
+int file_in_way,
 struct object_id *o_oid, int o_mode,
 struct object_id *a_oid, int a_mode,
 struct object_id *b_oid, int b_mode,
@@ -2760,7 +2780,7 @@ static int merge_content(struct merge_options *o,
return -1;
}
 
-   if (df_conflict_remains) {
+   if (df_conflict_remains || file_in_way) {
char *new_path;
if (o->call_depth) {
remove_file_from_cache(path);
@@ -2794,6 +2814,30 @@ static int merge_content(struct merge_options *o,
return mfi.clean;
 }
 
+static int conflict_rename_normal(struct merge_options *o,
+ const char *path,
+ struct object_id *o_oid, unsigned int o_mode,
+ struct object_id *a_oid, unsigned int a_mode,
+ struct object_id *b_oid, unsigned int b_mode,
+ struct rename_conflict_info *ci)
+{
+   int clean_merge;
+   int file_in_the_way = 0;
+
+   if (was_dirty(o, path)) {
+   file_in_the_way = 1;
+   output(o, 1, _("Refusing to lose dirty file at %s"), path);
+   }
+
+   /* Merge the content and write it out */
+   clean_merge = merge_content(o, path, 

[PATCH v8 28/29] merge-recursive: avoid spurious rename/rename conflict from dir renames

[Elijah Newren]

If a file on one side of history was renamed, and merely modified on the
other side, then applying a directory rename to the modified side gives us
a rename/rename(1to2) conflict.  We should only apply directory renames to
pairs representing either adds or renames.

Making this change means that a directory rename testcase that was
previously reported as a rename/delete conflict will now be reported as a
modify/delete conflict.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c   |  4 +--
 t/t6043-merge-rename-directories.sh | 55 +
 2 files changed, 27 insertions(+), 32 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 6ff54c0e86..5063caabe1 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1991,7 +1991,7 @@ static void compute_collisions(struct hashmap *collisions,
char *new_path;
struct diff_filepair *pair = pairs->queue[i];
 
-   if (pair->status == 'D')
+   if (pair->status != 'A' && pair->status != 'R')
continue;
dir_rename_ent = check_dir_renamed(pair->two->path,
   dir_renames);
@@ -2218,7 +2218,7 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_filepair *pair = pairs->queue[i];
char *new_path; /* non-NULL only with directory renames */
 
-   if (pair->status == 'D') {
+   if (pair->status != 'A' && pair->status != 'R') {
diff_free_filepair(pair);
continue;
}
diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 5b84591445..45f620633f 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2078,18 +2078,23 @@ test_expect_success '8b-check: Dual-directory rename, 
one into the others way, w
)
 '
 
-# Testcase 8c, rename+modify/delete
-#   (Related to testcases 5b and 8d)
+# Testcase 8c, modify/delete or rename+modify/delete?
+#   (Related to testcases 5b, 8d, and 9h)
 #   Commit O: z/{b,c,d}
 #   Commit A: y/{b,c}
 #   Commit B: z/{b,c,d_modified,e}
-#   Expected: y/{b,c,e}, CONFLICT(rename+modify/delete: x/d -> y/d or deleted)
+#   Expected: y/{b,c,e}, CONFLICT(modify/delete: on z/d)
 #
-#   Note: This testcase doesn't present any concerns for me...until you
-# compare it with testcases 5b and 8d.  See notes in 8d for more
-# details.
-
-test_expect_success '8c-setup: rename+modify/delete' '
+#   Note: It could easily be argued that the correct resolution here is
+# y/{b,c,e}, CONFLICT(rename/delete: z/d -> y/d vs deleted)
+# and that the modifed version of d should be present in y/ after
+# the merge, just marked as conflicted.  Indeed, I previously did
+# argue that.  But applying directory renames to the side of
+# history where a file is merely modified results in spurious
+# rename/rename(1to2) conflicts -- see testcase 9h.  See also
+# notes in 8d.
+
+test_expect_success '8c-setup: modify/delete or rename+modify/delete?' '
test_create_repo 8c &&
(
cd 8c &&
@@ -2122,32 +2127,32 @@ test_expect_success '8c-setup: rename+modify/delete' '
)
 '
 
-test_expect_success '8c-check: rename+modify/delete' '
+test_expect_success '8c-check: modify/delete or rename+modify/delete' '
(
cd 8c &&
 
git checkout A^0 &&
 
test_must_fail git merge -s recursive B^0 >out &&
-   test_i18ngrep "CONFLICT (rename/delete).* z/d.*y/d" out &&
+   test_i18ngrep "CONFLICT (modify/delete).* z/d" out &&
 
git ls-files -s >out &&
-   test_line_count = 4 out &&
+   test_line_count = 5 out &&
git ls-files -u >out &&
-   test_line_count = 1 out &&
+   test_line_count = 2 out &&
git ls-files -o >out &&
test_line_count = 1 out &&
 
git rev-parse >actual \
-   :0:y/b :0:y/c :0:y/e :3:y/d &&
+   :0:y/b :0:y/c :0:y/e :1:z/d :3:z/d &&
git rev-parse >expect \
-O:z/b  O:z/c  B:z/e  B:z/d &&
+O:z/b  O:z/c  B:z/e  O:z/d  B:z/d &&
test_cmp expect actual &&
 
-   test_must_fail git rev-parse :1:y/d &&
-   test_must_fail git rev-parse :2:y/d &&
-   git ls-files -s y/d | grep ^100755 &&
-   test_path_is_file y/d
+   test_must_fail git rev-parse :2:z/d &&
+   git ls-files -s z/d | grep ^100755 &&
+   test_path_is_file z/d &&
+   test_path_is_missing y/d
)
 '
 
@@ -2161,16 +2166,6 @@ 

[PATCH v8 21/29] merge-recursive: check for file level conflicts then get new name

[Elijah Newren]

Before trying to apply directory renames to paths within the given
directories, we want to make sure that there aren't conflicts at the
file level either.  If there aren't any, then get the new name from
any directory renames.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 174 ++--
 strbuf.c|  16 
 strbuf.h|  16 
 t/t6043-merge-rename-directories.sh |   2 +-
 4 files changed, 199 insertions(+), 9 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 92f0d62f60..efe9c9a98e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1519,6 +1519,91 @@ static void remove_hashmap_entries(struct hashmap 
*dir_renames,
string_list_clear(items_to_remove, 0);
 }
 
+/*
+ * See if there is a directory rename for path, and if there are any file
+ * level conflicts for the renamed location.  If there is a rename and
+ * there are no conflicts, return the new name.  Otherwise, return NULL.
+ */
+static char *handle_path_level_conflicts(struct merge_options *o,
+const char *path,
+struct dir_rename_entry *entry,
+struct hashmap *collisions,
+struct tree *tree)
+{
+   char *new_path = NULL;
+   struct collision_entry *collision_ent;
+   int clean = 1;
+   struct strbuf collision_paths = STRBUF_INIT;
+
+   /*
+* entry has the mapping of old directory name to new directory name
+* that we want to apply to path.
+*/
+   new_path = apply_dir_rename(entry, path);
+
+   if (!new_path) {
+   /* This should only happen when entry->non_unique_new_dir set */
+   if (!entry->non_unique_new_dir)
+   BUG("entry->non_unqiue_dir not set and !new_path");
+   output(o, 1, _("CONFLICT (directory rename split): "
+  "Unclear where to place %s because directory "
+  "%s was renamed to multiple other directories, "
+  "with no destination getting a majority of the "
+  "files."),
+  path, entry->dir);
+   clean = 0;
+   return NULL;
+   }
+
+   /*
+* The caller needs to have ensured that it has pre-populated
+* collisions with all paths that map to new_path.  Do a quick check
+* to ensure that's the case.
+*/
+   collision_ent = collision_find_entry(collisions, new_path);
+   if (collision_ent == NULL)
+   BUG("collision_ent is NULL");
+
+   /*
+* Check for one-sided add/add/.../add conflicts, i.e.
+* where implicit renames from the other side doing
+* directory rename(s) can affect this side of history
+* to put multiple paths into the same location.  Warn
+* and bail on directory renames for such paths.
+*/
+   if (collision_ent->reported_already) {
+   clean = 0;
+   } else if (tree_has_path(tree, new_path)) {
+   collision_ent->reported_already = 1;
+   strbuf_add_separated_string_list(_paths, ", ",
+_ent->source_files);
+   output(o, 1, _("CONFLICT (implicit dir rename): Existing "
+  "file/dir at %s in the way of implicit "
+  "directory rename(s) putting the following "
+  "path(s) there: %s."),
+  new_path, collision_paths.buf);
+   clean = 0;
+   } else if (collision_ent->source_files.nr > 1) {
+   collision_ent->reported_already = 1;
+   strbuf_add_separated_string_list(_paths, ", ",
+_ent->source_files);
+   output(o, 1, _("CONFLICT (implicit dir rename): Cannot map "
+  "more than one path to %s; implicit directory "
+  "renames tried to put these paths there: %s"),
+  new_path, collision_paths.buf);
+   clean = 0;
+   }
+
+   /* Free memory we no longer need */
+   strbuf_release(_paths);
+   if (!clean && new_path) {
+   free(new_path);
+   return NULL;
+   }
+
+   return new_path;
+}
+
 /*
  * There are a couple things we want to do at the directory level:
  *   1. Check for both sides renaming to the same thing, in order to avoid
@@ -1798,6 +1883,59 @@ static void compute_collisions(struct hashmap 
*collisions,
}
 }
 
+static char *check_for_directory_rename(struct merge_options *o,
+   const char *path,
+   

[PATCH v8 10/29] directory rename detection: tests for handling overwriting untracked files

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 367 
 1 file changed, 367 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index cbbb949014..a6cd38336c 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2879,4 +2879,371 @@ test_expect_failure '9g-check: Renamed directory that 
only contained immediate s
 #   side of history for any implicit directory renames.
 ###
 
+###
+# SECTION 10: Handling untracked files
+#
+# unpack_trees(), upon which the recursive merge algorithm is based, aborts
+# the operation if untracked or dirty files would be deleted or overwritten
+# by the merge.  Unfortunately, unpack_trees() does not understand renames,
+# and if it doesn't abort, then it muddies up the working directory before
+# we even get to the point of detecting renames, so we need some special
+# handling, at least in the case of directory renames.
+###
+
+# Testcase 10a, Overwrite untracked: normal rename/delete
+#   Commit O: z/{b,c_1}
+#   Commit A: z/b + untracked z/c + untracked z/d
+#   Commit B: z/{b,d_1}
+#   Expected: Aborted Merge +
+#   ERROR_MSG(untracked working tree files would be overwritten by merge)
+
+test_expect_success '10a-setup: Overwrite untracked with normal rename/delete' 
'
+   test_create_repo 10a &&
+   (
+   cd 10a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git rm z/c &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv z/c z/d &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '10a-check: Overwrite untracked with normal rename/delete' 
'
+   (
+   cd 10a &&
+
+   git checkout A^0 &&
+   echo very >z/c &&
+   echo important >z/d &&
+
+   test_must_fail git merge -s recursive B^0 >out 2>err &&
+   test_i18ngrep "The following untracked working tree files would 
be overwritten by merge" err &&
+
+   git ls-files -s >out &&
+   test_line_count = 1 out &&
+   git ls-files -o >out &&
+   test_line_count = 4 out &&
+
+   echo very >expect &&
+   test_cmp expect z/c &&
+
+   echo important >expect &&
+   test_cmp expect z/d &&
+
+   git rev-parse HEAD:z/b >actual &&
+   git rev-parse O:z/b >expect &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 10b, Overwrite untracked: dir rename + delete
+#   Commit O: z/{b,c_1}
+#   Commit A: y/b + untracked y/{c,d,e}
+#   Commit B: z/{b,d_1,e}
+#   Expected: Failed Merge; y/b + untracked y/c + untracked y/d on disk +
+# z/c_1 -> z/d_1 rename recorded at stage 3 for y/d +
+#   ERROR_MSG(refusing to lose untracked file at 'y/d')
+
+test_expect_success '10b-setup: Overwrite untracked with dir rename + delete' '
+   test_create_repo 10b &&
+   (
+   cd 10b &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git rm z/c &&
+   git mv z/ y/ &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv z/c z/d &&
+   echo e >z/e &&
+   git add z/e &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '10b-check: Overwrite untracked with dir rename + delete' '
+   (
+   cd 10b &&
+
+   git checkout A^0 &&
+   echo very >y/c &&
+   echo important >y/d &&
+   echo contents >y/e &&
+
+   test_must_fail git merge -s recursive B^0 >out 2>err &&
+   test_i18ngrep "CONFLICT (rename/delete).*Version B\^0 of y/d 
left in tree at y/d~B\^0" out &&
+   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
writing to y/e~B\^0 instead" out &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out 

[PATCH v8 24/29] merge-recursive: avoid clobbering untracked files with directory renames

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 42 +++--
 t/t6043-merge-rename-directories.sh |  6 +++---
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 0ed437c370..cd91eef805 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1150,6 +1150,26 @@ static int conflict_rename_dir(struct merge_options *o,
 {
const struct diff_filespec *dest = pair->two;
 
+   if (!o->call_depth && would_lose_untracked(dest->path)) {
+   char *alt_path = unique_path(o, dest->path, rename_branch);
+
+   output(o, 1, _("Error: Refusing to lose untracked file at %s; "
+  "writing to %s instead."),
+  dest->path, alt_path);
+   /*
+* Write the file in worktree at alt_path, but not in the
+* index.  Instead, write to dest->path for the index but
+* only at the higher appropriate stage.
+*/
+   if (update_file(o, 0, >oid, dest->mode, alt_path))
+   return -1;
+   free(alt_path);
+   return update_stages(o, dest->path, NULL,
+rename_branch == o->branch1 ? dest : NULL,
+rename_branch == o->branch1 ? NULL : dest);
+   }
+
+   /* Update dest->path both in index and in worktree */
if (update_file(o, 1, >oid, dest->mode, dest->path))
return -1;
return 0;
@@ -1168,7 +1188,8 @@ static int handle_change_delete(struct merge_options *o,
const char *update_path = path;
int ret = 0;
 
-   if (dir_in_way(path, !o->call_depth, 0)) {
+   if (dir_in_way(path, !o->call_depth, 0) ||
+   (!o->call_depth && would_lose_untracked(path))) {
update_path = alt_path = unique_path(o, path, change_branch);
}
 
@@ -1294,6 +1315,12 @@ static int handle_file(struct merge_options *o,
dst_name = unique_path(o, rename->path, cur_branch);
output(o, 1, _("%s is a directory in %s adding as %s 
instead"),
   rename->path, other_branch, dst_name);
+   } else if (!o->call_depth &&
+  would_lose_untracked(rename->path)) {
+   dst_name = unique_path(o, rename->path, cur_branch);
+   output(o, 1, _("Refusing to lose untracked file at %s; "
+  "adding as %s instead"),
+  rename->path, dst_name);
}
}
if ((ret = update_file(o, 0, >oid, rename->mode, dst_name)))
@@ -1419,7 +1446,18 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
char *new_path2 = unique_path(o, path, ci->branch2);
output(o, 1, _("Renaming %s to %s and %s to %s instead"),
   a->path, new_path1, b->path, new_path2);
-   remove_file(o, 0, path, 0);
+   if (would_lose_untracked(path))
+   /*
+* Only way we get here is if both renames were from
+* a directory rename AND user had an untracked file
+* at the location where both files end up after the
+* two directory renames.  See testcase 10d of t6043.
+*/
+   output(o, 1, _("Refusing to lose untracked file at "
+  "%s, even though it's in the way."),
+  path);
+   else
+   remove_file(o, 0, path, 0);
ret = update_file(o, 0, _c1.oid, mfi_c1.mode, new_path1);
if (!ret)
ret = update_file(o, 0, _c2.oid, mfi_c2.mode,
diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 3525c54bb4..0b60eb8053 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2992,7 +2992,7 @@ test_expect_success '10b-setup: Overwrite untracked with 
dir rename + delete' '
)
 '
 
-test_expect_failure '10b-check: Overwrite untracked with dir rename + delete' '
+test_expect_success '10b-check: Overwrite untracked with dir rename + delete' '
(
cd 10b &&
 
@@ -3070,7 +3070,7 @@ test_expect_success '10c-setup: Overwrite untracked with 
dir rename/rename(1to2)
)
 '
 
-test_expect_failure '10c-check: Overwrite untracked with dir 
rename/rename(1to2)' '
+test_expect_success '10c-check: Overwrite untracked with dir 
rename/rename(1to2)' '
(
cd 10c &&
 
@@ -3145,7 +3145,7 @@ test_expect_success '10d-setup: Delete untracked with dir 
rename/rename(2to1)' '

[PATCH v8 08/29] directory rename detection: testcases exploring possibly suboptimal merges

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 404 
 1 file changed, 404 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 6db1439675..e211e8ca31 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1912,4 +1912,408 @@ test_expect_failure '7e-check: transitive rename in 
rename/delete AND dirs in th
)
 '
 
+###
+# SECTION 8: Suboptimal merges
+#
+# As alluded to in the last section, the ruleset we have built up for
+# detecting directory renames unfortunately has some special cases where it
+# results in slightly suboptimal or non-intuitive behavior.  This section
+# explores these cases.
+#
+# To be fair, we already had non-intuitive or suboptimal behavior for most
+# of these cases in git before introducing implicit directory rename
+# detection, but it'd be nice if there was a modified ruleset out there
+# that handled these cases a bit better.
+###
+
+# Testcase 8a, Dual-directory rename, one into the others' way
+#   Commit O. x/{a,b},   y/{c,d}
+#   Commit A. x/{a,b,e}, y/{c,d,f}
+#   Commit B. y/{a,b},   z/{c,d}
+#
+# Possible Resolutions:
+#   w/o dir-rename detection: y/{a,b,f},   z/{c,d},   x/e
+#   Currently expected:   y/{a,b,e,f}, z/{c,d}
+#   Optimal:  y/{a,b,e},   z/{c,d,f}
+#
+# Note: Both x and y got renamed and it'd be nice to detect both, and we do
+# better with directory rename detection than git did without, but the
+# simple rule from section 5 prevents me from handling this as optimally as
+# we potentially could.
+
+test_expect_success '8a-setup: Dual-directory rename, one into the others way' 
'
+   test_create_repo 8a &&
+   (
+   cd 8a &&
+
+   mkdir x &&
+   mkdir y &&
+   echo a >x/a &&
+   echo b >x/b &&
+   echo c >y/c &&
+   echo d >y/d &&
+   git add x y &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   echo e >x/e &&
+   echo f >y/f &&
+   git add x/e y/f &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv y z &&
+   git mv x y &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '8a-check: Dual-directory rename, one into the others way' 
'
+   (
+   cd 8a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 6 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   HEAD:y/a HEAD:y/b HEAD:y/e HEAD:y/f HEAD:z/c HEAD:z/d &&
+   git rev-parse >expect \
+   O:x/aO:x/bA:x/eA:y/fO:y/cO:y/d &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 8b, Dual-directory rename, one into the others' way, with 
conflicting filenames
+#   Commit O. x/{a_1,b_1}, y/{a_2,b_2}
+#   Commit A. x/{a_1,b_1,e_1}, y/{a_2,b_2,e_2}
+#   Commit B. y/{a_1,b_1}, z/{a_2,b_2}
+#
+#   w/o dir-rename detection: y/{a_1,b_1,e_2}, z/{a_2,b_2}, x/e_1
+#   Currently expected:   
+#   Scary:y/{a_1,b_1}, z/{a_2,b_2}, CONFLICT(add/add, 
e_1 vs. e_2)
+#   Optimal:  y/{a_1,b_1,e_1}, z/{a_2,b_2,e_2}
+#
+# Note: Very similar to 8a, except instead of 'e' and 'f' in directories x and
+# y, both are named 'e'.  Without directory rename detection, neither file
+# moves directories.  Implement directory rename detection suboptimally, and
+# you get an add/add conflict, but both files were added in commit A, so this
+# is an add/add conflict where one side of history added both files --
+# something we can't represent in the index.  Obviously, we'd prefer the last
+# resolution, but our previous rules are too coarse to allow it.  Using both
+# the rules from section 4 and section 5 save us from the Scary resolution,
+# making us fall back to pre-directory-rename-detection behavior for both
+# e_1 and e_2.
+
+test_expect_success '8b-setup: Dual-directory rename, one into the others way, 
with conflicting filenames' '
+   test_create_repo 8b &&
+   (
+   cd 8b &&
+
+   mkdir x &&
+   mkdir y &&
+   echo a1 >x/a &&
+   

[PATCH v8 27/29] directory rename detection: new testcases showcasing a pair of bugs

[Elijah Newren]

Add a testcase showing spurious rename/rename(1to2) conflicts occurring
due to directory rename detection.

Also add a pair of testcases dealing with moving directory hierarchies
around that were suggested by Stefan Beller as "food for thought" during
his review of an earlier patch series, but which actually uncovered a
bug.  Round things out with a test that is a cross between the two
testcases that showed existing bugs in order to make sure we aren't
merely addressing problems in isolation but in general.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 296 
 1 file changed, 296 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 33e2649824..5b84591445 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -159,6 +159,7 @@ test_expect_success '1b-check: Merge a directory with 
another' '
 # Testcase 1c, Transitive renaming
 #   (Related to testcases 3a and 6d -- when should a transitive rename apply?)
 #   (Related to testcases 9c and 9d -- can transitivity repeat?)
+#   (Related to testcase 12b -- joint-transitivity?)
 #   Commit O: z/{b,c},   x/d
 #   Commit A: y/{b,c},   x/d
 #   Commit B: z/{b,c,d}
@@ -2871,6 +2872,68 @@ test_expect_failure '9g-check: Renamed directory that 
only contained immediate s
)
 '
 
+# Testcase 9h, Avoid implicit rename if involved as source on other side
+#   (Extremely closely related to testcase 3a)
+#   Commit O: z/{b,c,d_1}
+#   Commit A: z/{b,c,d_2}
+#   Commit B: y/{b,c}, x/d_1
+#   Expected: y/{b,c}, x/d_2
+#   NOTE: If we applied the z/ -> y/ rename to z/d, then we'd end up with
+# a rename/rename(1to2) conflict (z/d -> y/d vs. x/d)
+test_expect_success '9h-setup: Avoid dir rename on merely modified path' '
+   test_create_repo 9h &&
+   (
+   cd 9h &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   printf "1\n2\n3\n4\n5\n6\n7\n8\nd\n" >z/d &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   test_tick &&
+   echo more >>z/d &&
+   git add z/d &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   mkdir y &&
+   mkdir x &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   git mv z/d x/ &&
+   rmdir z &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '9h-check: Avoid dir rename on merely modified path' '
+   (
+   cd 9h &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:x/d &&
+   git rev-parse >expect \
+   O:z/bO:z/cA:z/d &&
+   test_cmp expect actual
+   )
+'
+
 ###
 # Rules suggested by section 9:
 #
@@ -3704,4 +3767,237 @@ test_expect_success '11f-check: Avoid deleting 
not-uptodate with dir rename/rena
)
 '
 
+###
+# SECTION 12: Everything else
+#
+# Tests suggested by others.  Tests added after implementation completed
+# and submitted.  Grab bag.
+###
+
+# Testcase 12a, Moving one directory hierarchy into another
+#   (Related to testcase 9a)
+#   Commit O: node1/{leaf1,leaf2}, node2/{leaf3,leaf4}
+#   Commit A: node1/{leaf1,leaf2,node2/{leaf3,leaf4}}
+#   Commit B: node1/{leaf1,leaf2,leaf5}, node2/{leaf3,leaf4,leaf6}
+#   Expected: node1/{leaf1,leaf2,leaf5,node2/{leaf3,leaf4,leaf6}}
+
+test_expect_success '12a-setup: Moving one directory hierarchy into another' '
+   test_create_repo 12a &&
+   (
+   cd 12a &&
+
+   mkdir -p node1 node2 &&
+   echo leaf1 >node1/leaf1 &&
+   echo leaf2 >node1/leaf2 &&
+   echo leaf3 >node2/leaf3 &&
+   echo leaf4 >node2/leaf4 &&
+   git add node1 node2 &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git mv node2/ node1/ &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   echo leaf5 >node1/leaf5 &&
+   echo leaf6 

[PATCH v8 14/29] merge-recursive: fix leaks of allocated renames and diff_filepairs

[Elijah Newren]

get_renames() has always zero'ed out diff_queued_diff.nr while only
manually free'ing diff_filepairs that did not correspond to renames.
Further, it allocated struct renames that were tucked away in the
return string_list.  Make sure all of these are deallocated when we
are done with them.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index eac3041261..1986af79a9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1662,13 +1662,23 @@ static int handle_renames(struct merge_options *o,
return process_renames(o, ri->head_renames, ri->merge_renames);
 }
 
-static void cleanup_renames(struct rename_info *re_info)
+static void cleanup_rename(struct string_list *rename)
 {
-   string_list_clear(re_info->head_renames, 0);
-   string_list_clear(re_info->merge_renames, 0);
+   const struct rename *re;
+   int i;
 
-   free(re_info->head_renames);
-   free(re_info->merge_renames);
+   for (i = 0; i < rename->nr; i++) {
+   re = rename->items[i].util;
+   diff_free_filepair(re->pair);
+   }
+   string_list_clear(rename, 1);
+   free(rename);
+}
+
+static void cleanup_renames(struct rename_info *re_info)
+{
+   cleanup_rename(re_info->head_renames);
+   cleanup_rename(re_info->merge_renames);
 }
 
 static struct object_id *stage_oid(const struct object_id *oid, unsigned mode)
-- 
2.16.1.232.g28d5be9217

[PATCH v8 20/29] merge-recursive: add computation of collisions due to dir rename & merging

[Elijah Newren]

directory renaming and merging can cause one or more files to be moved to
where an existing file is, or to cause several files to all be moved to
the same (otherwise vacant) location.  Add checking and reporting for such
cases, falling back to no-directory-rename handling for such paths.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 146 --
 merge-recursive.h |   7 +++
 2 files changed, 150 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index a9f1579d22..92f0d62f60 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -87,6 +87,29 @@ static void dir_rename_entry_init(struct dir_rename_entry 
*entry,
string_list_init(>possible_new_dirs, 0);
 }
 
+static struct collision_entry *collision_find_entry(struct hashmap *hashmap,
+   char *target_file)
+{
+   struct collision_entry key;
+
+   hashmap_entry_init(, strhash(target_file));
+   key.target_file = target_file;
+   return hashmap_get(hashmap, , NULL);
+}
+
+static int collision_cmp(void *unused_cmp_data,
+const struct collision_entry *e1,
+const struct collision_entry *e2,
+const void *unused_keydata)
+{
+   return strcmp(e1->target_file, e2->target_file);
+}
+
+static void collision_init(struct hashmap *map)
+{
+   hashmap_init(map, (hashmap_cmp_fn) collision_cmp, NULL, 0);
+}
+
 static void flush_output(struct merge_options *o)
 {
if (o->buffer_output < 2 && o->obuf.len) {
@@ -1403,6 +1426,31 @@ static int tree_has_path(struct tree *tree, const char 
*path)
   hashy, _o);
 }
 
+/*
+ * Return a new string that replaces the beginning portion (which matches
+ * entry->dir), with entry->new_dir.  In perl-speak:
+ *   new_path_name = (old_path =~ s/entry->dir/entry->new_dir/);
+ * NOTE:
+ *   Caller must ensure that old_path starts with entry->dir + '/'.
+ */
+static char *apply_dir_rename(struct dir_rename_entry *entry,
+ const char *old_path)
+{
+   struct strbuf new_path = STRBUF_INIT;
+   int oldlen, newlen;
+
+   if (entry->non_unique_new_dir)
+   return NULL;
+
+   oldlen = strlen(entry->dir);
+   newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1;
+   strbuf_grow(_path, newlen);
+   strbuf_addbuf(_path, >new_dir);
+   strbuf_addstr(_path, _path[oldlen]);
+
+   return strbuf_detach(_path, NULL);
+}
+
 static void get_renamed_dir_portion(const char *old_path, const char *new_path,
char **old_dir, char **new_dir)
 {
@@ -1672,6 +1720,84 @@ static struct hashmap *get_directory_renames(struct 
diff_queue_struct *pairs,
return dir_renames;
 }
 
+static struct dir_rename_entry *check_dir_renamed(const char *path,
+ struct hashmap *dir_renames)
+{
+   char temp[PATH_MAX];
+   char *end;
+   struct dir_rename_entry *entry;
+
+   strcpy(temp, path);
+   while ((end = strrchr(temp, '/'))) {
+   *end = '\0';
+   entry = dir_rename_find_entry(dir_renames, temp);
+   if (entry)
+   return entry;
+   }
+   return NULL;
+}
+
+static void compute_collisions(struct hashmap *collisions,
+  struct hashmap *dir_renames,
+  struct diff_queue_struct *pairs)
+{
+   int i;
+
+   /*
+* Multiple files can be mapped to the same path due to directory
+* renames done by the other side of history.  Since that other
+* side of history could have merged multiple directories into one,
+* if our side of history added the same file basename to each of
+* those directories, then all N of them would get implicitly
+* renamed by the directory rename detection into the same path,
+* and we'd get an add/add/.../add conflict, and all those adds
+* from *this* side of history.  This is not representable in the
+* index, and users aren't going to easily be able to make sense of
+* it.  So we need to provide a good warning about what's
+* happening, and fall back to no-directory-rename detection
+* behavior for those paths.
+*
+* See testcases 9e and all of section 5 from t6043 for examples.
+*/
+   collision_init(collisions);
+
+   for (i = 0; i < pairs->nr; ++i) {
+   struct dir_rename_entry *dir_rename_ent;
+   struct collision_entry *collision_ent;
+   char *new_path;
+   struct diff_filepair *pair = pairs->queue[i];
+
+   if (pair->status == 'D')
+   continue;
+   dir_rename_ent = 

[PATCH v8 23/29] merge-recursive: apply necessary modifications for directory renames

[Elijah Newren]

This commit hooks together all the directory rename logic by making the
necessary changes to the rename struct, it's dst_entry, and the
diff_filepair under consideration.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 187 +++-
 t/t6043-merge-rename-directories.sh |  50 +-
 2 files changed, 211 insertions(+), 26 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 18f92be608..0ed437c370 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -180,6 +180,7 @@ static int oid_eq(const struct object_id *a, const struct 
object_id *b)
 
 enum rename_type {
RENAME_NORMAL = 0,
+   RENAME_DIR,
RENAME_DELETE,
RENAME_ONE_FILE_TO_ONE,
RENAME_ONE_FILE_TO_TWO,
@@ -610,6 +611,7 @@ struct rename {
 */
struct stage_data *src_entry;
struct stage_data *dst_entry;
+   unsigned add_turned_into_rename:1;
unsigned processed:1;
 };
 
@@ -644,6 +646,27 @@ static int update_stages(struct merge_options *opt, const 
char *path,
return 0;
 }
 
+static int update_stages_for_stage_data(struct merge_options *opt,
+   const char *path,
+   const struct stage_data *stage_data)
+{
+   struct diff_filespec o, a, b;
+
+   o.mode = stage_data->stages[1].mode;
+   oidcpy(, _data->stages[1].oid);
+
+   a.mode = stage_data->stages[2].mode;
+   oidcpy(, _data->stages[2].oid);
+
+   b.mode = stage_data->stages[3].mode;
+   oidcpy(, _data->stages[3].oid);
+
+   return update_stages(opt, path,
+is_null_sha1(o.oid.hash) ? NULL : ,
+is_null_sha1(a.oid.hash) ? NULL : ,
+is_null_sha1(b.oid.hash) ? NULL : );
+}
+
 static void update_entry(struct stage_data *entry,
 struct diff_filespec *o,
 struct diff_filespec *a,
@@ -1120,6 +1143,18 @@ static int merge_file_one(struct merge_options *o,
return merge_file_1(o, , , , branch1, branch2, mfi);
 }
 
+static int conflict_rename_dir(struct merge_options *o,
+  struct diff_filepair *pair,
+  const char *rename_branch,
+  const char *other_branch)
+{
+   const struct diff_filespec *dest = pair->two;
+
+   if (update_file(o, 1, >oid, dest->mode, dest->path))
+   return -1;
+   return 0;
+}
+
 static int handle_change_delete(struct merge_options *o,
 const char *path, const char *old_path,
 const struct object_id *o_oid, int o_mode,
@@ -1389,6 +1424,24 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
if (!ret)
ret = update_file(o, 0, _c2.oid, mfi_c2.mode,
  new_path2);
+   /*
+* unpack_trees() actually populates the index for us for
+* "normal" rename/rename(2to1) situtations so that the
+* correct entries are at the higher stages, which would
+* make the call below to update_stages_for_stage_data
+* unnecessary.  However, if either of the renames came
+* from a directory rename, then unpack_trees() will not
+* have gotten the right data loaded into the index, so we
+* need to do so now.  (While it'd be tempting to move this
+* call to update_stages_for_stage_data() to
+* apply_directory_rename_modifications(), that would break
+* our intermediate calls to would_lose_untracked() since
+* those rely on the current in-memory index.  See also the
+* big "NOTE" in update_stages()).
+*/
+   if (update_stages_for_stage_data(o, path, ci->dst_entry1))
+   ret = -1;
+
free(new_path2);
free(new_path1);
}
@@ -1951,6 +2004,111 @@ static char *check_for_directory_rename(struct 
merge_options *o,
return new_path;
 }
 
+static void apply_directory_rename_modifications(struct merge_options *o,
+struct diff_filepair *pair,
+char *new_path,
+struct rename *re,
+struct tree *tree,
+struct tree *o_tree,
+struct tree *a_tree,
+struct tree *b_tree,
+struct string_list *entries,
+   

[PATCH v8 07/29] directory rename detection: more involved edge/corner testcases

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 396 
 1 file changed, 396 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 9ae245a522..6db1439675 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1516,4 +1516,400 @@ test_expect_success '6e-check: Add/add from one side' '
 #   side of history is the one doing the renaming.
 ###
 
+
+###
+# SECTION 7: More involved Edge/Corner cases
+#
+# The ruleset we have generated in the above sections seems to provide
+# well-defined merges.  But can we find edge/corner cases that either (a)
+# are harder for users to understand, or (b) have a resolution that is
+# non-intuitive or suboptimal?
+#
+# The testcases in this section dive into cases that I've tried to craft in
+# a way to find some that might be surprising to users or difficult for
+# them to understand (the next section will look at non-intuitive or
+# suboptimal merge results).  Some of the testcases are similar to ones
+# from past sections, but have been simplified to try to highlight error
+# messages using a "modified" path (due to the directory rename).  Are
+# users okay with these?
+#
+# In my opinion, testcases that are difficult to understand from this
+# section is due to difficulty in the testcase rather than the directory
+# renaming (similar to how t6042 and t6036 have difficult resolutions due
+# to the problem setup itself being complex).  And I don't think the
+# error messages are a problem.
+#
+# On the other hand, the testcases in section 8 worry me slightly more...
+###
+
+# Testcase 7a, rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file
+#   Commit O: z/{b,c}
+#   Commit A: y/{b,c}
+#   Commit B: w/b, x/c, z/d
+#   Expected: y/d, CONFLICT(rename/rename for both z/b and z/c)
+#   NOTE: There's a rename of z/ here, y/ has more renames, so z/d -> y/d.
+
+test_expect_success '7a-setup: rename-dir vs. rename-dir (NOT split evenly) 
PLUS add-other-file' '
+   test_create_repo 7a &&
+   (
+   cd 7a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git mv z y &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   mkdir w &&
+   mkdir x &&
+   git mv z/b w/ &&
+   git mv z/c x/ &&
+   echo d > z/d &&
+   git add z/d &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '7a-check: rename-dir vs. rename-dir (NOT split evenly) 
PLUS add-other-file' '
+   (
+   cd 7a &&
+
+   git checkout A^0 &&
+
+   test_must_fail git merge -s recursive B^0 >out &&
+   test_i18ngrep "CONFLICT (rename/rename).*z/b.*y/b.*w/b" out &&
+   test_i18ngrep "CONFLICT (rename/rename).*z/c.*y/c.*x/c" out &&
+
+   git ls-files -s >out &&
+   test_line_count = 7 out &&
+   git ls-files -u >out &&
+   test_line_count = 6 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   :1:z/b :2:y/b :3:w/b :1:z/c :2:y/c :3:x/c :0:y/d &&
+   git rev-parse >expect \
+O:z/b  O:z/b  O:z/b  O:z/c  O:z/c  O:z/c  B:z/d &&
+   test_cmp expect actual &&
+
+   git hash-object >actual \
+   y/b   w/b   y/c   x/c &&
+   git rev-parse >expect \
+   O:z/b O:z/b O:z/c O:z/c &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 7b, rename/rename(2to1), but only due to transitive rename
+#   (Related to testcase 1d)
+#   Commit O: z/{b,c}, x/d_1, w/d_2
+#   Commit A: y/{b,c,d_2}, x/d_1
+#   Commit B: z/{b,c,d_1},w/d_2
+#   Expected: y/{b,c}, CONFLICT(rename/rename(2to1): x/d_1, w/d_2 -> y_d)
+
+test_expect_success '7b-setup: rename/rename(2to1), but only due to transitive 
rename' '
+   test_create_repo 7b &&
+   (
+   cd 7b &&
+
+   mkdir z &&
+   mkdir x &&
+   mkdir w &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d1 > x/d &&
+   echo d2 > w/d &&
+   git add z x 

[PATCH v8 13/29] merge-recursive: introduce new functions to handle rename logic

[Elijah Newren]

The amount of logic in merge_trees() relative to renames was just a few
lines, but split it out into new handle_renames() and cleanup_renames()
functions to prepare for additional logic to be added to each.  No code or
logic changes, just a new place to put stuff for when the rename detection
gains additional checks.

Note that process_renames() records pointers to various information (such
as diff_filepairs) into rename_conflict_info structs.  Even though the
rename string_lists are not directly used once handle_renames() completes,
we should not immediately free the lists at the end of that function
because they store the information referenced in the rename_conflict_info,
which is used later in process_entry().  Thus the reason for a separate
cleanup_renames().

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 43 +--
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 2028dd113b..eac3041261 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1645,6 +1645,32 @@ static int process_renames(struct merge_options *o,
return clean_merge;
 }
 
+struct rename_info {
+   struct string_list *head_renames;
+   struct string_list *merge_renames;
+};
+
+static int handle_renames(struct merge_options *o,
+ struct tree *common,
+ struct tree *head,
+ struct tree *merge,
+ struct string_list *entries,
+ struct rename_info *ri)
+{
+   ri->head_renames  = get_renames(o, head, common, head, merge, entries);
+   ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
+   return process_renames(o, ri->head_renames, ri->merge_renames);
+}
+
+static void cleanup_renames(struct rename_info *re_info)
+{
+   string_list_clear(re_info->head_renames, 0);
+   string_list_clear(re_info->merge_renames, 0);
+
+   free(re_info->head_renames);
+   free(re_info->merge_renames);
+}
+
 static struct object_id *stage_oid(const struct object_id *oid, unsigned mode)
 {
return (is_null_oid(oid) || mode == 0) ? NULL: (struct object_id *)oid;
@@ -2004,7 +2030,8 @@ int merge_trees(struct merge_options *o,
}
 
if (unmerged_cache()) {
-   struct string_list *entries, *re_head, *re_merge;
+   struct string_list *entries;
+   struct rename_info re_info;
int i;
/*
 * Only need the hashmap while processing entries, so
@@ -2018,9 +2045,8 @@ int merge_trees(struct merge_options *o,
get_files_dirs(o, merge);
 
entries = get_unmerged();
-   re_head  = get_renames(o, head, common, head, merge, entries);
-   re_merge = get_renames(o, merge, common, head, merge, entries);
-   clean = process_renames(o, re_head, re_merge);
+   clean = handle_renames(o, common, head, merge, entries,
+  _info);
record_df_conflict_files(o, entries);
if (clean < 0)
goto cleanup;
@@ -2045,16 +2071,13 @@ int merge_trees(struct merge_options *o,
}
 
 cleanup:
-   string_list_clear(re_merge, 0);
-   string_list_clear(re_head, 0);
+   cleanup_renames(_info);
+
string_list_clear(entries, 1);
+   free(entries);
 
hashmap_free(>current_file_dir_set, 1);
 
-   free(re_merge);
-   free(re_head);
-   free(entries);
-
if (clean < 0)
return clean;
}
-- 
2.16.1.232.g28d5be9217

[PATCH v8 03/29] directory rename detection: testcases to avoid taking detection too far

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 153 
 1 file changed, 153 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index b22a9052b3..8049ed5fc9 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -582,4 +582,157 @@ test_expect_success '2b-check: Directory split into two 
on one side, with equal
 #   messages are handled correctly.
 ###
 
+
+###
+# SECTION 3: Path in question is the source path for some rename already
+#
+# Combining cases from Section 1 and trying to handle them could lead to
+# directory renaming detection being over-applied.  So, this section
+# provides some good testcases to check that the implementation doesn't go
+# too far.
+###
+
+# Testcase 3a, Avoid implicit rename if involved as source on other side
+#   (Related to testcases 1c and 1f)
+#   Commit O: z/{b,c,d}
+#   Commit A: z/{b,c,d} (no change)
+#   Commit B: y/{b,c}, x/d
+#   Expected: y/{b,c}, x/d
+test_expect_success '3a-setup: Avoid implicit rename if involved as source on 
other side' '
+   test_create_repo 3a &&
+   (
+   cd 3a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d >z/d &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   test_tick &&
+   git commit --allow-empty -m "A" &&
+
+   git checkout B &&
+   mkdir y &&
+   mkdir x &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   git mv z/d x/ &&
+   rmdir z &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '3a-check: Avoid implicit rename if involved as source on 
other side' '
+   (
+   cd 3a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:x/d &&
+   git rev-parse >expect \
+   O:z/bO:z/cO:z/d &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 3b, Avoid implicit rename if involved as source on other side
+#   (Related to testcases 5c and 7c, also kind of 1e and 1f)
+#   Commit O: z/{b,c,d}
+#   Commit A: y/{b,c}, x/d
+#   Commit B: z/{b,c}, w/d
+#   Expected: y/{b,c}, CONFLICT:(z/d -> x/d vs. w/d)
+#   NOTE: We're particularly checking that since z/d is already involved as
+# a source in a file rename on the same side of history, that we don't
+# get it involved in directory rename detection.  If it were, we might
+# end up with CONFLICT:(z/d -> y/d vs. x/d vs. w/d), i.e. a
+# rename/rename/rename(1to3) conflict, which is just weird.
+test_expect_success '3b-setup: Avoid implicit rename if involved as source on 
current side' '
+   test_create_repo 3b &&
+   (
+   cd 3b &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d >z/d &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   mkdir y &&
+   mkdir x &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   git mv z/d x/ &&
+   rmdir z &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   mkdir w &&
+   git mv z/d w/ &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '3b-check: Avoid implicit rename if involved as source on 
current side' '
+   (
+   cd 3b &&
+
+   git checkout A^0 &&
+
+   test_must_fail git merge -s recursive B^0 >out &&
+   test_i18ngrep CONFLICT.*rename/rename.*z/d.*x/d.*w/d out &&
+   test_i18ngrep ! CONFLICT.*rename/rename.*y/d out &&
+
+   git ls-files -s >out &&
+   test_line_count = 5 out &&
+   git ls-files -u >out &&
+   test_line_count = 3 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git 

[PATCH v8 29/29] merge-recursive: ensure we write updates for directory-renamed file

[Elijah Newren]

When a file is present in HEAD before the merge and the other side of the
merge does not modify that file, we try to avoid re-writing the file and
making it stat-dirty.  However, when a file is present in HEAD before the
merge and was in a directory that was renamed by the other side of the
merge, we have to move the file to a new location and re-write it.
Update the code that checks whether we can skip the update to also work in
the presence of directory renames.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 4 +---
 t/t6043-merge-rename-directories.sh | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 5063caabe1..ffe1d0d117 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2772,7 +2772,6 @@ static int merge_content(struct merge_options *o,
 
if (mfi.clean && !df_conflict_remains &&
oid_eq(, a_oid) && mfi.mode == a_mode) {
-   int path_renamed_outside_HEAD;
output(o, 3, _("Skipped %s (merged same as existing)"), path);
/*
 * The content merge resulted in the same file contents we
@@ -2780,8 +2779,7 @@ static int merge_content(struct merge_options *o,
 * are recorded at the correct path (which may not be true
 * if the merge involves a rename).
 */
-   path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
-   if (!path_renamed_outside_HEAD) {
+   if (was_tracked(path)) {
add_cacheinfo(o, mfi.mode, , path,
  0, (!o->call_depth), 0);
return mfi.clean;
diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 45f620633f..2e28f2908d 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3884,7 +3884,7 @@ test_expect_success '12b-setup: Moving one directory 
hierarchy into another' '
)
 '
 
-test_expect_failure '12b-check: Moving one directory hierarchy into another' '
+test_expect_success '12b-check: Moving one directory hierarchy into another' '
(
cd 12b &&
 
-- 
2.16.1.232.g28d5be9217

[PATCH v8 22/29] merge-recursive: when comparing files, don't include trees

[Elijah Newren]

get_renames() would look up stage data that already existed (populated
in get_unmerged(), taken from whatever unpack_trees() created), and if
it didn't exist, would call insert_stage_data() to create the necessary
entry for the given file.  The insert_stage_data() fallback becomes
much more important for directory rename detection, because that creates
a mechanism to have a file in the resulting merge that didn't exist on
either side of history.  However, insert_stage_data(), due to calling
get_tree_entry() loaded up trees as readily as files.  We aren't
interested in comparing trees to files; the D/F conflict handling is
done elsewhere.  This code is just concerned with what entries existed
for a given path on the different sides of the merge, so create a
get_tree_entry_if_blob() helper function and use it.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index efe9c9a98e..18f92be608 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -421,6 +421,21 @@ static void get_files_dirs(struct merge_options *o, struct 
tree *tree)
read_tree_recursive(tree, "", 0, 0, _all, save_files_dirs, o);
 }
 
+static int get_tree_entry_if_blob(const unsigned char *tree,
+ const char *path,
+ unsigned char *hashy,
+ unsigned int *mode_o)
+{
+   int ret;
+
+   ret = get_tree_entry(tree, path, hashy, mode_o);
+   if (S_ISDIR(*mode_o)) {
+   hashcpy(hashy, null_sha1);
+   *mode_o = 0;
+   }
+   return ret;
+}
+
 /*
  * Returns an index_entry instance which doesn't have to correspond to
  * a real cache entry in Git's index.
@@ -431,12 +446,12 @@ static struct stage_data *insert_stage_data(const char 
*path,
 {
struct string_list_item *item;
struct stage_data *e = xcalloc(1, sizeof(struct stage_data));
-   get_tree_entry(o->object.oid.hash, path,
-   e->stages[1].oid.hash, >stages[1].mode);
-   get_tree_entry(a->object.oid.hash, path,
-   e->stages[2].oid.hash, >stages[2].mode);
-   get_tree_entry(b->object.oid.hash, path,
-   e->stages[3].oid.hash, >stages[3].mode);
+   get_tree_entry_if_blob(o->object.oid.hash, path,
+  e->stages[1].oid.hash, >stages[1].mode);
+   get_tree_entry_if_blob(a->object.oid.hash, path,
+  e->stages[2].oid.hash, >stages[2].mode);
+   get_tree_entry_if_blob(b->object.oid.hash, path,
+  e->stages[3].oid.hash, >stages[3].mode);
item = string_list_insert(entries, path);
item->util = e;
return e;
-- 
2.16.1.232.g28d5be9217

[PATCH v8 06/29] directory rename detection: testcases checking which side did the rename

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 336 
 1 file changed, 336 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index b469c807c2..9ae245a522 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1180,4 +1180,340 @@ test_expect_failure '5d-check: Directory/file/file 
conflict due to directory ren
 #   back to old handling.  But, sadly, see testcases 8a and 8b.
 ###
 
+
+###
+# SECTION 6: Same side of the merge was the one that did the rename
+#
+# It may sound obvious that you only want to apply implicit directory
+# renames to directories if the _other_ side of history did the renaming.
+# If you did make an implementation that didn't explicitly enforce this
+# rule, the majority of cases that would fall under this section would
+# also be solved by following the rules from the above sections.  But
+# there are still a few that stick out, so this section covers them just
+# to make sure we also get them right.
+###
+
+# Testcase 6a, Tricky rename/delete
+#   Commit O: z/{b,c,d}
+#   Commit A: z/b
+#   Commit B: y/{b,c}, z/d
+#   Expected: y/b, CONFLICT(rename/delete, z/c -> y/c vs. NULL)
+#   Note: We're just checking here that the rename of z/b and z/c to put
+# them under y/ doesn't accidentally catch z/d and make it look like
+# it is also involved in a rename/delete conflict.
+
+test_expect_success '6a-setup: Tricky rename/delete' '
+   test_create_repo 6a &&
+   (
+   cd 6a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d >z/d &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git rm z/c &&
+   git rm z/d &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   mkdir y &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '6a-check: Tricky rename/delete' '
+   (
+   cd 6a &&
+
+   git checkout A^0 &&
+
+   test_must_fail git merge -s recursive B^0 >out &&
+   test_i18ngrep "CONFLICT (rename/delete).*z/c.*y/c" out &&
+
+   git ls-files -s >out &&
+   test_line_count = 2 out &&
+   git ls-files -u >out &&
+   test_line_count = 1 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   :0:y/b :3:y/c &&
+   git rev-parse >expect \
+O:z/b  O:z/c &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 6b, Same rename done on both sides
+#   (Related to testcases 6c and 8e)
+#   Commit O: z/{b,c}
+#   Commit A: y/{b,c}
+#   Commit B: y/{b,c}, z/d
+#   Expected: y/{b,c}, z/d
+#   Note: If we did directory rename detection here, we'd move z/d into y/,
+# but B did that rename and still decided to put the file into z/,
+# so we probably shouldn't apply directory rename detection for it.
+
+test_expect_success '6b-setup: Same rename done on both sides' '
+   test_create_repo 6b &&
+   (
+   cd 6b &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git mv z y &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv z y &&
+   mkdir z &&
+   echo d >z/d &&
+   git add z/d &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '6b-check: Same rename done on both sides' '
+   (
+   cd 6b &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+

[PATCH v8 09/29] directory rename detection: miscellaneous testcases to complete coverage

[Elijah Newren]

I came up with the testcases in the first eight sections before coding up
the implementation.  The testcases in this section were mostly ones I
thought of while coding/debugging, and which I was too lazy to insert
into the previous sections because I didn't want to re-label with all the
testcase references.  :-)

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 565 +++-
 1 file changed, 564 insertions(+), 1 deletion(-)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index e211e8ca31..cbbb949014 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -305,6 +305,7 @@ test_expect_failure '1d-check: Directory renames cause a 
rename/rename(2to1) con
 '
 
 # Testcase 1e, Renamed directory, with all filenames being renamed too
+#   (Related to testcases 9f & 9g)
 #   Commit O: z/{oldb,oldc}
 #   Commit A: y/{newb,newc}
 #   Commit B: z/{oldb,oldc,d}
@@ -593,7 +594,7 @@ test_expect_success '2b-check: Directory split into two on 
one side, with equal
 ###
 
 # Testcase 3a, Avoid implicit rename if involved as source on other side
-#   (Related to testcases 1c and 1f)
+#   (Related to testcases 1c, 1f, and 9h)
 #   Commit O: z/{b,c,d}
 #   Commit A: z/{b,c,d} (no change)
 #   Commit B: y/{b,c}, x/d
@@ -2316,4 +2317,566 @@ test_expect_failure '8e-check: Both sides rename, one 
side adds to original dire
)
 '
 
+###
+# SECTION 9: Other testcases
+#
+# This section consists of miscellaneous testcases I thought of during
+# the implementation which round out the testing.
+###
+
+# Testcase 9a, Inner renamed directory within outer renamed directory
+#   (Related to testcase 1f)
+#   Commit O: z/{b,c,d/{e,f,g}}
+#   Commit A: y/{b,c}, x/w/{e,f,g}
+#   Commit B: z/{b,c,d/{e,f,g,h},i}
+#   Expected: y/{b,c,i}, x/w/{e,f,g,h}
+#   NOTE: The only reason this one is interesting is because when a directory
+# is split into multiple other directories, we determine by the weight
+# of which one had the most paths going to it.  A naive implementation
+# of that could take the new file in commit B at z/i to x/w/i or x/i.
+
+test_expect_success '9a-setup: Inner renamed directory within outer renamed 
directory' '
+   test_create_repo 9a &&
+   (
+   cd 9a &&
+
+   mkdir -p z/d &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo e >z/d/e &&
+   echo f >z/d/f &&
+   echo g >z/d/g &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   mkdir x &&
+   git mv z/d x/w &&
+   git mv z y &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   echo h >z/d/h &&
+   echo i >z/i &&
+   git add z &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '9a-check: Inner renamed directory within outer renamed 
directory' '
+   (
+   cd 9a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 7 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/i &&
+   git rev-parse >expect \
+   O:z/bO:z/cB:z/i &&
+   test_cmp expect actual &&
+
+   git rev-parse >actual \
+   HEAD:x/w/e HEAD:x/w/f HEAD:x/w/g HEAD:x/w/h &&
+   git rev-parse >expect \
+   O:z/d/eO:z/d/fO:z/d/gB:z/d/h &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 9b, Transitive rename with content merge
+#   (Related to testcase 1c)
+#   Commit O: z/{b,c},   x/d_1
+#   Commit A: y/{b,c},   x/d_2
+#   Commit B: z/{b,c,d_3}
+#   Expected: y/{b,c,d_merged}
+
+test_expect_success '9b-setup: Transitive rename with content merge' '
+   test_create_repo 9b &&
+   (
+   cd 9b &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   mkdir x &&
+   test_seq 1 10 >x/d &&
+   git add z x &&
+   test_tick &&
+   git commit -m "O" &&
+
+  

[PATCH v8 04/29] directory rename detection: partially renamed directory testcase/discussion

[Elijah Newren]

Add a long note about why we are not considering "partial directory
renames" for the current directory rename detection implementation.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 115 
 1 file changed, 115 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 8049ed5fc9..713ad2b75e 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -735,4 +735,119 @@ test_expect_success '3b-check: Avoid implicit rename if 
involved as source on cu
 #   of a rename on either side of a merge.
 ###
 
+
+###
+# SECTION 4: Partially renamed directory; still exists on both sides of merge
+#
+# What if we were to attempt to do directory rename detection when someone
+# "mostly" moved a directory but still left some files around, or,
+# equivalently, fully renamed a directory in one commmit and then recreated
+# that directory in a later commit adding some new files and then tried to
+# merge?
+#
+# It's hard to divine user intent in these cases, because you can make an
+# argument that, depending on the intermediate history of the side being
+# merged, that some users will want files in that directory to
+# automatically be detected and renamed, while users with a different
+# intermediate history wouldn't want that rename to happen.
+#
+# I think that it is best to simply not have directory rename detection
+# apply to such cases.  My reasoning for this is four-fold: (1) it's
+# easiest for users in general to figure out what happened if we don't
+# apply directory rename detection in any such case, (2) it's an easy rule
+# to explain ["We don't do directory rename detection if the directory
+# still exists on both sides of the merge"], (3) we can get some hairy
+# edge/corner cases that would be really confusing and possibly not even
+# representable in the index if we were to even try, and [related to 3] (4)
+# attempting to resolve this issue of divining user intent by examining
+# intermediate history goes against the spirit of three-way merges and is a
+# path towards crazy corner cases that are far more complex than what we're
+# already dealing with.
+#
+# Note that the wording of the rule ("We don't do directory rename
+# detection if the directory still exists on both sides of the merge.")
+# also excludes "renaming" of a directory into a subdirectory of itself
+# (e.g. /some/dir/* -> /some/dir/subdir/*).  It may be possible to carve
+# out an exception for "renaming"-beneath-itself cases without opening
+# weird edge/corner cases for other partial directory renames, but for now
+# we are keeping the rule simple.
+#
+# This section contains a test for a partially-renamed-directory case.
+###
+
+# Testcase 4a, Directory split, with original directory still present
+#   (Related to testcase 1f)
+#   Commit O: z/{b,c,d,e}
+#   Commit A: y/{b,c,d}, z/e
+#   Commit B: z/{b,c,d,e,f}
+#   Expected: y/{b,c,d}, z/{e,f}
+#   NOTE: Even though most files from z moved to y, we don't want f to follow.
+
+test_expect_success '4a-setup: Directory split, with original directory still 
present' '
+   test_create_repo 4a &&
+   (
+   cd 4a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d >z/d &&
+   echo e >z/e &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   mkdir y &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   git mv z/d y/ &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   echo f >z/f &&
+   git add z/f &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '4a-check: Directory split, with original directory still 
present' '
+   (
+   cd 4a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 5 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/d HEAD:z/e HEAD:z/f &&
+   git rev-parse >expect \
+   O:z/bO:z/cO:z/dO:z/eB:z/f &&
+   test_cmp 

[PATCH v8 15/29] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over.  It seems more straightforward to simply avoid calling
either function in that case.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1986af79a9..4e6d0c248e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1338,8 +1338,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-   if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1657,6 +1655,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1667,6 +1671,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.16.1.232.g28d5be9217

[PATCH v8 12/29] merge-recursive: move the get_renames() function

[Elijah Newren]

Move this function so it can re-use some others (without either
moving all of them or adding an annoying split between function
declarations and definitions).  Cheat slightly by adding a blank line
for readability, and in order to silence checkpatch.pl.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 139 +++---
 1 file changed, 70 insertions(+), 69 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 9d53f30111..2028dd113b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -537,75 +537,6 @@ struct rename {
unsigned processed:1;
 };
 
-/*
- * Get information of all renames which occurred between 'o_tree' and
- * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and
- * 'b_tree') to be able to associate the correct cache entries with
- * the rename information. 'tree' is always equal to either a_tree or b_tree.
- */
-static struct string_list *get_renames(struct merge_options *o,
-  struct tree *tree,
-  struct tree *o_tree,
-  struct tree *a_tree,
-  struct tree *b_tree,
-  struct string_list *entries)
-{
-   int i;
-   struct string_list *renames;
-   struct diff_options opts;
-
-   renames = xcalloc(1, sizeof(struct string_list));
-   if (!o->detect_rename)
-   return renames;
-
-   diff_setup();
-   opts.flags.recursive = 1;
-   opts.flags.rename_empty = 0;
-   opts.detect_rename = DIFF_DETECT_RENAME;
-   opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
-   o->diff_rename_limit >= 0 ? o->diff_rename_limit :
-   1000;
-   opts.rename_score = o->rename_score;
-   opts.show_rename_progress = o->show_rename_progress;
-   opts.output_format = DIFF_FORMAT_NO_OUTPUT;
-   diff_setup_done();
-   diff_tree_oid(_tree->object.oid, >object.oid, "", );
-   diffcore_std();
-   if (opts.needed_rename_limit > o->needed_rename_limit)
-   o->needed_rename_limit = opts.needed_rename_limit;
-   for (i = 0; i < diff_queued_diff.nr; ++i) {
-   struct string_list_item *item;
-   struct rename *re;
-   struct diff_filepair *pair = diff_queued_diff.queue[i];
-   if (pair->status != 'R') {
-   diff_free_filepair(pair);
-   continue;
-   }
-   re = xmalloc(sizeof(*re));
-   re->processed = 0;
-   re->pair = pair;
-   item = string_list_lookup(entries, re->pair->one->path);
-   if (!item)
-   re->src_entry = insert_stage_data(re->pair->one->path,
-   o_tree, a_tree, b_tree, entries);
-   else
-   re->src_entry = item->util;
-
-   item = string_list_lookup(entries, re->pair->two->path);
-   if (!item)
-   re->dst_entry = insert_stage_data(re->pair->two->path,
-   o_tree, a_tree, b_tree, entries);
-   else
-   re->dst_entry = item->util;
-   item = string_list_insert(renames, pair->one->path);
-   item->util = re;
-   }
-   opts.output_format = DIFF_FORMAT_NO_OUTPUT;
-   diff_queued_diff.nr = 0;
-   diff_flush();
-   return renames;
-}
-
 static int update_stages(struct merge_options *opt, const char *path,
 const struct diff_filespec *o,
 const struct diff_filespec *a,
@@ -1389,6 +1320,76 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
return ret;
 }
 
+/*
+ * Get information of all renames which occurred between 'o_tree' and
+ * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and
+ * 'b_tree') to be able to associate the correct cache entries with
+ * the rename information. 'tree' is always equal to either a_tree or b_tree.
+ */
+static struct string_list *get_renames(struct merge_options *o,
+  struct tree *tree,
+  struct tree *o_tree,
+  struct tree *a_tree,
+  struct tree *b_tree,
+  struct string_list *entries)
+{
+   int i;
+   struct string_list *renames;
+   struct diff_options opts;
+
+   renames = xcalloc(1, sizeof(struct string_list));
+   if (!o->detect_rename)
+   return renames;
+
+   diff_setup();
+   opts.flags.recursive = 1;
+   opts.flags.rename_empty = 0;
+   opts.detect_rename = 

[PATCH v8 16/29] merge-recursive: split out code for determining diff_filepairs

[Elijah Newren]

Create a new function, get_diffpairs() to compute the diff_filepairs
between two trees.  While these are currently only used in
get_renames(), I want them to be available to some new functions.  No
actual logic changes yet.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 84 ---
 1 file changed, 62 insertions(+), 22 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 4e6d0c248e..db8833675e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1321,24 +1321,15 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
 }
 
 /*
- * Get information of all renames which occurred between 'o_tree' and
- * 'tree'. We need the three trees in the merge ('o_tree', 'a_tree' and
- * 'b_tree') to be able to associate the correct cache entries with
- * the rename information. 'tree' is always equal to either a_tree or b_tree.
+ * Get the diff_filepairs changed between o_tree and tree.
  */
-static struct string_list *get_renames(struct merge_options *o,
-  struct tree *tree,
-  struct tree *o_tree,
-  struct tree *a_tree,
-  struct tree *b_tree,
-  struct string_list *entries)
+static struct diff_queue_struct *get_diffpairs(struct merge_options *o,
+  struct tree *o_tree,
+  struct tree *tree)
 {
-   int i;
-   struct string_list *renames;
+   struct diff_queue_struct *ret;
struct diff_options opts;
 
-   renames = xcalloc(1, sizeof(struct string_list));
-
diff_setup();
opts.flags.recursive = 1;
opts.flags.rename_empty = 0;
@@ -1354,10 +1345,41 @@ static struct string_list *get_renames(struct 
merge_options *o,
diffcore_std();
if (opts.needed_rename_limit > o->needed_rename_limit)
o->needed_rename_limit = opts.needed_rename_limit;
-   for (i = 0; i < diff_queued_diff.nr; ++i) {
+
+   ret = xmalloc(sizeof(*ret));
+   *ret = diff_queued_diff;
+
+   opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+   diff_queued_diff.nr = 0;
+   diff_queued_diff.queue = NULL;
+   diff_flush();
+   return ret;
+}
+
+/*
+ * Get information of all renames which occurred in 'pairs', making use of
+ * any implicit directory renames inferred from the other side of history.
+ * We need the three trees in the merge ('o_tree', 'a_tree' and 'b_tree')
+ * to be able to associate the correct cache entries with the rename
+ * information; tree is always equal to either a_tree or b_tree.
+ */
+static struct string_list *get_renames(struct merge_options *o,
+  struct diff_queue_struct *pairs,
+  struct tree *tree,
+  struct tree *o_tree,
+  struct tree *a_tree,
+  struct tree *b_tree,
+  struct string_list *entries)
+{
+   int i;
+   struct string_list *renames;
+
+   renames = xcalloc(1, sizeof(struct string_list));
+
+   for (i = 0; i < pairs->nr; ++i) {
struct string_list_item *item;
struct rename *re;
-   struct diff_filepair *pair = diff_queued_diff.queue[i];
+   struct diff_filepair *pair = pairs->queue[i];
 
if (pair->status != 'R') {
diff_free_filepair(pair);
@@ -1382,9 +1404,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
item = string_list_insert(renames, pair->one->path);
item->util = re;
}
-   opts.output_format = DIFF_FORMAT_NO_OUTPUT;
-   diff_queued_diff.nr = 0;
-   diff_flush();
return renames;
 }
 
@@ -1655,15 +1674,36 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   struct diff_queue_struct *head_pairs, *merge_pairs;
+   int clean;
+
ri->head_renames = NULL;
ri->merge_renames = NULL;
 
if (!o->detect_rename)
return 1;
 
-   ri->head_renames  = get_renames(o, head, common, head, merge, entries);
-   ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
-   return process_renames(o, ri->head_renames, ri->merge_renames);
+   head_pairs = get_diffpairs(o, common, head);
+   merge_pairs = get_diffpairs(o, common, merge);
+
+   ri->head_renames  = get_renames(o, head_pairs, head,
+common, head, merge, entries);
+   ri->merge_renames = get_renames(o, merge_pairs, merge,

[PATCH v8 11/29] directory rename detection: tests for handling overwriting dirty files

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 458 
 1 file changed, 458 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index a6cd38336c..8ea9ec49bc 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3246,4 +3246,462 @@ test_expect_failure '10e-check: Does git complain about 
untracked file that is n
)
 '
 
+###
+# SECTION 11: Handling dirty (not up-to-date) files
+#
+# unpack_trees(), upon which the recursive merge algorithm is based, aborts
+# the operation if untracked or dirty files would be deleted or overwritten
+# by the merge.  Unfortunately, unpack_trees() does not understand renames,
+# and if it doesn't abort, then it muddies up the working directory before
+# we even get to the point of detecting renames, so we need some special
+# handling.  This was true even of normal renames, but there are additional
+# codepaths that need special handling with directory renames.  Add
+# testcases for both renamed-by-directory-rename-detection and standard
+# rename cases.
+###
+
+# Testcase 11a, Avoid losing dirty contents with simple rename
+#   Commit O: z/{a,b_v1},
+#   Commit A: z/{a,c_v1}, and z/c_v1 has uncommitted mods
+#   Commit B: z/{a,b_v2}
+#   Expected: ERROR_MSG(Refusing to lose dirty file at z/c) +
+# z/a, staged version of z/c has sha1sum matching B:z/b_v2,
+# z/c~HEAD with contents of B:z/b_v2,
+# z/c with uncommitted mods on top of A:z/c_v1
+
+test_expect_success '11a-setup: Avoid losing dirty contents with simple 
rename' '
+   test_create_repo 11a &&
+   (
+   cd 11a &&
+
+   mkdir z &&
+   echo a >z/a &&
+   test_seq 1 10 >z/b &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git mv z/b z/c &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   echo 11 >>z/b &&
+   git add z/b &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '11a-check: Avoid losing dirty contents with simple 
rename' '
+   (
+   cd 11a &&
+
+   git checkout A^0 &&
+   echo stuff >>z/c &&
+
+   test_must_fail git merge -s recursive B^0 >out 2>err &&
+   test_i18ngrep "Refusing to lose dirty file at z/c" out &&
+
+   test_seq 1 10 >expected &&
+   echo stuff >>expected &&
+   test_cmp expected z/c &&
+
+   git ls-files -s >out &&
+   test_line_count = 2 out &&
+   git ls-files -u >out &&
+   test_line_count = 1 out &&
+   git ls-files -o >out &&
+   test_line_count = 4 out &&
+
+   git rev-parse >actual \
+   :0:z/a :2:z/c &&
+   git rev-parse >expect \
+O:z/a  B:z/b &&
+   test_cmp expect actual &&
+
+   git hash-object z/c~HEAD >actual &&
+   git rev-parse B:z/b >expect &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 11b, Avoid losing dirty file involved in directory rename
+#   Commit O: z/a, x/{b,c_v1}
+#   Commit A: z/{a,c_v1},  x/b,   and z/c_v1 has uncommitted mods
+#   Commit B: y/a, x/{b,c_v2}
+#   Expected: y/{a,c_v2}, x/b, z/c_v1 with uncommitted mods untracked,
+# ERROR_MSG(Refusing to lose dirty file at z/c)
+
+
+test_expect_success '11b-setup: Avoid losing dirty file involved in directory 
rename' '
+   test_create_repo 11b &&
+   (
+   cd 11b &&
+
+   mkdir z x &&
+   echo a >z/a &&
+   echo b >x/b &&
+   test_seq 1 10 >x/c &&
+   git add z x &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git mv x/c z/c &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv z y &&
+   echo 11 >>x/c &&
+   git add x/c &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '11b-check: Avoid losing dirty file involved in directory 
rename' '
+   (
+   cd 11b &&
+
+   git checkout A^0 &&
+ 

[PATCH v8 01/29] directory rename detection: basic testcases

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 442 
 1 file changed, 442 insertions(+)
 create mode 100755 t/t6043-merge-rename-directories.sh

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
new file mode 100755
index 00..d045f0e31e
--- /dev/null
+++ b/t/t6043-merge-rename-directories.sh
@@ -0,0 +1,442 @@
+#!/bin/sh
+
+test_description="recursive merge with directory renames"
+# includes checking of many corner cases, with a similar methodology to:
+#   t6042: corner cases with renames but not criss-cross merges
+#   t6036: corner cases with both renames and criss-cross merges
+#
+# The setup for all of them, pictorially, is:
+#
+#  A
+#  o
+# / \
+#  O o   ?
+# \ /
+#  o
+#  B
+#
+# To help make it easier to follow the flow of tests, they have been
+# divided into sections and each test will start with a quick explanation
+# of what commits O, A, and B contain.
+#
+# Notation:
+#z/{b,c}   means  files z/b and z/c both exist
+#x/d_1 means  file x/d exists with content d1.  (Purpose of the
+# underscore notation is to differentiate different
+# files that might be renamed into each other's paths.)
+
+. ./test-lib.sh
+
+
+###
+# SECTION 1: Basic cases we should be able to handle
+###
+
+# Testcase 1a, Basic directory rename.
+#   Commit O: z/{b,c}
+#   Commit A: y/{b,c}
+#   Commit B: z/{b,c,d,e/f}
+#   Expected: y/{b,c,d,e/f}
+
+test_expect_success '1a-setup: Simple directory rename detection' '
+   test_create_repo 1a &&
+   (
+   cd 1a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git mv z y &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   echo d >z/d &&
+   mkdir z/e &&
+   echo f >z/e/f &&
+   git add z/d z/e/f &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '1a-check: Simple directory rename detection' '
+   (
+   cd 1a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 4 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/d HEAD:y/e/f &&
+   git rev-parse >expect \
+   O:z/bO:z/cB:z/dB:z/e/f &&
+   test_cmp expect actual &&
+
+   git hash-object y/d >actual &&
+   git rev-parse B:z/d >expect &&
+   test_cmp expect actual &&
+
+   test_must_fail git rev-parse HEAD:z/d &&
+   test_must_fail git rev-parse HEAD:z/e/f &&
+   test_path_is_missing z/d &&
+   test_path_is_missing z/e/f
+   )
+'
+
+# Testcase 1b, Merge a directory with another
+#   Commit O: z/{b,c},   y/d
+#   Commit A: z/{b,c,e}, y/d
+#   Commit B: y/{b,c,d}
+#   Expected: y/{b,c,d,e}
+
+test_expect_success '1b-setup: Merge a directory with another' '
+   test_create_repo 1b &&
+   (
+   cd 1b &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   mkdir y &&
+   echo d >y/d &&
+   git add z y &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   echo e >z/e &&
+   git add z/e &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv z/b y &&
+   git mv z/c y &&
+   rmdir z &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '1b-check: Merge a directory with another' '
+   (
+   cd 1b &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 4 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/d HEAD:y/e &&
+   git rev-parse >expect \
+   O:z/bO:z/cO:y/dA:z/e &&
+   test_cmp expect actual &&
+   test_must_fail git rev-parse 

[PATCH v8 05/29] directory rename detection: files/directories in the way of some renames

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 330 
 1 file changed, 330 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 713ad2b75e..b469c807c2 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -850,4 +850,334 @@ test_expect_success '4a-check: Directory split, with 
original directory still pr
 #   detection.)  But, sadly, see testcase 8b.
 ###
 
+
+###
+# SECTION 5: Files/directories in the way of subset of to-be-renamed paths
+#
+# Implicitly renaming files due to a detected directory rename could run
+# into problems if there are files or directories in the way of the paths
+# we want to rename.  Explore such cases in this section.
+###
+
+# Testcase 5a, Merge directories, other side adds files to original and target
+#   Commit O: z/{b,c},   y/d
+#   Commit A: z/{b,c,e_1,f}, y/{d,e_2}
+#   Commit B: y/{b,c,d}
+#   Expected: z/e_1, y/{b,c,d,e_2,f} + CONFLICT warning
+#   NOTE: While directory rename detection is active here causing z/f to
+# become y/f, we did not apply this for z/e_1 because that would
+# give us an add/add conflict for y/e_1 vs y/e_2.  This problem with
+# this add/add, is that both versions of y/e are from the same side
+# of history, giving us no way to represent this conflict in the
+# index.
+
+test_expect_success '5a-setup: Merge directories, other side adds files to 
original and target' '
+   test_create_repo 5a &&
+   (
+   cd 5a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   mkdir y &&
+   echo d >y/d &&
+   git add z y &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   echo e1 >z/e &&
+   echo f >z/f &&
+   echo e2 >y/e &&
+   git add z/e z/f y/e &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   rmdir z &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '5a-check: Merge directories, other side adds files to 
original and target' '
+   (
+   cd 5a &&
+
+   git checkout A^0 &&
+
+   test_must_fail git merge -s recursive B^0 >out &&
+   test_i18ngrep "CONFLICT.*implicit dir rename" out &&
+
+   git ls-files -s >out &&
+   test_line_count = 6 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   :0:y/b :0:y/c :0:y/d :0:y/e :0:z/e :0:y/f &&
+   git rev-parse >expect \
+O:z/b  O:z/c  O:y/d  A:y/e  A:z/e  A:z/f &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 5b, Rename/delete in order to get add/add/add conflict
+#   (Related to testcase 8d; these may appear slightly inconsistent to users;
+#Also related to testcases 7d and 7e)
+#   Commit O: z/{b,c,d_1}
+#   Commit A: y/{b,c,d_2}
+#   Commit B: z/{b,c,d_1,e}, y/d_3
+#   Expected: y/{b,c,e}, CONFLICT(add/add: y/d_2 vs. y/d_3)
+#   NOTE: If z/d_1 in commit B were to be involved in dir rename detection, as
+# we normaly would since z/ is being renamed to y/, then this would be
+# a rename/delete (z/d_1 -> y/d_1 vs. deleted) AND an add/add/add
+# conflict of y/d_1 vs. y/d_2 vs. y/d_3.  Add/add/add is not
+# representable in the index, so the existence of y/d_3 needs to
+# cause us to bail on directory rename detection for that path, falling
+# back to git behavior without the directory rename detection.
+
+test_expect_success '5b-setup: Rename/delete in order to get add/add/add 
conflict' '
+   test_create_repo 5b &&
+   (
+   cd 5b &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d1 >z/d &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   git rm z/d &&
+   git mv z y &&
+   echo d2 >y/d &&
+   

[PATCH v8 18/29] merge-recursive: add get_directory_renames()

[Elijah Newren]

This populates a set of directory renames for us.  The set of directory
renames is not yet used, but will be in subsequent commits.

Note that the use of a string_list for possible_new_dirs in the new
dir_rename_entry struct implies an O(n^2) algorithm; however, in practice
I expect the number of distinct directories that files were renamed into
from a single original directory to be O(1).  My guess is that n has a
mode of 1 and a mean of less than 2, so, for now, string_list seems good
enough for possible_new_dirs.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 224 +-
 merge-recursive.h |  18 +
 2 files changed, 239 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index be20660527..aca4acdfcb 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -49,6 +49,44 @@ static unsigned int path_hash(const char *path)
return ignore_case ? strihash(path) : strhash(path);
 }
 
+static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
+ char *dir)
+{
+   struct dir_rename_entry key;
+
+   if (dir == NULL)
+   return NULL;
+   hashmap_entry_init(, strhash(dir));
+   key.dir = dir;
+   return hashmap_get(hashmap, , NULL);
+}
+
+static int dir_rename_cmp(const void *unused_cmp_data,
+ const void *entry,
+ const void *entry_or_key,
+ const void *unused_keydata)
+{
+   const struct dir_rename_entry *e1 = entry;
+   const struct dir_rename_entry *e2 = entry_or_key;
+
+   return strcmp(e1->dir, e2->dir);
+}
+
+static void dir_rename_init(struct hashmap *map)
+{
+   hashmap_init(map, dir_rename_cmp, NULL, 0);
+}
+
+static void dir_rename_entry_init(struct dir_rename_entry *entry,
+ char *directory)
+{
+   hashmap_entry_init(entry, strhash(directory));
+   entry->dir = directory;
+   entry->non_unique_new_dir = 0;
+   strbuf_init(>new_dir, 0);
+   string_list_init(>possible_new_dirs, 0);
+}
+
 static void flush_output(struct merge_options *o)
 {
if (o->buffer_output < 2 && o->obuf.len) {
@@ -1356,6 +1394,169 @@ static struct diff_queue_struct *get_diffpairs(struct 
merge_options *o,
return ret;
 }
 
+static void get_renamed_dir_portion(const char *old_path, const char *new_path,
+   char **old_dir, char **new_dir)
+{
+   char *end_of_old, *end_of_new;
+   int old_len, new_len;
+
+   *old_dir = NULL;
+   *new_dir = NULL;
+
+   /*
+* For
+*"a/b/c/d/e/foo.c" -> "a/b/some/thing/else/e/foo.c"
+* the "e/foo.c" part is the same, we just want to know that
+*"a/b/c/d" was renamed to "a/b/some/thing/else"
+* so, for this example, this function returns "a/b/c/d" in
+* *old_dir and "a/b/some/thing/else" in *new_dir.
+*
+* Also, if the basename of the file changed, we don't care.  We
+* want to know which portion of the directory, if any, changed.
+*/
+   end_of_old = strrchr(old_path, '/');
+   end_of_new = strrchr(new_path, '/');
+
+   if (end_of_old == NULL || end_of_new == NULL)
+   return;
+   while (*--end_of_new == *--end_of_old &&
+  end_of_old != old_path &&
+  end_of_new != new_path)
+   ; /* Do nothing; all in the while loop */
+   /*
+* We've found the first non-matching character in the directory
+* paths.  That means the current directory we were comparing
+* represents the rename.  Move end_of_old and end_of_new back
+* to the full directory name.
+*/
+   if (*end_of_old == '/')
+   end_of_old++;
+   if (*end_of_old != '/')
+   end_of_new++;
+   end_of_old = strchr(end_of_old, '/');
+   end_of_new = strchr(end_of_new, '/');
+
+   /*
+* It may have been the case that old_path and new_path were the same
+* directory all along.  Don't claim a rename if they're the same.
+*/
+   old_len = end_of_old - old_path;
+   new_len = end_of_new - new_path;
+
+   if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
+   *old_dir = xstrndup(old_path, old_len);
+   *new_dir = xstrndup(new_path, new_len);
+   }
+}
+
+static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
+struct tree *tree)
+{
+   struct hashmap *dir_renames;
+   struct hashmap_iter iter;
+   struct dir_rename_entry *entry;
+   int i;
+
+   /*
+* Typically, we think of a directory rename as all files from a
+* certain directory being moved to a target directory.  However,
+* 

[PATCH v8 26/29] merge-recursive: fix remaining directory rename + dirty overwrite cases

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c   | 25 ++---
 t/t6043-merge-rename-directories.sh |  8 
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 4532b982a4..6ff54c0e86 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1323,11 +1323,22 @@ static int handle_file(struct merge_options *o,
 
add = filespec_from_entry(, dst_entry, stage ^ 1);
if (add) {
+   int ren_src_was_dirty = was_dirty(o, rename->path);
char *add_name = unique_path(o, rename->path, other_branch);
if (update_file(o, 0, >oid, add->mode, add_name))
return -1;
 
-   remove_file(o, 0, rename->path, 0);
+   if (ren_src_was_dirty) {
+   output(o, 1, _("Refusing to lose dirty file at %s"),
+  rename->path);
+   }
+   /*
+* Because the double negatives somehow keep confusing me...
+*1) update_wd iff !ren_src_was_dirty.
+*2) no_wd iff !update_wd
+*3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty
+*/
+   remove_file(o, 0, rename->path, ren_src_was_dirty);
dst_name = unique_path(o, rename->path, cur_branch);
} else {
if (dir_in_way(rename->path, !o->call_depth, 0)) {
@@ -1465,7 +1476,10 @@ static int conflict_rename_rename_2to1(struct 
merge_options *o,
char *new_path2 = unique_path(o, path, ci->branch2);
output(o, 1, _("Renaming %s to %s and %s to %s instead"),
   a->path, new_path1, b->path, new_path2);
-   if (would_lose_untracked(path))
+   if (was_dirty(o, path))
+   output(o, 1, _("Refusing to lose dirty file at %s"),
+  path);
+   else if (would_lose_untracked(path))
/*
 * Only way we get here is if both renames were from
 * a directory rename AND user had an untracked file
@@ -2074,6 +2088,7 @@ static void apply_directory_rename_modifications(struct 
merge_options *o,
 {
struct string_list_item *item;
int stage = (tree == a_tree ? 2 : 3);
+   int update_wd;
 
/*
 * In all cases where we can do directory rename detection,
@@ -2084,7 +2099,11 @@ static void apply_directory_rename_modifications(struct 
merge_options *o,
 * saying the file would have been overwritten), but it might
 * be dirty, though.
 */
-   remove_file(o, 1, pair->two->path, 0 /* no_wd */);
+   update_wd = !was_dirty(o, pair->two->path);
+   if (!update_wd)
+   output(o, 1, _("Refusing to lose dirty file at %s"),
+  pair->two->path);
+   remove_file(o, 1, pair->two->path, !update_wd);
 
/* Find or create a new re->dst_entry */
item = string_list_lookup(entries, new_path);
diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index b94ba066fe..33e2649824 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3370,7 +3370,7 @@ test_expect_success '11b-setup: Avoid losing dirty file 
involved in directory re
)
 '
 
-test_expect_failure '11b-check: Avoid losing dirty file involved in directory 
rename' '
+test_expect_success '11b-check: Avoid losing dirty file involved in directory 
rename' '
(
cd 11b &&
 
@@ -3512,7 +3512,7 @@ test_expect_success '11d-setup: Avoid losing not-uptodate 
with rename + D/F conf
)
 '
 
-test_expect_failure '11d-check: Avoid losing not-uptodate with rename + D/F 
conflict' '
+test_expect_success '11d-check: Avoid losing not-uptodate with rename + D/F 
conflict' '
(
cd 11d &&
 
@@ -3591,7 +3591,7 @@ test_expect_success '11e-setup: Avoid deleting 
not-uptodate with dir rename/rena
)
 '
 
-test_expect_failure '11e-check: Avoid deleting not-uptodate with dir 
rename/rename(1to2)/add' '
+test_expect_success '11e-check: Avoid deleting not-uptodate with dir 
rename/rename(1to2)/add' '
(
cd 11e &&
 
@@ -3667,7 +3667,7 @@ test_expect_success '11f-setup: Avoid deleting 
not-uptodate with dir rename/rena
)
 '
 
-test_expect_failure '11f-check: Avoid deleting not-uptodate with dir 
rename/rename(2to1)' '
+test_expect_success '11f-check: Avoid deleting not-uptodate with dir 
rename/rename(2to1)' '
(
cd 11f &&
 
-- 
2.16.1.232.g28d5be9217

[PATCH v8 19/29] merge-recursive: check for directory level conflicts

[Elijah Newren]

Before trying to apply directory renames to paths within the given
directories, we want to make sure that there aren't conflicts at the
directory level.  There will be additional checks at the individual
file level too, which will be added later.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 119 ++
 1 file changed, 119 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index aca4acdfcb..a9f1579d22 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1394,6 +1394,15 @@ static struct diff_queue_struct *get_diffpairs(struct 
merge_options *o,
return ret;
 }
 
+static int tree_has_path(struct tree *tree, const char *path)
+{
+   unsigned char hashy[GIT_MAX_RAWSZ];
+   unsigned int mode_o;
+
+   return !get_tree_entry(tree->object.oid.hash, path,
+  hashy, _o);
+}
+
 static void get_renamed_dir_portion(const char *old_path, const char *new_path,
char **old_dir, char **new_dir)
 {
@@ -1449,6 +1458,112 @@ static void get_renamed_dir_portion(const char 
*old_path, const char *new_path,
}
 }
 
+static void remove_hashmap_entries(struct hashmap *dir_renames,
+  struct string_list *items_to_remove)
+{
+   int i;
+   struct dir_rename_entry *entry;
+
+   for (i = 0; i < items_to_remove->nr; i++) {
+   entry = items_to_remove->items[i].util;
+   hashmap_remove(dir_renames, entry, NULL);
+   }
+   string_list_clear(items_to_remove, 0);
+}
+
+/*
+ * There are a couple things we want to do at the directory level:
+ *   1. Check for both sides renaming to the same thing, in order to avoid
+ *  implicit renaming of files that should be left in place.  (See
+ *  testcase 6b in t6043 for details.)
+ *   2. Prune directory renames if there are still files left in the
+ *  the original directory.  These represent a partial directory rename,
+ *  i.e. a rename where only some of the files within the directory
+ *  were renamed elsewhere.  (Technically, this could be done earlier
+ *  in get_directory_renames(), except that would prevent us from
+ *  doing the previous check and thus failing testcase 6b.)
+ *   3. Check for rename/rename(1to2) conflicts (at the directory level).
+ *  In the future, we could potentially record this info as well and
+ *  omit reporting rename/rename(1to2) conflicts for each path within
+ *  the affected directories, thus cleaning up the merge output.
+ *   NOTE: We do NOT check for rename/rename(2to1) conflicts at the
+ * directory level, because merging directories is fine.  If it
+ * causes conflicts for files within those merged directories, then
+ * that should be detected at the individual path level.
+ */
+static void handle_directory_level_conflicts(struct merge_options *o,
+struct hashmap *dir_re_head,
+struct tree *head,
+struct hashmap *dir_re_merge,
+struct tree *merge)
+{
+   struct hashmap_iter iter;
+   struct dir_rename_entry *head_ent;
+   struct dir_rename_entry *merge_ent;
+
+   struct string_list remove_from_head = STRING_LIST_INIT_NODUP;
+   struct string_list remove_from_merge = STRING_LIST_INIT_NODUP;
+
+   hashmap_iter_init(dir_re_head, );
+   while ((head_ent = hashmap_iter_next())) {
+   merge_ent = dir_rename_find_entry(dir_re_merge, head_ent->dir);
+   if (merge_ent &&
+   !head_ent->non_unique_new_dir &&
+   !merge_ent->non_unique_new_dir &&
+   !strbuf_cmp(_ent->new_dir, _ent->new_dir)) {
+   /* 1. Renamed identically; remove it from both sides */
+   string_list_append(_from_head,
+  head_ent->dir)->util = head_ent;
+   strbuf_release(_ent->new_dir);
+   string_list_append(_from_merge,
+  merge_ent->dir)->util = merge_ent;
+   strbuf_release(_ent->new_dir);
+   } else if (tree_has_path(head, head_ent->dir)) {
+   /* 2. This wasn't a directory rename after all */
+   string_list_append(_from_head,
+  head_ent->dir)->util = head_ent;
+   strbuf_release(_ent->new_dir);
+   }
+   }
+
+   remove_hashmap_entries(dir_re_head, _from_head);
+   remove_hashmap_entries(dir_re_merge, _from_merge);
+
+   hashmap_iter_init(dir_re_merge, );
+   while ((merge_ent = hashmap_iter_next())) {
+   head_ent = 

[PATCH v8 00/29] Add directory rename detection to git

[Elijah Newren]

This patchset introduces directory rename detection to merge-recursive.  See
  https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
for the first series (including design considerations, etc.)  This series
continues to depend on en/merge-recursive-fixes in next, at least
contextually.  For the curious, follow-up series and comments can also be
found at
  https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/
  https://public-inbox.org/git/20171121080059.32304-1-new...@gmail.com/
  https://public-inbox.org/git/20171129014237.32570-1-new...@gmail.com/
  https://public-inbox.org/git/20171228041352.27880-1-new...@gmail.com/
  https://public-inbox.org/git/20180105202711.24311-1-new...@gmail.com/
  https://public-inbox.org/git/20180130232533.25846-1-new...@gmail.com/

Also, as a reminder, this series fixes a few bugs somewhat as a side effect:
  * a bug causing dirty files involved in a rename to be overwritten
  * a few memory leaks

Changes since v7 (full tbdiff follows below):
  * Added Stefan's Reviewed-by.
  * Squashed commits introducing new hash structs and associated functions
into the commit that used them to avoid unused function
warnings/errors.
  * Added or clarified a number of comments where things were unclear
  * Minor stuff:
* Style (and typo) fixes for commit message and comments
* Avoiding casting with hash initialization function
* s/malloc/xmalloc/
* struct assignment
* s/20/GIT_MAX_RAWSZ/

Elijah Newren (29):
  directory rename detection: basic testcases
  directory rename detection: directory splitting testcases
  directory rename detection: testcases to avoid taking detection too
far
  directory rename detection: partially renamed directory
testcase/discussion
  directory rename detection: files/directories in the way of some
renames
  directory rename detection: testcases checking which side did the
rename
  directory rename detection: more involved edge/corner testcases
  directory rename detection: testcases exploring possibly suboptimal
merges
  directory rename detection: miscellaneous testcases to complete
coverage
  directory rename detection: tests for handling overwriting untracked
files
  directory rename detection: tests for handling overwriting dirty files
  merge-recursive: move the get_renames() function
  merge-recursive: introduce new functions to handle rename logic
  merge-recursive: fix leaks of allocated renames and diff_filepairs
  merge-recursive: make !o->detect_rename codepath more obvious
  merge-recursive: split out code for determining diff_filepairs
  merge-recursive: make a helper function for cleanup for handle_renames
  merge-recursive: add get_directory_renames()
  merge-recursive: check for directory level conflicts
  merge-recursive: add computation of collisions due to dir rename &
merging
  merge-recursive: check for file level conflicts then get new name
  merge-recursive: when comparing files, don't include trees
  merge-recursive: apply necessary modifications for directory renames
  merge-recursive: avoid clobbering untracked files with directory
renames
  merge-recursive: fix overwriting dirty files involved in renames
  merge-recursive: fix remaining directory rename + dirty overwrite
cases
  directory rename detection: new testcases showcasing a pair of bugs
  merge-recursive: avoid spurious rename/rename conflict from dir
renames
  merge-recursive: ensure we write updates for directory-renamed file

 merge-recursive.c   | 1243 ++-
 merge-recursive.h   |   27 +
 strbuf.c|   16 +
 strbuf.h|   16 +
 t/t3501-revert-cherry-pick.sh   |2 +-
 t/t6043-merge-rename-directories.sh | 3998 +++
 t/t7607-merge-overwrite.sh  |2 +-
 unpack-trees.c  |4 +-
 unpack-trees.h  |4 +
 9 files changed, 5197 insertions(+), 115 deletions(-)
 create mode 100755 t/t6043-merge-rename-directories.sh

Full tbdiff (the biggest code changes come from commit squashing):

 1: 5ba69c9c7b !  1: 9f1d894d89 directory rename detection: basic testcases
@@ -2,6 +2,7 @@
 
 directory rename detection: basic testcases
 
+Reviewed-by: Stefan Beller 
 Signed-off-by: Elijah Newren 
 
 diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
 2: e1d23f7f95 !  2: 36a4b05757 directory rename detection: directory splitting 
testcases
@@ -2,6 +2,7 @@
 
 directory rename detection: directory splitting testcases
 
+Reviewed-by: Stefan Beller 
 Signed-off-by: Elijah Newren 
 
 diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
 3: b10cb49cf9 !  3: 031a835801 directory 

[PATCH v8 02/29] directory rename detection: directory splitting testcases

[Elijah Newren]

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 143 
 1 file changed, 143 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index d045f0e31e..b22a9052b3 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -439,4 +439,147 @@ test_expect_failure '1f-check: Split a directory into two 
other directories' '
 #   in section 2, plus testcases 3a and 4a.
 ###
 
+
+###
+# SECTION 2: Split into multiple directories, with equal number of paths
+#
+# Explore the splitting-a-directory rules a bit; what happens in the
+# edge cases?
+#
+# Note that there is a closely related case of a directory not being
+# split on either side of history, but being renamed differently on
+# each side.  See testcase 8e for that.
+###
+
+# Testcase 2a, Directory split into two on one side, with equal numbers of 
paths
+#   Commit O: z/{b,c}
+#   Commit A: y/b, w/c
+#   Commit B: z/{b,c,d}
+#   Expected: y/b, w/c, z/d, with warning about z/ -> (y/ vs. w/) conflict
+test_expect_success '2a-setup: Directory split into two on one side, with 
equal numbers of paths' '
+   test_create_repo 2a &&
+   (
+   cd 2a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   mkdir y &&
+   mkdir w &&
+   git mv z/b y/ &&
+   git mv z/c w/ &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   echo d >z/d &&
+   git add z/d &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '2a-check: Directory split into two on one side, with 
equal numbers of paths' '
+   (
+   cd 2a &&
+
+   git checkout A^0 &&
+
+   test_must_fail git merge -s recursive B^0 >out &&
+   test_i18ngrep "CONFLICT.*directory rename split" out &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   :0:y/b :0:w/c :0:z/d &&
+   git rev-parse >expect \
+O:z/b  O:z/c  B:z/d &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 2b, Directory split into two on one side, with equal numbers of 
paths
+#   Commit O: z/{b,c}
+#   Commit A: y/b, w/c
+#   Commit B: z/{b,c}, x/d
+#   Expected: y/b, w/c, x/d; No warning about z/ -> (y/ vs. w/) conflict
+test_expect_success '2b-setup: Directory split into two on one side, with 
equal numbers of paths' '
+   test_create_repo 2b &&
+   (
+   cd 2b &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   mkdir y &&
+   mkdir w &&
+   git mv z/b y/ &&
+   git mv z/c w/ &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   mkdir x &&
+   echo d >x/d &&
+   git add x/d &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '2b-check: Directory split into two on one side, with 
equal numbers of paths' '
+   (
+   cd 2b &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 >out &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   :0:y/b :0:w/c :0:x/d &&
+   git rev-parse >expect \
+O:z/b  O:z/c  B:x/d &&
+   test_cmp expect actual &&
+   test_i18ngrep ! "CONFLICT.*directory rename split" out
+   )
+'
+

[PATCH v8 17/29] merge-recursive: make a helper function for cleanup for handle_renames

[Elijah Newren]

In anticipation of more involved cleanup to come, make a helper function
for doing the cleanup at the end of handle_renames.  Rename the already
existing cleanup_rename[s]() to final_cleanup_rename[s](), name the new
helper initial_cleanup_rename(), and leave the big comment in the code
about why we can't do all the cleanup at once.

Reviewed-by: Stefan Beller 
Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index db8833675e..be20660527 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1667,6 +1667,12 @@ struct rename_info {
struct string_list *merge_renames;
 };
 
+static void initial_cleanup_rename(struct diff_queue_struct *pairs)
+{
+   free(pairs->queue);
+   free(pairs);
+}
+
 static int handle_renames(struct merge_options *o,
  struct tree *common,
  struct tree *head,
@@ -1697,16 +1703,13 @@ static int handle_renames(struct merge_options *o,
 * data structures are still needed and referenced in
 * process_entry().  But there are a few things we can free now.
 */
-
-   free(head_pairs->queue);
-   free(head_pairs);
-   free(merge_pairs->queue);
-   free(merge_pairs);
+   initial_cleanup_rename(head_pairs);
+   initial_cleanup_rename(merge_pairs);
 
return clean;
 }
 
-static void cleanup_rename(struct string_list *rename)
+static void final_cleanup_rename(struct string_list *rename)
 {
const struct rename *re;
int i;
@@ -1722,10 +1725,10 @@ static void cleanup_rename(struct string_list *rename)
free(rename);
 }
 
-static void cleanup_renames(struct rename_info *re_info)
+static void final_cleanup_renames(struct rename_info *re_info)
 {
-   cleanup_rename(re_info->head_renames);
-   cleanup_rename(re_info->merge_renames);
+   final_cleanup_rename(re_info->head_renames);
+   final_cleanup_rename(re_info->merge_renames);
 }
 
 static struct object_id *stage_oid(const struct object_id *oid, unsigned mode)
@@ -2128,7 +2131,7 @@ int merge_trees(struct merge_options *o,
}
 
 cleanup:
-   cleanup_renames(_info);
+   final_cleanup_renames(_info);
 
string_list_clear(entries, 1);
free(entries);
-- 
2.16.1.232.g28d5be9217

Re: [PATCH 0/6] minor test-hashmap fixes

[Jeff King]

On Wed, Feb 14, 2018 at 10:41:35AM -0800, Stefan Beller wrote:

> I have lost track of the hashmap improvements lately, but with
> such a good test helper, we could reduce the amount of
> commented code in hashmap.h and just link to the test helper?
> (as an extra step after this series, I thought we already had that)

Yeah, the example code in hashmap.h looks to be more or less the same
thing as what's here (it maps a long to a value, which simplifies the
FLEX_ARRAY bits, but I think it serves equally well as a reference).

I'd also be fine with drastically simplifying the example in hashmap.h,
and letting test-hashmap serve as a more fleshed-out example (we don't
need to see the main and scanf bits there).

-Peff

Re: [PATCH 1/6] test-hashmap: use ALLOC_ARRAY rather than bare malloc

[Code AI]

Jeff,

Thanks for the feedback!  Most of our users find CodeAI's fixes
useful, even if they are not exactly correct.  However, we are
constantly working on improving the quality of CodeAI's fixes.  The
more people who use it, the more we can improve it.  We would love for
you to try the system directly by visiting mycode.ai.  Its free for
open source users, and you can easily login using your GitHub account.
Also, you may attribute the allocator sizeof() issue to C0deAi.
That's the Github account used by the tool to submit pull requests.

-Ben

On Wed, Feb 14, 2018 at 1:05 PM, Jeff King  wrote:
> These two array allocations have several minor flaws:
>
>   - they use bare malloc, rather than our error-checking
> xmalloc
>
>   - they do a bare multiplication to determine the total
> size (which in theory can overflow, though in this case
> the sizes are all constants)
>
>   - they use sizeof(type), but the type in the second one
> doesn't match the actual array (though it's "int" versus
> "unsigned int", which are guaranteed by C99 to have the
> same size)
>
> None of these are likely to be problems in practice, and
> this is just a test helper. But since people often look at
> test helpers as reference code, we should do our best to
> model the recommended techniques.
>
> Switching to ALLOC_ARRAY fixes all three.
>
> Signed-off-by: Jeff King 
> ---
> The sizeof() thing came from Code AI's original email. I'm happy to
> include a Reported-by there, but I wasn't sure of the correct entity to
> credit. :)
>
>  t/helper/test-hashmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
> index 1145d51671..b36886bf35 100644
> --- a/t/helper/test-hashmap.c
> +++ b/t/helper/test-hashmap.c
> @@ -85,8 +85,8 @@ static void perf_hashmap(unsigned int method, unsigned int 
> rounds)
> unsigned int *hashes;
> unsigned int i, j;
>
> -   entries = malloc(TEST_SIZE * sizeof(struct test_entry *));
> -   hashes = malloc(TEST_SIZE * sizeof(int));
> +   ALLOC_ARRAY(entries, TEST_SIZE);
> +   ALLOC_ARRAY(hashes, TEST_SIZE);
> for (i = 0; i < TEST_SIZE; i++) {
> snprintf(buf, sizeof(buf), "%i", i);
> entries[i] = alloc_test_entry(0, buf, strlen(buf), "", 0);
> --
> 2.16.1.464.gc4bae515b7
>



-- 
Sincerely,

CodeAI Tech Support Team

Re: [PATCH 0/6] minor test-hashmap fixes

[Stefan Beller]

On Wed, Feb 14, 2018 at 10:03 AM, Jeff King  wrote:
> This series started with me fixing the sizeof() mismatch discussed in
>
>   https://public-inbox.org/git/20180214164628.ga...@sigill.intra.peff.net/
>
> but I found a number of minor irritations. Most of them are cosmetic in
> practice, but I think it's important for test-helper code like this to
> model best practices, since people are likely to use it as a reference.
>
>   [1/6]: test-hashmap: use ALLOC_ARRAY rather than bare malloc
>   [2/6]: test-hashmap: check allocation computation for overflow
>   [3/6]: test-hashmap: use xsnprintf rather than snprintf
>   [4/6]: test-hashmap: use strbuf_getline rather than fgets
>   [5/6]: test-hashmap: simplify alloc_test_entry
>   [6/6]: test-hashmap: use "unsigned int" for hash storage
>

The whole series is
Reviewed-by: Stefan Beller 

Thanks for improving the example code.
I have lost track of the hashmap improvements lately, but with
such a good test helper, we could reduce the amount of
commented code in hashmap.h and just link to the test helper?
(as an extra step after this series, I thought we already had that)

Thanks,
Stefan

"git add" with several pathspecS and one doesn't match

[Goffredo Baroncelli]

Hi All,

I am facing this issue: I am ADDing some file with several pathspec, and one of 
these fails. The results is that no file is added at all.

Simple test case:

$ git init .
$ touch 123.txt
$ git add "*.txt" "*.doc"
fatal: pathspec '*.doc' did not match any files
$ git status
[...]
Untracked files:
  (use "git add ..." to include in what will be committed)

123.txt
[...]


Results: no file is added
Expected results: the files which match any pathspec should be added

Looking at the code, git works properly:

from builtins/add.c, near line 500
[...]
for (i = 0; i < pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
if (pathspec.items[i].magic & PATHSPEC_EXCLUDE)
continue;
if (!seen[i] && path[0] &&
((pathspec.items[i].magic &
  (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
 !file_exists(path))) {
if (ignore_missing) {
int dtype = DT_UNKNOWN;
if (is_excluded(, _index, path, 
))
dir_add_ignored(, 
_index,
path, 
pathspec.items[i].len);
} else
die(_("pathspec '%s' did not match any 
files"),
pathspec.items[i].original);
}
}


It seems that if any pathspec doesn't match, all add action fails. Which is the 
rationale of this choice ? I would expect that an error message would be 
printed, but the matched files would be added.

My use case is the following: I use "git" as backup system, and I do something 
like:
$ git add paths/*.doc 
$ git add paths/*.pdf
$ git commit -m "bla bla"

I know that git is not the best method for that, however we have a lot of files 
which are moved between different directories, and git seems to handle this job 
quite nicely. 
Unfortunately the filesystem is quite slow and quite huge, so I would prefer to 
do a single "git add", in order to avoid to traverse all the filesystem more 
times. But this would not work because if one pathspce fails, it prevents all 
other pathspecs to success.


Please put me in CC because I am not subscribed.


BR
G.Baroncelli


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli 
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH v3 00/14] Serialized Git Commit Graph

[Stefan Beller]

On Wed, Feb 14, 2018 at 10:15 AM, Derrick Stolee  wrote:
> There has been a lot of interesting discussion on this topic. Some of that
> involves some decently significant changes from v3, so I wanted to summarize
> my understanding of the feedback and seek out more feedback from reviewers
> before rolling v4.
>
> If we have consensus on these topics, then I'll re-roll on Friday, Feb 16th.
> Please let me know if you are planning on reviewing v3 and need more time
> than that.
>
>
> * Graph Storage:
>
> - Move the graph files to a different directory than the "pack"
> directory. Currently considering ".git/objects/info"

In my copy of git there is already a file

  $ cat .git/objects/info/packs
  P pack-8fdfd126aa8c2a868baf1f89788b07b79a4d365b.pack

which seems to be in line with the information provided in
'man gitrepository-layout':
objects/info
   Additional information about the object store is
   recorded in this directory.

The commit graph files are not exactly "additional info about the
object store" but rather "about the objects". Close enough IMO.

Stefan

Re: [PATCH v1] fsmonitor: update documentation to remove reference to invalid config settings

[Junio C Hamano]

Ben Peart  writes:

> Remove the reference to setting core.fsmonitor to `true` (or `false`) as those
> are not valid settings.
>
> Signed-off-by: Ben Peart 
> ---

Thanks.  It is a bit embarrassing that nobody caught it for this
long.  Will apply.

>
> Notes:
> Base Ref: master
> Web-Diff: https://github.com/benpeart/git/commit/4b7ec2c11e
> Checkout: git fetch https://github.com/benpeart/git fsmonitor_docs-v1 && 
> git checkout 4b7ec2c11e
>
>  Documentation/git-update-index.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-update-index.txt 
> b/Documentation/git-update-index.txt
> index bdb0342593..ad2383d7ed 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -484,8 +484,8 @@ the `core.fsmonitor` configuration variable (see
>  linkgit:git-config[1]) than using the `--fsmonitor` option to
>  `git update-index` in each repository, especially if you want to do so
>  across all repositories you use, because you can set the configuration
> -variable to `true` (or `false`) in your `$HOME/.gitconfig` just once
> -and have it affect all repositories you touch.
> +variable in your `$HOME/.gitconfig` just once and have it affect all
> +repositories you touch.
>  
>  When the `core.fsmonitor` configuration variable is changed, the
>  file system monitor is added to or removed from the index the next time
>
> base-commit: e7e80778e705ea3f9332c634781d6d0f8c6eab64

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Junio C Hamano]

Jeff King  writes:

>> Support $HOME expansion for all filename options. There are about seven
>> of them.
>
> I think this probably makes sense.
>
>>  parse-options.c | 9 ++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> Should this be mentioned in the comment documenting OPT_FILENAME()?

Perhaps.  I think all mention of "$HOME expansion" should be
replaced with "tilde expansion", though.  I first thought we are
expanding any environment variable and $HOME is merely an example of
it when I read the title and the log message, before seeing that the
patch just adds a call to expand_user_path().

Other than that, looks good.  Thanks for a quick enhancement and a
review.

>> diff --git a/parse-options.c b/parse-options.c
>> index d265a756b5..c33f14c74e 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -38,10 +38,13 @@ static int get_arg(struct parse_opt_ctx_t *p, const 
>> struct option *opt,
>>  
>>  static void fix_filename(const char *prefix, const char **file)
>>  {
>> -if (!file || !*file || !prefix || is_absolute_path(*file)
>> -|| !strcmp("-", *file))
>> +if (!file || !*file || is_absolute_path(*file) ||
>> +!strcmp("-", *file))
>>  return;
>> -*file = prefix_filename(prefix, *file);
>> +if (**file == '~')
>> +*file = expand_user_path(*file, 0);
>> +else if (prefix)
>> +*file = prefix_filename(prefix, *file);
>>  }
>
> I thought at first this needed a final "else" clause, because we don't
> assign to *file if we have neither a prefix nor a user-path. But that's
> what the callers expect (and we are similarly a noop if we hit the first
> conditional). So this looks right.
>
> -Peff

Re: What's cooking in git.git (Feb 2018, #02; Tue, 13)

[Stefan Beller]

On Wed, Feb 14, 2018 at 10:11 AM, Brandon Williams  wrote:
> On 02/13, Junio C Hamano wrote:
>>
>> * bw/c-plus-plus (2018-01-30) 37 commits
>>  - replace: rename 'new' variables
>>  - trailer: rename 'template' variables
>>  - tempfile: rename 'template' variables
>>  - wrapper: rename 'template' variables
>>  - environment: rename 'namespace' variables
>>  - diff: rename 'template' variables
>>  - environment: rename 'template' variables
>>  - init-db: rename 'template' variables
>>  - unpack-trees: rename 'new' variables
>>  - trailer: rename 'new' variables
>>  - submodule: rename 'new' variables
>>  - split-index: rename 'new' variables
>>  - remote: rename 'new' variables
>>  - ref-filter: rename 'new' variables
>>  - read-cache: rename 'new' variables
>>  - line-log: rename 'new' variables
>>  - imap-send: rename 'new' variables
>>  - http: rename 'new' variables
>>  - entry: rename 'new' variables
>>  - diffcore-delta: rename 'new' variables
>>  - diff: rename 'new' variables
>>  - diff-lib: rename 'new' variable
>>  - commit: rename 'new' variables
>>  - combine-diff: rename 'new' variables
>>  - remote: rename 'new' variables
>>  - reflog: rename 'new' variables
>>  - pack-redundant: rename 'new' variables
>>  - help: rename 'new' variables
>>  - checkout: rename 'new' variables
>>  - apply: rename 'new' variables
>>  - apply: rename 'try' variables
>>  - diff: rename 'this' variables
>>  - rev-parse: rename 'this' variable
>>  - pack-objects: rename 'this' variables
>>  - blame: rename 'this' variables
>>  - object: rename function 'typename' to 'type_name'
>>  - object_info: change member name from 'typename' to 'type_name'
>>
>>  I do not mind refraining from using these keywords in a foreign
>>  language in our codebase too much, but at the same time, renaming
>>  must be done a bit more thoughtfully.  When the original uses 'new'
>>  together with and in contrast to 'old', renaming 'new' must be done
>>  while preserving the pairing (which may involve renaming 'old' as
>>  well), for example.
>>
>>  Backburnered, i.e. will drop if other topics start to conflict with
>>  it, but will accept rerolls.
>
> I was under the impression that people didn't care too much about this
> (which is a shame but that's an opinion :).

I care, so you are free to change your opinion. :)

> If people were more
> interested in a change like this then I'd be happy to go back through
> and rename the 'old' variables too.

Quoting Duy from a neighboring refactor thread:

  My stand is a bit more aggressive. We should try to achieve better
  [clean code] if possible. But if it makes [Brandon's] life hell, it's not
  worth doing. Converting to ['C++' compatible] is already a step
  forward. Actually if it discourages him from finishing this work, it's
  already not worth doing.

:-)

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

So if you can pick up the work to even make it consistent with old/new
variable names, this would be huge!

Thanks,
Stefan

Re: [PATCH v3 00/14] Serialized Git Commit Graph

[Derrick Stolee]

There has been a lot of interesting discussion on this topic. Some of 
that involves some decently significant changes from v3, so I wanted to 
summarize my understanding of the feedback and seek out more feedback 
from reviewers before rolling v4.


If we have consensus on these topics, then I'll re-roll on Friday, Feb 
16th. Please let me know if you are planning on reviewing v3 and need 
more time than that.



* Graph Storage:

    - Move the graph files to a different directory than the "pack" 
directory. Currently considering ".git/objects/info"


    - Change the "--pack-dir" command-line arguments to "--object-dir" 
arguments.


    - Keep a "graph_head" file, but expand on the reasons (as discussed 
[1]) in the commit message.


    - Adjust "graph_head" and the "--graph-id" argument to use a full 
filename (assuming based in {object-dir}/info/).


    - Remove "pack_dir" from struct commit_graph and 
load_commit_graph_one().


    - Drop "git commit-graph clear" subcommand.


* Graph format:

    - remove redundant hash type & length bytes in favor of a combined 
type/length enum byte.


    - emphasize the fact that the file can contain chunk ids unknown to 
Git and will be ignored on read. Also fix the read code to not die() on 
unknown chunk ids.


    - Don't write the large-edge chunk if it is going to be empty. 
Modify tests to verify this.



* Tests:

    - Format (last apostrophe on new line)

    - Bug check (--stdin-commits should limit by reachability)


* Other style fixes.


[1] 
https://public-inbox.org/git/99543db0-26e4-8daa-a580-b618497e4...@gmail.com/

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds  writes:

> On Tue, Feb 13, 2018 at 9:18 AM, Junio C Hamano  wrote:
>>
>> That makes me wonder if another heuristic I floated earlier is more
>> appropriate.  When merging a tag object T, if refs/tags/T exists and
>> it is that tag object, then an updated "merge" would default to "--ff";
>> otherwise, it would keep the current default of creating a merge even
>> when we could fast-forward, in order to record that tag T in the
>> resulting history.
>
> Oooh. Yes, that sounds like the right thing to do.
>
> So the "no fast-forward" logic would trigger only if the name we used
> for merging is one of the temporary ones (ie .git/{FETCH,MERGE}_HEAD),
> not if the mentioned tag is already a normal tag reference.
>
> Then it's very explicitly about "don't lose the signing information".
>
> I'd still have to teach people to use "--only-ff" if they don't do the
> "fetch and merge" model but literally just do  "git pull upstream
> vX.Y", but at least the case Mauro describes would automatically just
> DTRT.
>
> Me likey.

The implementation cannot exactly be "did the user give FETCH_HEAD
or v4.16-rc1 from the command line?", because we'd want to catch it
when Mauro says "git fetch linus && git merge v4.16-rc1" and behave
identically as "git pull linus v4.16-rc1" (and the latter internally
gets turned into "git merge FETCH_HEAD").

So, instead, we read the "tag" line from the tag object to learn the
tagname T, see if refs/tags/T exists and points at that object, to
see if we are Mauro who follows your tags, or if we are you who
fetch and merge contributors' "for-linus" signed tag (which I am
assuming you won't contaminate your refs/tags/ hierarchy with).

There are a few fallouts in the testsuite if we go this route.  I am
not quite decided if I like the approach.

 builtin/merge.c  | 42 ++
 t/t6200-fmt-merge-msg.sh |  2 +-
 t/t7600-merge.sh |  2 +-
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..45c7916505 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -33,6 +33,7 @@
 #include "sequencer.h"
 #include "string-list.h"
 #include "packfile.h"
+#include "tag.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -1125,6 +1126,42 @@ static struct commit_list *collect_parents(struct commit 
*head_commit,
return remoteheads;
 }
 
+static int merging_a_throwaway_tag(struct commit *commit)
+{
+   const char *tag_ref;
+   struct object_id oid;
+
+   /* Are we merging a tag? */
+   if (!merge_remote_util(commit) ||
+   !merge_remote_util(commit)->obj ||
+   merge_remote_util(commit)->obj->type != OBJ_TAG)
+   return 0;
+
+   /*
+* Now we know we are merging a tag object.  Are we downstream
+* and following the tags from upstream?  If so, we must have
+* the tag object pointed at by "refs/tags/$T" where $T is the
+* tagname recorded in the tag object.  We want to allow such
+* a "just to catch up" merge to fast-forward.
+*/
+   tag_ref = xstrfmt("refs/tags/%s",
+ ((struct tag *)merge_remote_util(commit)->obj)->tag);
+
+   if (!read_ref(tag_ref, ) &&
+   !oidcmp(, _remote_util(commit)->obj->oid))
+   return 0;
+
+   /*
+* Otherwise, we are playing an integrator's role, making a
+* merge with a throw-away tag from a contributor with
+* something like "git pull $contributor $signed_tag".
+* We want to forbid such a merge from fast-forwarding
+* by default; otherwise we would not keep the signature
+* anywhere.
+*/
+   return 1;
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
struct object_id result_tree, stash, head_oid;
@@ -1322,10 +1359,7 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
oid_to_hex(>object.oid));
setenv(buf.buf, merge_remote_util(commit)->name, 1);
strbuf_reset();
-   if (fast_forward != FF_ONLY &&
-   merge_remote_util(commit) &&
-   merge_remote_util(commit)->obj &&
-   merge_remote_util(commit)->obj->type == OBJ_TAG)
+   if (fast_forward != FF_ONLY && merging_a_throwaway_tag(commit))
fast_forward = FF_NO;
}
 
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 2e2fb0e957..a54a52aaa4 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -512,7 +512,7 @@ test_expect_success 'merge-msg with "merging" an annotated 
tag' '
 
test_when_finished "git reset --hard" &&
annote=$(git rev-parse annote) &&
-   git merge --no-commit $annote &&
+   git merge --no-commit --no-ff $annote &&
{
cat <<-EOF
 

Re: What's cooking in git.git (Feb 2018, #02; Tue, 13)

[Brandon Williams]

On 02/13, Junio C Hamano wrote:
> 
> * bw/c-plus-plus (2018-01-30) 37 commits
>  - replace: rename 'new' variables
>  - trailer: rename 'template' variables
>  - tempfile: rename 'template' variables
>  - wrapper: rename 'template' variables
>  - environment: rename 'namespace' variables
>  - diff: rename 'template' variables
>  - environment: rename 'template' variables
>  - init-db: rename 'template' variables
>  - unpack-trees: rename 'new' variables
>  - trailer: rename 'new' variables
>  - submodule: rename 'new' variables
>  - split-index: rename 'new' variables
>  - remote: rename 'new' variables
>  - ref-filter: rename 'new' variables
>  - read-cache: rename 'new' variables
>  - line-log: rename 'new' variables
>  - imap-send: rename 'new' variables
>  - http: rename 'new' variables
>  - entry: rename 'new' variables
>  - diffcore-delta: rename 'new' variables
>  - diff: rename 'new' variables
>  - diff-lib: rename 'new' variable
>  - commit: rename 'new' variables
>  - combine-diff: rename 'new' variables
>  - remote: rename 'new' variables
>  - reflog: rename 'new' variables
>  - pack-redundant: rename 'new' variables
>  - help: rename 'new' variables
>  - checkout: rename 'new' variables
>  - apply: rename 'new' variables
>  - apply: rename 'try' variables
>  - diff: rename 'this' variables
>  - rev-parse: rename 'this' variable
>  - pack-objects: rename 'this' variables
>  - blame: rename 'this' variables
>  - object: rename function 'typename' to 'type_name'
>  - object_info: change member name from 'typename' to 'type_name'
> 
>  I do not mind refraining from using these keywords in a foreign
>  language in our codebase too much, but at the same time, renaming
>  must be done a bit more thoughtfully.  When the original uses 'new'
>  together with and in contrast to 'old', renaming 'new' must be done
>  while preserving the pairing (which may involve renaming 'old' as
>  well), for example.
> 
>  Backburnered, i.e. will drop if other topics start to conflict with
>  it, but will accept rerolls.

I was under the impression that people didn't care too much about this
(which is a shame but that's an opinion :). If people were more
interested in a change like this then I'd be happy to go back through
and rename the 'old' variables too.

-- 
Brandon Williams

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

[Derrick Stolee]

On 2/13/2018 7:12 PM, Jonathan Tan wrote:

On Thu,  8 Feb 2018 15:37:35 -0500
Derrick Stolee  wrote:


| Command  | Before | After  | Rel % |
|--|||---|
| log --oneline --topo-order -1000 |  5.9s  |  0.7s  | -88%  |
| branch -vv   |  0.42s |  0.27s | -35%  |
| rev-list --all   |  6.4s  |  1.0s  | -84%  |
| rev-list --all --objects | 32.6s  | 27.6s  | -15%  |

Could we have a performance test (in t/perf) demonstrating this?


The rev-list perf tests are found in t/perf/p0001-rev-list.sh

The "log --oneline --topo-order -1000" test would be good to add to 
t/perf/p4211-line-log.sh


The "branch -vv" test is pretty uninteresting unless you set up your 
repo to have local branches significantly behind the remote branches. It 
depends a lot more on the data shape than the others which only need a 
large number of reachable objects.


One reason I did not use the builtin perf test scripts is that they seem 
to ignore all local config options, and hence do not inherit the 
core.commitGraph=true setting from the repos pointed at by GIT_PERF_REPO.





+static int check_commit_parents(struct commit *item, struct commit_graph *g,
+   uint32_t pos, const unsigned char *commit_data)

Document what this function does? Also, this function probably needs a
better name.


+/*
+ * Given a commit struct, try to fill the commit struct info, including:
+ *  1. tree object
+ *  2. date
+ *  3. parents.
+ *
+ * Returns 1 if and only if the commit was found in the commit graph.
+ *
+ * See parse_commit_buffer() for the fallback after this call.
+ */
+int parse_commit_in_graph(struct commit *item)
+{

The documentation above duplicates what's in the header file, so we can
probably omit it.


+extern struct object_id *get_nth_commit_oid(struct commit_graph *g,
+   uint32_t n,
+   struct object_id *oid);

This doesn't seem to be used elsewhere - do you plan for a future patch
to use it?

[PATCH 6/6] test-hashmap: use "unsigned int" for hash storage

[Jeff King]

The hashmap API always use an unsigned value for storing
and comparing hashes. Whereas this test code uses "int".
This works out in practice since one can typically
round-trip between "int" and "unsigned int". But since this
is essentially reference code for the hashmap API, we should
model using the correct types.

Signed-off-by: Jeff King 
---
 t/helper/test-hashmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 56efff36e8..9ae9281c07 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -30,7 +30,8 @@ static int test_entry_cmp(const void *cmp_data,
return strcmp(e1->key, key ? key : e2->key);
 }
 
-static struct test_entry *alloc_test_entry(int hash, char *key, char *value)
+static struct test_entry *alloc_test_entry(unsigned int hash,
+  char *key, char *value)
 {
size_t klen = strlen(key);
size_t vlen = strlen(value);
@@ -156,7 +157,7 @@ int cmd_main(int argc, const char **argv)
/* process commands from stdin */
while (strbuf_getline(, stdin) != EOF) {
char *cmd, *p1 = NULL, *p2 = NULL;
-   int hash = 0;
+   unsigned int hash = 0;
struct test_entry *entry;
 
/* break line into command and up to two parameters */
-- 
2.16.1.464.gc4bae515b7

[PATCH 5/6] test-hashmap: simplify alloc_test_entry

[Jeff King]

This function takes two ptr/len pairs, which implies that
they can be arbitrary buffers. But internally, it assumes
that each "ptr" is NUL-terminated at "len" (because we
memcpy an extra byte to pick up the NUL terminator).

In practice this works because each caller only ever passes
strlen(ptr) as the length. But let's drop the "len"
parameters to make our expectations clear.

Note that we can get rid of the "l1" and "l2" variables from
cmd_main() as a further cleanup, since they are now mostly
used to check whether the p1 and p2 arguments are present
(technically the length parameters conflated NULL with the
empty string, which we no longer do, but I think that is
actually an improvement).

Signed-off-by: Jeff King 
---
 t/helper/test-hashmap.c | 35 +--
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 15fc4e372f..56efff36e8 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -30,9 +30,10 @@ static int test_entry_cmp(const void *cmp_data,
return strcmp(e1->key, key ? key : e2->key);
 }
 
-static struct test_entry *alloc_test_entry(int hash, char *key, int klen,
-   char *value, int vlen)
+static struct test_entry *alloc_test_entry(int hash, char *key, char *value)
 {
+   size_t klen = strlen(key);
+   size_t vlen = strlen(value);
struct test_entry *entry = xmalloc(st_add4(sizeof(*entry), klen, vlen, 
2));
hashmap_entry_init(entry, hash);
memcpy(entry->key, key, klen + 1);
@@ -89,7 +90,7 @@ static void perf_hashmap(unsigned int method, unsigned int 
rounds)
ALLOC_ARRAY(hashes, TEST_SIZE);
for (i = 0; i < TEST_SIZE; i++) {
xsnprintf(buf, sizeof(buf), "%i", i);
-   entries[i] = alloc_test_entry(0, buf, strlen(buf), "", 0);
+   entries[i] = alloc_test_entry(0, buf, "");
hashes[i] = hash(method, i, entries[i]->key);
}
 
@@ -155,7 +156,7 @@ int cmd_main(int argc, const char **argv)
/* process commands from stdin */
while (strbuf_getline(, stdin) != EOF) {
char *cmd, *p1 = NULL, *p2 = NULL;
-   int l1 = 0, l2 = 0, hash = 0;
+   int hash = 0;
struct test_entry *entry;
 
/* break line into command and up to two parameters */
@@ -166,31 +167,29 @@ int cmd_main(int argc, const char **argv)
 
p1 = strtok(NULL, DELIM);
if (p1) {
-   l1 = strlen(p1);
hash = icase ? strihash(p1) : strhash(p1);
p2 = strtok(NULL, DELIM);
-   if (p2)
-   l2 = strlen(p2);
}
 
-   if (!strcmp("hash", cmd) && l1) {
+   if (!strcmp("hash", cmd) && p1) {
 
/* print results of different hash functions */
-   printf("%u %u %u %u\n", strhash(p1), memhash(p1, l1),
-   strihash(p1), memihash(p1, l1));
+   printf("%u %u %u %u\n",
+  strhash(p1), memhash(p1, strlen(p1)),
+  strihash(p1), memihash(p1, strlen(p1)));
 
-   } else if (!strcmp("add", cmd) && l1 && l2) {
+   } else if (!strcmp("add", cmd) && p1 && p2) {
 
/* create entry with key = p1, value = p2 */
-   entry = alloc_test_entry(hash, p1, l1, p2, l2);
+   entry = alloc_test_entry(hash, p1, p2);
 
/* add to hashmap */
hashmap_add(, entry);
 
-   } else if (!strcmp("put", cmd) && l1 && l2) {
+   } else if (!strcmp("put", cmd) && p1 && p2) {
 
/* create entry with key = p1, value = p2 */
-   entry = alloc_test_entry(hash, p1, l1, p2, l2);
+   entry = alloc_test_entry(hash, p1, p2);
 
/* add / replace entry */
entry = hashmap_put(, entry);
@@ -199,7 +198,7 @@ int cmd_main(int argc, const char **argv)
puts(entry ? get_value(entry) : "NULL");
free(entry);
 
-   } else if (!strcmp("get", cmd) && l1) {
+   } else if (!strcmp("get", cmd) && p1) {
 
/* lookup entry in hashmap */
entry = hashmap_get_from_hash(, hash, p1);
@@ -212,7 +211,7 @@ int cmd_main(int argc, const char **argv)
entry = hashmap_get_next(, entry);
}
 
-   } else if (!strcmp("remove", cmd) && l1) {
+   } else if (!strcmp("remove", cmd) && p1) {
 
/* setup static key */
struct hashmap_entry key;
@@ -238,7 +237,7 @@ int 

Re: [PATCH 14/26] sha1_file: allow prepare_alt_odb to handle arbitrary repositories

[Brandon Williams]

On 02/14, Duy Nguyen wrote:
> On Tue, Feb 13, 2018 at 8:22 AM, Stefan Beller  wrote:
> > Signed-off-by: Stefan Beller 
> > ---
> >  object-store.h |  3 +--
> >  sha1_file.c| 21 +++--
> >  2 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/object-store.h b/object-store.h
> > index d96a16edd1..add1d4e27c 100644
> > --- a/object-store.h
> > +++ b/object-store.h
> > @@ -61,7 +61,6 @@ struct packed_git {
> > char pack_name[FLEX_ARRAY]; /* more */
> >  };
> >
> > -#define prepare_alt_odb(r) prepare_alt_odb_##r()
> > -extern void prepare_alt_odb_the_repository(void);
> > +void prepare_alt_odb(struct repository *r);
> >
> >  #endif /* OBJECT_STORE_H */
> > diff --git a/sha1_file.c b/sha1_file.c
> > index d18ce3aeba..f046d560f8 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -677,21 +677,22 @@ int foreach_alt_odb(alt_odb_fn fn, void *cb)
> > return r;
> >  }
> >
> > -void prepare_alt_odb_the_repository(void)
> > +void prepare_alt_odb(struct repository *r)
> >  {
> > -   const char *alt;
> > -
> > -   if (the_repository->objects.alt_odb_tail)
> > +   if (r->objects.alt_odb_tail)
> > return;
> >
> > -   alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> > +   r->objects.alt_odb_tail = >objects.alt_odb_list;
> > +
> > +   if (!r->ignore_env) {
> > +   const char *alt = getenv(ALTERNATE_DB_ENVIRONMENT);
> 
> If one day the majority of git moves to use 'struct repository', then
> ALTERNATE_DB_ENVIRONMENT is always ignored because ignore_env is
> always true. I think if you ignore_env, then you still need to get
> this "alt" from  'struct raw_object_store' (or 'struct repository').
> 
> Since you get lots of getenv() in repo_setup_env(), I think this
> getenv(ALTERNATE_DB_ENVIRONMENT) belongs there too. Then here, if
> ignore_env is true, you read r->objects.alt or something instead of
> doing getenv().
> 
> I really want to kill this getenv() in this code, which is basically
> delayed initialization because we haven't done proper init on
> the_repo. I realize that it cannot be done earlier, when
> prepare_alt_odb() does not even have a  'struct repository *' to work
> with. Would it be ok if I contributed a patch on top of your series to
> basically do repo_init(_repo) for all builtin/external commands
> (and fix all the bugs that come with it)? Then we would not need
> ignore_env here anymore.

At some point yes we would definitely want the setup code to fully
initialize a repository object (in this case the_repository).  And I
would even like to change the function signatures of all the builtin
commands to take a repository object so that they don't implicitly rely
on the_repository at all.

When I introduced struct repository I seem to remember there being a
couple things which were different about setup that made it difficult to
simply call repo_init() on the_repository.  If you can fix whatever
those issues with setup were (I can't remember all of them) then that
would be great :)

> 
> > +   if (!alt)
> > +   alt = "";
> >
> > -   the_repository->objects.alt_odb_tail =
> > -   _repository->objects.alt_odb_list;
> > -   link_alt_odb_entries(the_repository, alt,
> > -PATH_SEP, NULL, 0);
> > +   link_alt_odb_entries(r, alt, PATH_SEP, NULL, 0);
> > +   }
> >
> > -   read_info_alternates(the_repository, get_object_directory(), 0);
> > +   read_info_alternates(r, r->objects.objectdir, 0);
> >  }
> >
> >  /* Returns 1 if we have successfully freshened the file, 0 otherwise. */
> > --
> > 2.16.1.73.ga2c3e9663f.dirty
> >
> 
> 
> 
> -- 
> Duy

-- 
Brandon Williams

[PATCH 3/6] test-hashmap: use xsnprintf rather than snprintf

[Jeff King]

In general, using a bare snprintf can truncate the resulting
buffer, leading to confusing results. In this case we know
that our buffer is sized large enough to accommodate our
loop, so there's no bug. However, we should use xsnprintf()
to document (and check) that assumption, and to model good
practice to people reading the code.

Signed-off-by: Jeff King 
---
 t/helper/test-hashmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 2100877c2b..28b913fbd6 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -87,7 +87,7 @@ static void perf_hashmap(unsigned int method, unsigned int 
rounds)
ALLOC_ARRAY(entries, TEST_SIZE);
ALLOC_ARRAY(hashes, TEST_SIZE);
for (i = 0; i < TEST_SIZE; i++) {
-   snprintf(buf, sizeof(buf), "%i", i);
+   xsnprintf(buf, sizeof(buf), "%i", i);
entries[i] = alloc_test_entry(0, buf, strlen(buf), "", 0);
hashes[i] = hash(method, i, entries[i]->key);
}
-- 
2.16.1.464.gc4bae515b7

[PATCH 4/6] test-hashmap: use strbuf_getline rather than fgets

[Jeff King]

Using fgets() with a fixed-size buffer can lead to lines
being accidentally split across two calls if they are larger
than the buffer size.

As this is just a test helper, this is unlikely to be a
problem in practice. But since people may look at test
helpers as reference code, it's a good idea for them to
model the preferred behavior.

Signed-off-by: Jeff King 
---
 t/helper/test-hashmap.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 28b913fbd6..15fc4e372f 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "hashmap.h"
+#include "strbuf.h"
 
 struct test_entry
 {
@@ -143,7 +144,7 @@ static void perf_hashmap(unsigned int method, unsigned int 
rounds)
  */
 int cmd_main(int argc, const char **argv)
 {
-   char line[1024];
+   struct strbuf line = STRBUF_INIT;
struct hashmap map;
int icase;
 
@@ -152,13 +153,13 @@ int cmd_main(int argc, const char **argv)
hashmap_init(, test_entry_cmp, , 0);
 
/* process commands from stdin */
-   while (fgets(line, sizeof(line), stdin)) {
+   while (strbuf_getline(, stdin) != EOF) {
char *cmd, *p1 = NULL, *p2 = NULL;
int l1 = 0, l2 = 0, hash = 0;
struct test_entry *entry;
 
/* break line into command and up to two parameters */
-   cmd = strtok(line, DELIM);
+   cmd = strtok(line.buf, DELIM);
/* ignore empty lines */
if (!cmd || *cmd == '#')
continue;
@@ -262,6 +263,7 @@ int cmd_main(int argc, const char **argv)
}
}
 
+   strbuf_release();
hashmap_free(, 1);
return 0;
 }
-- 
2.16.1.464.gc4bae515b7

[PATCH 2/6] test-hashmap: check allocation computation for overflow

[Jeff King]

When we allocate the test_entry flex-struct, we have to add
up all of the elements that go into the flex array. If these
were to overflow a size_t, this would allocate a too-small
buffer, which we would then overflow in our memcpy steps.

Since this is just a test-helper, it probably doesn't matter
in practice, but we should model the correct technique by
using the st_add() macros.

Unfortunately, we cannot use the FLEX_ALLOC() macros here,
because we are stuffing two different buffers into a single
flex array.

While we're here, let's also swap out "malloc" for our
error-checking "xmalloc", and use the preferred
"sizeof(*var)" instead of "sizeof(type)".

Signed-off-by: Jeff King 
---
 t/helper/test-hashmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index b36886bf35..2100877c2b 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -32,8 +32,7 @@ static int test_entry_cmp(const void *cmp_data,
 static struct test_entry *alloc_test_entry(int hash, char *key, int klen,
char *value, int vlen)
 {
-   struct test_entry *entry = malloc(sizeof(struct test_entry) + klen
-   + vlen + 2);
+   struct test_entry *entry = xmalloc(st_add4(sizeof(*entry), klen, vlen, 
2));
hashmap_entry_init(entry, hash);
memcpy(entry->key, key, klen + 1);
memcpy(entry->key + klen + 1, value, vlen + 1);
-- 
2.16.1.464.gc4bae515b7

[PATCH 1/6] test-hashmap: use ALLOC_ARRAY rather than bare malloc

[Jeff King]

These two array allocations have several minor flaws:

  - they use bare malloc, rather than our error-checking
xmalloc

  - they do a bare multiplication to determine the total
size (which in theory can overflow, though in this case
the sizes are all constants)

  - they use sizeof(type), but the type in the second one
doesn't match the actual array (though it's "int" versus
"unsigned int", which are guaranteed by C99 to have the
same size)

None of these are likely to be problems in practice, and
this is just a test helper. But since people often look at
test helpers as reference code, we should do our best to
model the recommended techniques.

Switching to ALLOC_ARRAY fixes all three.

Signed-off-by: Jeff King 
---
The sizeof() thing came from Code AI's original email. I'm happy to
include a Reported-by there, but I wasn't sure of the correct entity to
credit. :)

 t/helper/test-hashmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 1145d51671..b36886bf35 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -85,8 +85,8 @@ static void perf_hashmap(unsigned int method, unsigned int 
rounds)
unsigned int *hashes;
unsigned int i, j;
 
-   entries = malloc(TEST_SIZE * sizeof(struct test_entry *));
-   hashes = malloc(TEST_SIZE * sizeof(int));
+   ALLOC_ARRAY(entries, TEST_SIZE);
+   ALLOC_ARRAY(hashes, TEST_SIZE);
for (i = 0; i < TEST_SIZE; i++) {
snprintf(buf, sizeof(buf), "%i", i);
entries[i] = alloc_test_entry(0, buf, strlen(buf), "", 0);
-- 
2.16.1.464.gc4bae515b7

[PATCH 0/6] minor test-hashmap fixes

[Jeff King]

This series started with me fixing the sizeof() mismatch discussed in

  https://public-inbox.org/git/20180214164628.ga...@sigill.intra.peff.net/

but I found a number of minor irritations. Most of them are cosmetic in
practice, but I think it's important for test-helper code like this to
model best practices, since people are likely to use it as a reference.

  [1/6]: test-hashmap: use ALLOC_ARRAY rather than bare malloc
  [2/6]: test-hashmap: check allocation computation for overflow
  [3/6]: test-hashmap: use xsnprintf rather than snprintf
  [4/6]: test-hashmap: use strbuf_getline rather than fgets
  [5/6]: test-hashmap: simplify alloc_test_entry
  [6/6]: test-hashmap: use "unsigned int" for hash storage

 t/helper/test-hashmap.c | 53 +
 1 file changed, 27 insertions(+), 26 deletions(-)

-Peff

Re: [PATCH] color.h: document and modernize header

[Stefan Beller]

On Tue, Feb 13, 2018 at 11:23 PM, Eric Sunshine  wrote:
> On Mon, Feb 12, 2018 at 10:55 PM, Eric Sunshine  
> wrote:
>> On Mon, Feb 12, 2018 at 8:41 PM, Stefan Beller  wrote:
>>> + * Output the formatted string in the specified color (and then reset to 
>>> normal
>>> + * color so subsequent output is uncolored). Omits the color encapsulation 
>>> if
>>> + * `color` is NULL. The `color_fprintf_ln` prints a new line after 
>>> resetting
>>> + * the color.  BUG: The `color_print_strbuf` prints the given pre-formatted
>>> + * strbuf instead, up to its first NUL character.
>>
>> "`color_print_strbuf` prints the given pre-formatted strbuf (BUG: but
>> only up to the first NUL character)."
>>
>> Probably not worth a re-roll is Junio can amend it locally.
>
> By the way, thanks for the patience in the face of the nit-picking
> this patch has undergone.

In retrospect it is clear why we have so much nitpicking here
as it adds documentation to a part of git that is rather non-essential. ;-)

Thanks for bearing with my inability to write perfect code on the first try.

Re: What's cooking in git.git (Feb 2018, #02; Tue, 13)

[Derrick Stolee]

On 2/14/2018 12:23 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


There have been a few "What's cooking" emails since I submitted v1 of
"Serialized Git Commit Graph" and it has not appeared with a tracking
branch. Is this a mistake, or is it something about the state of the
review?

The latter.

Once I pick up a topic and have it in 'pu', I'd be committing to
carrying it and keeping it up-to-date, while dealing with possible
conflicts with other topics.  As I do not have infinite bandwidth, I
try not to chase targets that are still moving too rapidly, which in
turn means that a hot topic everybody is excited by its goal will
take more rerolls than other topics before hitting 'pu', because it
gets more good suggestions and it takes time for its patches to stop
morphing a lot.


Thanks for clarifying. That makes sense.


The discussion in the last and current rounds gave me an impression
that some stuff (e.g. "graph-head") are still likely to change quite
a lot during the review-response cycle.  Is everybody happy with the
latest set of patches or are there issues raised already in the
review that are better addressed before we start making it interact
with other topics in flight?


To avoid causing a tangent in this thread, I'll send a message on the v3 
thread summarizing what I plan to do for v4 and ask for consensus on 
that approach before I do.


Thanks,
-Stolee

Re: [PATCH 3/7] worktree move: new command

[Junio C Hamano]

Duy Nguyen  writes:

> On Wed, Feb 14, 2018 at 10:16 AM, Jeff King  wrote:
>> Hmm. That is not too bad, but somehow it feels funny to me to be
>> polluting each test script with these annotations. And to be driving it
>> from inside the test scripts.
>>
>> It seems like:
>>
>>   make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"
>>
>> would be sufficient.
>
> And all new test files are considered leak-free by default? I like that!

Sounds good ;-)

>
>> And updating the list would just be:
>>
>>   # assume we're using prove, which will keep running after failure,
>>   # and will record the results for us to parse (using "--state=").
>>   # Otherwise use "make -k" and grep in t/test-results.
>>   make SANITIZE=leak test
>>   (cd t && prove --dry --state=failed) |
>>   perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
>>   sort >known-leaky
>>
>> That would update both now-passing and now-failing tests. Presumably
>> we'd keep it checked in, so "git diff" would show you the changes.

Sounds good, too.

Re: Bug? Error during commit

[Junio C Hamano]

Jeff King  writes:

> Here's the patch to make "show -B --stat" work. I'll give some more
> thought to whether this is a good idea and prepare a series.
>
> One downside is that in the common case it causes us to look up each
> object twice (once to get its size, and then again to load the content).
> I wonder if we should have a function for "read this object, unless it's
> over N bytes, in which case just tell me the size". That's weirdly
> specific, but I think pretty much every user of core.bigfilethreshold
> would want it.

After reading through "git grep" hits for the global variable, I
think it makes sense to have such a helper with a good name without
configurable N (just use big_file_threshold that is global).  The
user of the interface either punt on size like this caller, or
would switch to the streaming interface.

>
> ---
> diff --git a/diffcore-break.c b/diffcore-break.c
> index c64359f489..35f5b50bcc 100644
> --- a/diffcore-break.c
> +++ b/diffcore-break.c
> @@ -61,6 +61,13 @@ static int should_break(struct diff_filespec *src,
>   !oidcmp(>oid, >oid))
>   return 0; /* they are the same */
>  
> + if (diff_populate_filespec(src, CHECK_SIZE_ONLY) ||
> + diff_populate_filespec(dst, CHECK_SIZE_ONLY))
> + return 0; /* error but caught downstream */
> +
> + if (src->size > big_file_threshold || dst->size > big_file_threshold)
> + return 0; /* too big to be worth computation */
> +
>   if (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0))
>   return 0; /* error but caught downstream */
>  

Re: What's cooking in git.git (Feb 2018, #02; Tue, 13)

[Junio C Hamano]

Derrick Stolee  writes:

> There have been a few "What's cooking" emails since I submitted v1 of
> "Serialized Git Commit Graph" and it has not appeared with a tracking
> branch. Is this a mistake, or is it something about the state of the
> review?

The latter.

Once I pick up a topic and have it in 'pu', I'd be committing to
carrying it and keeping it up-to-date, while dealing with possible
conflicts with other topics.  As I do not have infinite bandwidth, I
try not to chase targets that are still moving too rapidly, which in
turn means that a hot topic everybody is excited by its goal will
take more rerolls than other topics before hitting 'pu', because it
gets more good suggestions and it takes time for its patches to stop
morphing a lot.

The discussion in the last and current rounds gave me an impression
that some stuff (e.g. "graph-head") are still likely to change quite
a lot during the review-response cycle.  Is everybody happy with the
latest set of patches or are there issues raised already in the
review that are better addressed before we start making it interact
with other topics in flight?

Thanks for pinging.

Re: What's cooking in git.git (Feb 2018, #02; Tue, 13)

[Junio C Hamano]

Duy Nguyen  writes:

>>  Will see another reroll.
>>  cf. 

Re: [PATCH] CodeAI fixes 1 Allocator sizeof() operand mismatch, 2 Null Pointer Dereference, and 2 Dead Code

[Jeff King]

On Wed, Feb 14, 2018 at 10:50:12AM -0500, Code AI wrote:

> Hi my name is Benjamin Bales.
> 
> I am the founder and creator of CodeAI,
> the first non-human contributor to your software project. CodeAI finds
> and fixes security defects for you. It fixed 18. It wants to merge 5
> commits - 1 Allocator sizeof() operand mismatch, 2 Null Pointer
> Dereference issues and 2 Dead Code issues in git. To view all 18 fixed
> issues from the run claim your free open source account at mycode.ai
> and the Dockerfile used to build and run your project in CodeAI, here-
> https://drive.google.com/open?id=12d2poeHabdc0DSShDcekSU5bI0Il6Qv- .
> It is always free for open source projects.
> 
> If you have any questions about these results or have general
> inquiries about CodeAI, please send an email to techsupp...@mycode.ai

Too bad the AI cannot follow SubmittingPatches. :)

We've often seen the results of static analyzers on the list. In general
we welcome fixes from static analyzers, and even fixes to silence false
positives from static analyzers if they're not too onerous (and if they
get the analyzer to a point where it generates only useful results).

But we prefer to see some analysis done on the call-sites to determine
if they are actual problems, and if the fix is appropriate.

Let's look at these ones.

> Allocator sizeof() mismatch:
> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
> index 1145d51..c3ea5c1 100644
> --- a/t/helper/test-hashmap.c
> +++ b/t/helper/test-hashmap.c
> @@ -86,7 +86,7 @@ static void perf_hashmap(unsigned int method,
> unsigned int rounds)
> unsigned int i, j;
> 
> entries = malloc(TEST_SIZE * sizeof(struct test_entry *));
> -   hashes = malloc(TEST_SIZE * sizeof(int));
> +   hashes = malloc(TEST_SIZE * sizeof(unsigned));

I agree this ought to be "unsigned", though I don't know if there is any
platform in practice where the sizes of "int" and "unsigned int" differ.
However, an even better solution is "sizeof(*hashes)", which eliminates
the need to keep it in sync. Or even ALLOC_ARRAY(hashes), which does
this for us.

> Null dereference fixes:
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 4c51aec..f26858a 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1604,7 +1604,7 @@ static void show_pack_info(int stat_only)
>  "non delta: %d objects",
>  baseobjects),
>   baseobjects);
> -   for (i = 0; i < deepest_delta; i++) {
> +   for (i = 0; chain_histogram && (i < deepest_delta); i++) {
> if (!chain_histogram[i])
> continue;
> printf_ln(Q_("chain length = %d: %lu object",

This one looks like a false positive. At the beginning of the function,
we allocate chain_histogram if deepest_delta is non-zero. And if it's
zero, we'll never enter this loop.

Curiously, the tool did not flag the reference to chain_histogram in the
earlier loop. Which is also correct, but in a much less obvious way. It
does:

  if (is_delta_type(obj->type))
  chain_histogram[obj_stat[i].delta_depth - 1]++;

So there the assumption is that if we saw any delta types, we would
previously have incremented deepest_delta to be non-zero. Which I think
holds, but seems way less likely for a static analysis tool to have
realized.

> diff --git a/unpack-trees.c b/unpack-trees.c
> index 96c3327..fcd9332 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1721,7 +1721,7 @@ static int verify_absent(const struct cache_entry *ce,
>  enum unpack_trees_error_types error_type,
>  struct unpack_trees_options *o)
>  {
> -   if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))
> +   if (ce && (!o->skip_sparse_checkout && (ce->ce_flags &
> CE_NEW_SKIP_WORKTREE)))
> return 0;
> return verify_absent_1(ce, error_type, o);
>  }

This one is hard to evaluate. It seems to suggest that somebody could
pass a NULL ce to verify_absent(). But without knowing how the tool came
to that conclusion, it's hard to know if that's true of any callsites
(though just grepping the callers, most seem to otherwise dereference
"ce").

If there is such a callsite, though, this patch isn't sufficient. We'd
pass the NULL down to verify_absent_1(), which may dereference it, too
Though it returns early in some cases, so it's _possible_ that the one
code path that passes a NULL never sets those flags (again, hard to tell
without the tool reporting which execution path it found with the NULL).
I'd argue that it's still the wrong fix, though, as the result would be
very brittle.

> Dead code fixes:
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -235,7 +235,6 @@ static int edit_patch(int argc, const char **argv,
> const char *prefix)
> init_revisions(, prefix);
> rev.diffopt.context = 7;
> 
> -   argc = setup_revisions(argc, 

[PATCH] CodeAI fixes 1 Allocator sizeof() operand mismatch, 2 Null Pointer Dereference, and 2 Dead Code

[Code AI]

Hi my name is Benjamin Bales.

I am the founder and creator of CodeAI,
the first non-human contributor to your software project. CodeAI finds
and fixes security defects for you. It fixed 18. It wants to merge 5
commits - 1 Allocator sizeof() operand mismatch, 2 Null Pointer
Dereference issues and 2 Dead Code issues in git. To view all 18 fixed
issues from the run claim your free open source account at mycode.ai
and the Dockerfile used to build and run your project in CodeAI, here-
https://drive.google.com/open?id=12d2poeHabdc0DSShDcekSU5bI0Il6Qv- .
It is always free for open source projects.

If you have any questions about these results or have general
inquiries about CodeAI, please send an email to techsupp...@mycode.ai

Allocator sizeof() mismatch:
diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c
index 1145d51..c3ea5c1 100644
--- a/t/helper/test-hashmap.c
+++ b/t/helper/test-hashmap.c
@@ -86,7 +86,7 @@ static void perf_hashmap(unsigned int method,
unsigned int rounds)
unsigned int i, j;

entries = malloc(TEST_SIZE * sizeof(struct test_entry *));
-   hashes = malloc(TEST_SIZE * sizeof(int));
+   hashes = malloc(TEST_SIZE * sizeof(unsigned));
for (i = 0; i < TEST_SIZE; i++) {
snprintf(buf, sizeof(buf), "%i", i);
entries[i] = alloc_test_entry(0, buf, strlen(buf), "", 0);


Null dereference fixes:
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 4c51aec..f26858a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1604,7 +1604,7 @@ static void show_pack_info(int stat_only)
 "non delta: %d objects",
 baseobjects),
  baseobjects);
-   for (i = 0; i < deepest_delta; i++) {
+   for (i = 0; chain_histogram && (i < deepest_delta); i++) {
if (!chain_histogram[i])
continue;
printf_ln(Q_("chain length = %d: %lu object",

diff --git a/unpack-trees.c b/unpack-trees.c
index 96c3327..fcd9332 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1721,7 +1721,7 @@ static int verify_absent(const struct cache_entry *ce,
 enum unpack_trees_error_types error_type,
 struct unpack_trees_options *o)
 {
-   if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))
+   if (ce && (!o->skip_sparse_checkout && (ce->ce_flags &
CE_NEW_SKIP_WORKTREE)))
return 0;
return verify_absent_1(ce, error_type, o);
 }


Dead code fixes:
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -235,7 +235,6 @@ static int edit_patch(int argc, const char **argv,
const char *prefix)
init_revisions(, prefix);
rev.diffopt.context = 7;

-   argc = setup_revisions(argc, argv, , NULL);
rev.diffopt.output_format = DIFF_FORMAT_PATCH;
rev.diffopt.use_color = 0;
rev.diffopt.flags.ignore_dirty_submodules = 1;

diff --git a/fsck.c b/fsck.c
index 032699e..78563c3 100644
--- a/fsck.c
+++ b/fsck.c
@@ -704,7 +704,6 @@ static int fsck_ident(const char **ident, struct
object *obj, struct fsck_option
!isdigit(p[4]) ||
(p[5] != '\n'))
return report(options, obj, FSCK_MSG_BAD_TIMEZONE,
"invalid author/committer line - bad time zone");
-   p += 6;
return 0;
 }

-- 
Sincerely,

CodeAI Tech Support Team

[PATCH v1] fsmonitor: update documentation to remove reference to invalid config settings

[Ben Peart]

Remove the reference to setting core.fsmonitor to `true` (or `false`) as those
are not valid settings.

Signed-off-by: Ben Peart 
---

Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/4b7ec2c11e
Checkout: git fetch https://github.com/benpeart/git fsmonitor_docs-v1 && 
git checkout 4b7ec2c11e

 Documentation/git-update-index.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index bdb0342593..ad2383d7ed 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -484,8 +484,8 @@ the `core.fsmonitor` configuration variable (see
 linkgit:git-config[1]) than using the `--fsmonitor` option to
 `git update-index` in each repository, especially if you want to do so
 across all repositories you use, because you can set the configuration
-variable to `true` (or `false`) in your `$HOME/.gitconfig` just once
-and have it affect all repositories you touch.
+variable in your `$HOME/.gitconfig` just once and have it affect all
+repositories you touch.
 
 When the `core.fsmonitor` configuration variable is changed, the
 file system monitor is added to or removed from the index the next time

base-commit: e7e80778e705ea3f9332c634781d6d0f8c6eab64
-- 
2.15.0.windows.1

Re: [PATCH v3 04/42] git-completion.bash: introduce __gitcomp_builtin

[SZEDER Gábor]

Thanks for working on this.  I anticipated that the completion script
lack some options, but wow, I didn't expect that there are so many
missing.

On Fri, Feb 9, 2018 at 12:01 PM, Nguyễn Thái Ngọc Duy  wrote:
> This is a __gitcomp wrapper that will execute
>
> git ... --git-completion-helper
>
> to get the list of completable options. The call will be made only
> once and cached to avoid performance issues, especially on Windows.

Nit: the call will be made every time; 'git ... --git-completion-helper'
will be executed only once.

> __gitcomp_builtin() allows callers to change its output a bit by adding
> some more options, or removing some.
>
> - Current --git-completion-helper for example does not output --no-foo
>   form, this has to be added manually by __gitcomp_builtin() callers
>   when necessary
>
> - Some options from --git-completion-helper should only be available in
>   certain conditions (e.g. --continue and friends). __gitcomp_builtin()
>   callers can remove them if the conditions are not met.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  contrib/completion/git-completion.bash | 33 ++
>  1 file changed, 33 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 3683c772c5..85e7f26328 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -280,6 +280,39 @@ __gitcomp ()
> esac
>  }
>
> +# This function is equivalent to
> +#
> +#__gitcomp "$(git xxx --git-completion-helper) ..."
> +#
> +# except that the output is cached. Accept 1-3 arguments:
> +# 1: the git command to execute, this is also the cache key
> +# 2: extra options to be added on top (e.g. negative forms)
> +# 3: options to be excluded
> +__gitcomp_builtin ()

Please excuse the bikeshed at v3, but I don't like the name of this
function.  It indicates that it completes builtins, but it completes
options of builtins, and even then only the options of those using parse
options.  Furthermore, the '__gitcomp' prefix is usually used for
functions that merely put words into COMPREPLY, but this function does a
whole lot more (getting the options from builtins, include and exclude
options, caching).

Alas I don't have any great name; __git_complete_options is better,
because it uses the right function name prefix, but only slightly
better, because it can't generally be used to complete options, as it
won't work with scripts or with builtins not using parse options (though
with time more scripts will be turned into builtins and more builtins
will use parse options).  I'm not sure it's that match better to make it
worth changing fourty-odd patches.

> +{
> +   # spaces must be replaced with underscore for multi-word
> +   # commands, e.g. "git remote add" becomes remote_add.
> +   local cmd="$1"

The alternative would be 'command subcommand', i.e. keeping that space,
and in that case we could spare the ${cmd/_/ } in this function, and
could even say '__gitcomp "$(git $1 --git-completion-helper)" in the
equivalent example above; OTOH we would need quoting on all callsites
with subcommands.  Again, I'm not sure it's worth it.

> +   local incl="$2"
> +   local excl="$3"
> +
> +   local var=__gitcomp_builtin_"${cmd/-/_}"
> +   local options
> +   eval "options=\$$var"
> +
> +   if [ -z "$options" ]; then
> +   # leading and trailing spaces are significant to make
> +   # option removal work correctly.
> +   options=" $(__git ${cmd/_/ } --git-completion-helper) $incl "
> +   for i in $excl; do
> +   options="${options/ $i / }"
> +   done
> +   eval "$var=\"$options\""
> +   fi
> +
> +   __gitcomp "$options"
> +}
> +
>  # Variation of __gitcomp_nl () that appends to the existing list of
>  # completion candidates, COMPREPLY.
>  __gitcomp_nl_append ()
> --
> 2.16.1.207.gedba492059
>

Re: [PATCH v3 29/42] completion: use __gitcomp_builtin in _git_notes

[SZEDER Gábor]

On Fri, Feb 9, 2018 at 12:02 PM, Nguyễn Thái Ngọc Duy  wrote:

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index c7b8b37f19..60127daebf 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1835,7 +1835,7 @@ _git_notes ()
>
> case "$subcommand,$cur" in
> ,--*)
> -   __gitcomp '--ref'
> +   __gitcomp_builtin notes
> ;;
> ,*)
> case "$prev" in

Hmm, after this patch is applied, this part of _git_notes() looks like
this:

case "$subcommand,$cur" in

,*)
case "$prev" in
--ref)
__git_complete_refs
;;
*)
__gitcomp "$subcommands --ref"
;;
esac
;;

Note that '--ref' option passed to __gitcomp() along with the
subcommands.

It would be great if that option could come from parse options as well,
but we can't rely on $__gitcomp_builtin_notes being already initialized
at this point and the current __gitcomp_builtin function tightly couples
initializing that variable and completing options.

It would be even greater, if all the subcommands could come from parse
options as well, but not sure how that would fit into parse options,
because subcommands are, well, not --options.

There are only a few commands with subcommands whose main command
accepts an --option, too; I could only find 'notes', 'remote', 'stash'
and 'submodule'.  The latter two are scripts, and for 'remote' we don't
offer its '--verbose' option along with its subcommands, but perhaps we
should.  Anyway, considering that there are only two such cases at the
moment, I think we could live with that two hard-coded options.

Re: [PATCH v2 1/3] send-email: add and use a local copy of Mail::Address

[Ævar Arnfjörð Bjarmason]

On Fri, Jan 5, 2018 at 7:36 PM, Matthieu Moy  wrote:

>  create mode 100644 perl/Git/FromCPAN/Mail/Address.pm
>  create mode 100755 perl/Git/Mail/Address.pm

I didn't notice this in my initial review, but just now when it's
landed in master and it's shiny-green in my terminal, this file should
be 644, not 755.

RE: git-bashe.exe fails to launch

[greenwood9]

This appears to be covered by 
https://github.com/git-for-windows/git/issues/1473.  In particular, avih's 
comment about git-bash working without a .minttyrc file applies to me.  
Apologize for the noise; should have fully reading bug reporting instructions.

-Original Message-
From: greenwo...@comcast.net [mailto:greenwo...@comcast.net] 
Sent: Wednesday, February 14, 2018 6:45 AM
To: 'git@vger.kernel.org' 
Subject: git-bashe.exe fails to launch

Resending as Plain Text.

After upgrading to 2.16.1.windows.4, I have been unable to launch “C:\Program 
Files\Git\git-bash.exe” from installed shortcut or from command line (cmd).  In 
both cases, the bash console flashes and closes immediately.

I am able to invoke “C:\Program Files\Git\bin\bash.exe” from the command line, 
in which case I appear to have full git functionality.

I have tried uninstalling and reinstalling a number of times to no effect.

Comparing a failing launch to a working launch on another computer using 
Process Explorer, the problem may be related to HarddiskVolume3.  This error 
shows up near the end of the failing launch.  Note the odd quote and “I” at the 
end of the path.

High Resolution Date & Time: 2/14/2018 6:14:13.7853585 AM Event Class:  
 File System
Operation: CreateFile
Result:  INVALID PARAMETER
Path: \Device\HarddiskVolume3Ἵ
TID:   13904
Duration:   0.095
Desired Access: Generic Read
Disposition:   Open
Options: Synchronous IO Non-Alert, Non-Directory File, Complete If 
Oplocked, Open By ID
Attributes: n/a
ShareMode:  Read, Write, Delete
AllocationSize:  n/a

This is my first post to this mailing list.  Please let me know if I need to 
register somewhere to see replies.

git-bashe.exe fails to launch

[greenwood9]

Resending as Plain Text.

After upgrading to 2.16.1.windows.4, I have been unable to launch “C:\Program 
Files\Git\git-bash.exe” from installed shortcut or from command line (cmd).  In 
both cases, the bash console flashes and closes immediately.

I am able to invoke “C:\Program Files\Git\bin\bash.exe” from the command line, 
in which case I appear to have full git functionality.

I have tried uninstalling and reinstalling a number of times to no effect.

Comparing a failing launch to a working launch on another computer using 
Process Explorer, the problem may be related to HarddiskVolume3.  This error 
shows up near the end of the failing launch.  Note the odd quote and “I” at the 
end of the path.

High Resolution Date & Time: 2/14/2018 6:14:13.7853585 AM
Event Class:   File System
Operation: CreateFile
Result:  INVALID PARAMETER
Path: \Device\HarddiskVolume3Ἵ
TID:   13904
Duration:   0.095
Desired Access: Generic Read
Disposition:   Open
Options: Synchronous IO Non-Alert, Non-Directory File, Complete If 
Oplocked, Open By ID
Attributes: n/a
ShareMode:  Read, Write, Delete
AllocationSize:  n/a

This is my first post to this mailing list.  Please let me know if I need to 
register somewhere to see replies.

Re: [PATCH v3 29/42] completion: use __gitcomp_builtin in _git_notes

[SZEDER Gábor]

On Fri, Feb 9, 2018 at 12:02 PM, Nguyễn Thái Ngọc Duy  wrote:

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index c7b8b37f19..60127daebf 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash

> @@ -1851,15 +1851,17 @@ _git_notes ()
> add,--reedit-message=*|append,--reedit-message=*)
> __git_complete_refs --cur="${cur#*=}"
> ;;
> -   add,--*|append,--*)
> -   __gitcomp '--file= --message= --reedit-message=
> -   --reuse-message='
> +   add,--*)
> +   __gitcomp_builtin notes_add
> +   ;;
> +   append,--*)
> +   __gitcomp_builtin notes_append
> ;;
> copy,--*)
> -   __gitcomp '--stdin'
> +   __gitcomp_builtin notes_copy
> ;;
> prune,--*)
> -   __gitcomp '--dry-run --verbose'
> +   __gitcomp_builtin notes_prune
> ;;
> prune,*)
> ;;

This could be simplified to:

  add,--*|append,--*|copy,--*|prune,--*)
__gitcomp_builtin notes_$subcommand
;;

And we could even go one step further:

  *,--*)
__gitcomp_builtin notes_$subcommand
;;

This would have the benefit that if any of the remaining subcommands
learn --options, then they would be completed right away, without the
need to add that subcommand to the case arms.  Case in point is 'git
notes remove', which already accepts two options, but they are not
completed because of the missing 'remove,--*)' case arm.  The same would
also apply to 'git notes merge's options, except that the completion
script completely misses the 'merge' subcommand in the first place
(blame on me, 2a5da75579 (bash: support more 'git notes' subcommands and
their options, 2010-10-10)).

This also applies to the completion functions of other commands with
subcommands ('remote', 'worktree').

The downside is that we would run 'git cmd subcmd
--git-completion-helper' even if a subcommand doesn't use parse options.
I think that's acceptable.

Re: What's cooking in git.git (Feb 2018, #02; Tue, 13)

[Derrick Stolee]

On 2/13/2018 8:51 PM, Junio C Hamano wrote:

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

 http://git-blame.blogspot.com/p/git-public-repositories.html

--


Hi Junio,

There have been a few "What's cooking" emails since I submitted v1 of 
"Serialized Git Commit Graph" and it has not appeared with a tracking 
branch. Is this a mistake, or is it something about the state of the review?


Thanks,
-Stolee

[1] 
https://public-inbox.org/git/20180125140231.65604-1-dsto...@microsoft.com/

    Patch v1, Jan 25

[2] 
https://public-inbox.org/git/1517348383-112294-1-git-send-email-dsto...@microsoft.com/

    Patch v2, Jan 30

[3] 
https://public-inbox.org/git/1518122258-157281-1-git-send-email-dsto...@microsoft.com/

    Patch v3, Feb 8

Re: [PATCH 2/2] init-db: change --template type to OPTION_FILENAME

[Jeff King]

On Wed, Feb 14, 2018 at 05:51:49PM +0700, Nguyễn Thái Ngọc Duy wrote:

> OPTION_FILENAME has some magic behind the scene, like prefixing which is
> useless for init-db. The $HOME expansion though does come handy and
> makes --template more consistent with the rest (both env and config var
> get $HOME expansion).

Yep, makes sense.

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 68ff4ad75a..d6bd9f19cb 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -473,8 +473,9 @@ int cmd_init_db(int argc, const char **argv, const char 
> *prefix)
>   const char *template_dir = NULL;
>   unsigned int flags = 0;
>   const struct option init_db_options[] = {
> - OPT_STRING(0, "template", _dir, 
> N_("template-directory"),
> - N_("directory from which templates will be 
> used")),
> + { OPTION_FILENAME, 0, "template", _dir,
> + N_("template-directory"),
> + N_("directory from which templates will be used")},

It's a shame we can't use the slightly more readable OPT_FILENAME(), but
it forces the use of "file" for the argument name. I wonder if it really
ought to be OPT_PATH(), and say "path", which would work more
universally.

At any rate, I'm fine with this until somebody feels like fiddling with
the macros.

-Peff

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Jeff King]

On Wed, Feb 14, 2018 at 05:51:48PM +0700, Nguyễn Thái Ngọc Duy wrote:

> When you specify "--path ~/foo", the shell will automatically expand
> ~/foo to $HOME/foo before it's passed to git. The expansion is not done
> on "--path=~/foo". An experienced user sees the difference but it could
> still be confusing for others (especially when tab-completion still
> works on --path=~/foo).
> 
> Support $HOME expansion for all filename options. There are about seven
> of them.

I think this probably makes sense.

>  parse-options.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Should this be mentioned in the comment documenting OPT_FILENAME()?

> diff --git a/parse-options.c b/parse-options.c
> index d265a756b5..c33f14c74e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -38,10 +38,13 @@ static int get_arg(struct parse_opt_ctx_t *p, const 
> struct option *opt,
>  
>  static void fix_filename(const char *prefix, const char **file)
>  {
> - if (!file || !*file || !prefix || is_absolute_path(*file)
> - || !strcmp("-", *file))
> + if (!file || !*file || is_absolute_path(*file) ||
> + !strcmp("-", *file))
>   return;
> - *file = prefix_filename(prefix, *file);
> + if (**file == '~')
> + *file = expand_user_path(*file, 0);
> + else if (prefix)
> + *file = prefix_filename(prefix, *file);
>  }

I thought at first this needed a final "else" clause, because we don't
assign to *file if we have neither a prefix nor a user-path. But that's
what the callers expect (and we are similarly a noop if we hit the first
conditional). So this looks right.

-Peff

Re: Regression in memory consumption of git fsck

[Jeff King]

On Wed, Feb 14, 2018 at 01:48:28PM +0100, SZEDER Gábor wrote:

> > If I revert that commit (on top of current master) the memory
> > consumption goes down to 2GB again. The change looks relatively harmless
> > to me, so does anyone know what's going on here?
> 
> I could reproduce the increased memory usage even for much smaller
> repositories.  The patch below seems to fix it for me.
> 
> 
>  -- >8 --
> 
> Subject: [PATCH] fsck: plug tree buffer leak

I think this is fixed already in ba3a08ca0e (fsck: fix leak when
traversing trees, 2018-01-20), which is in 'next' (and marked for "will
merge to master in yesterday's "what's cooking").

-Peff

Re: [PATCH v6 5/7] convert: add 'working-tree-encoding' attribute

[Lars Schneider]

> On 10 Feb 2018, at 10:48, Torsten Bögershausen  wrote:
> 
> On Fri, Feb 09, 2018 at 02:28:28PM +0100, lars.schnei...@autodesk.com wrote:
>> From: Lars Schneider 
>> 
>> ...
>> 
>> +Please note that using the `working-tree-encoding` attribute may have a
>> +number of pitfalls:
>> +
>> +- Git clients that do not support the `working-tree-encoding` attribute
> 
> A client to Git ?
> Or may be "third party Git implementations"

OK, I'll go with "Third party Git implementations".


>> 
>> +As an example, use the following attributes if your '*.proj' files are
>> +UTF-16 encoded with byte order mark (BOM) and you want Git to perform
>> +automatic line ending conversion based on your platform.
>> +
>> +
>> +*.proj  text working-tree-encoding=UTF-16
>> +
>> +
>> +Use the following attributes if your '*.proj' files are UTF-16 little
>> +endian encoded without BOM and you want Git to use Windows line endings
>> +in the working directory. Please note, it is highly recommended to
>> +explicitly define the line endings with `eol` if the `working-tree-encoding`
>> +attribute is used to avoid ambiguity.
>> +
>> +
>> +*.proj  working-tree-encoding=UTF-16LE text eol=CRLF
>> +
>> +
>> +You can get a list of all available encodings on your platform with the
>> +following command:
> 
> One question:
> +*.proj   text working-tree-encoding=UTF-16
> vs
> *.projworking-tree-encoding=UTF-16LE text eol=CRLF
> 
> Technically the order of attributes doesn't matter, but that is not what we
> want to demonstrate here and now.
> I would probably move the "text" attribute to the end of the line.
> So that readers don't start to wonder if the order is important.

I agree in general. However, I would move "text" to the beginning to be
consistent with the gitattribute pattern above. OK?


>> 
>> +if (has_prohibited_utf_bom(enc->name, src, src_len)) {
>> +const char *error_msg = _(
>> +"BOM is prohibited for '%s' if encoded as %s");
>> +const char *advise_msg = _(
>> +"You told Git to treat '%s' as %s. A byte order mark "
>> +"(BOM) is prohibited with this encoding. Either use "
>> +"%.6s as working tree encoding or remove the BOM from 
>> the "
>> +"file.");
> 
> "You told Git" is probly right from Gits point of view, and advises are 
> really helpfull.
> But what should the user do about it ?
> Could we give a better advise ?
> 
> 
> "A byte order mark (BOM) is prohibited with %s.
> Please remove the BOM from the file %s 
> or use "%s as working-tree-encoding"
> 
> I would probably suspect that a tool wrote the BOM, and that is
> good and can or should not be changed by a user.
> 
> So a simply message like this could be the preferred (and only)
> solution for a user:
> "A byte order mark (BOM) is prohibited with %s.
> Please use "%s as working-tree-encoding"

OK. I like the last one!


> (And why %.6s and not simply %s ?)

The encodings is UTF-16LE, UTF-16BE, UTF-32LE, or UTF-32BE.
I just use the first 6 characters to print the encoding that
allows BOMs (UTF-16 or UTF-32). I'll add a comment to explain 
the trickery in the code!

Thanks,
Lars

Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am

[SZEDER Gábor]

On Fri, Feb 9, 2018 at 12:01 PM, Nguyễn Thái Ngọc Duy  wrote:
> The new completable options are:
>
> --directory
> --exclude
> --gpg-sign
> --include
> --keep-cr
> --keep-non-patch
> --message-id
> --no-keep-cr
> --patch-format
> --quiet
> --reject
> --resolvemsg=
>
> In-progress options like --continue will be part of --git-completion-helper
> then filtered out by _git_am() unless the operation is in progress. This
> helps keep marking of these operations in just one place.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  contrib/completion/git-completion.bash | 11 ---
>  parse-options.h|  4 ++--
>  rerere.h   |  3 ++-
>  3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 1e0bd835fe..eba482eb9c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1105,12 +1105,13 @@ __git_count_arguments ()
>  }
>
>  __git_whitespacelist="nowarn warn error error-all fix"
> +__git_am_inprogress_options="--skip --continue --resolved --abort"
>
>  _git_am ()
>  {
> __git_find_repo_path
> if [ -d "$__git_repo_path"/rebase-apply ]; then
> -   __gitcomp "--skip --continue --resolved --abort"
> +   __gitcomp "$__git_am_inprogress_options"
> return
> fi
> case "$cur" in
> @@ -1119,12 +1120,8 @@ _git_am ()
> return
> ;;
> --*)
> -   __gitcomp "
> -   --3way --committer-date-is-author-date --ignore-date
> -   --ignore-whitespace --ignore-space-change
> -   --interactive --keep --no-utf8 --signoff --utf8
> -   --whitespace= --scissors
> -   "
> +   __gitcomp_builtin am "--no-utf8" \
> +   "$__git_am_inprogress_options"
> return
> esac
>  }
> diff --git a/parse-options.h b/parse-options.h
> index 3c32401736..009cd863e5 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -144,8 +144,8 @@ struct option {
>  #define OPT_STRING_LIST(s, l, v, a, h) \
> { OPTION_CALLBACK, (s), (l), (v), (a), \
>   (h), 0, _opt_string_list }
> -#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, \
> - (h), PARSE_OPT_NOARG, 
> _opt_tertiary }
> +#define OPT_UYN(s, l, v, h, f)  { OPTION_CALLBACK, (s), (l), (v), NULL, \
> + (h), PARSE_OPT_NOARG|(f), 
> _opt_tertiary }
>  #define OPT_DATE(s, l, v, h) \
> { OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,\
>   parse_opt_approxidate_cb }

Shouldn't this hunk go into a commit of its own?  Or at least it would
deserve a mention in the commit message.


> diff --git a/rerere.h b/rerere.h
> index c2961feaaa..5e5a312e4c 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *);
>  extern void rerere_gc(struct string_list *);
>
>  #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
> -   N_("update the index with reused conflict resolution if possible"))
> +   N_("update the index with reused conflict resolution if possible"), \
> +   PARSE_OPT_NOCOMPLETE)
>
>  #endif
> --
> 2.16.1.207.gedba492059
>

Re: Regression in memory consumption of git fsck

[SZEDER Gábor]

> I've noticed that recent versions of git consume a lot of memory during
> "git fsck", to the point where I've regularly had git fall victim to
> Linux's OOM killer.
> 
> For example, if I clone torvalds/linux.git, and then run "git fsck
> --connectivity-only" in the newly cloned repository, git will consume
> more than 6GB of physical memory, while older versions peak at about 2GB.
> 
> I've managed to bisect this down to this commit in v2.14:
> 
> ad2db4030e42890e569de529e3cd61a8d03de497
> fsck: remove redundant parse_tree() invocation
> 
> If I revert that commit (on top of current master) the memory
> consumption goes down to 2GB again. The change looks relatively harmless
> to me, so does anyone know what's going on here?

I could reproduce the increased memory usage even for much smaller
repositories.  The patch below seems to fix it for me.


 -- >8 --

Subject: [PATCH] fsck: plug tree buffer leak

Commit ad2db4030e (fsck: remove redundant parse_tree() invocation,
2017-07-18), along with that redundant call to parse_tree() in
traverse_one_object(), also removed a call to free_tree_buffer() from
that function.  This resulted in significantly increased memory usage
of 'git fsck' because of all the non-freed tree buffers; in case of
git.git and '--connectivity-only' it went from around 270MB to over
1.2GB.

Restore that free_tree_buffer() call to bring down memory usage to the
previous level.

Reported-by: Dominic Sacré 
Signed-off-by: SZEDER Gábor 
---

 builtin/fsck.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 7a8a679d4f..8bc1b59daf 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -180,7 +180,10 @@ static void mark_object_reachable(struct object *obj)
 
 static int traverse_one_object(struct object *obj)
 {
-   return fsck_walk(obj, obj, _walk_options);
+   int result = fsck_walk(obj, obj, _walk_options);
+   if (obj->type == OBJ_TREE)
+   free_tree_buffer((struct tree *)obj);
+   return result;
 }
 
 static int traverse_reachable(void)
-- 
2.16.1.347.gd41f2872c6

ipad air 2

[mostafa shahdadi]

Sent from my iPad

[PATCH] am: support --quit

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

Among the "in progress" commands, only git-am and git-merge do not
support --quit. Support --quit in git-am too.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-am.txt   |  6 +-
 builtin/am.c   | 12 ++--
 contrib/completion/git-completion.bash |  2 +-
 t/t4150-am.sh  | 12 
 4 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 12879e4029..460662e4b9 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 [--exclude=] [--include=] [--reject] [-q | --quiet]
 [--[no-]scissors] [-S[]] [--patch-format=]
 [( | )...]
-'git am' (--continue | --skip | --abort)
+'git am' (--continue | --skip | --abort | --quit)
 
 DESCRIPTION
 ---
@@ -167,6 +167,10 @@ default.   You can use `--no-utf8` to override this.
 --abort::
Restore the original branch and abort the patching operation.
 
+--quit::
+   Abort the patching operation but keep HEAD and the index
+   untouched.
+
 DISCUSSION
 --
 
diff --git a/builtin/am.c b/builtin/am.c
index 5bdd2d7578..793c1e2765 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2149,7 +2149,8 @@ enum resume_mode {
RESUME_APPLY,
RESUME_RESOLVED,
RESUME_SKIP,
-   RESUME_ABORT
+   RESUME_ABORT,
+   RESUME_QUIT
 };
 
 static int git_am_config(const char *k, const char *v, void *cb)
@@ -2249,6 +2250,9 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
OPT_CMDMODE(0, "abort", ,
N_("restore the original branch and abort the patching 
operation."),
RESUME_ABORT),
+   OPT_CMDMODE(0, "quit", ,
+   N_("abort the patching operation but keep HEAD where it 
is."),
+   RESUME_QUIT),
OPT_BOOL(0, "committer-date-is-author-date",
_date_is_author_date,
N_("lie about committer date")),
@@ -2317,7 +2321,7 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
 * stray directories.
 */
if (file_exists(state.dir) && !state.rebasing) {
-   if (resume == RESUME_ABORT) {
+   if (resume == RESUME_ABORT || resume == RESUME_QUIT) {
am_destroy();
am_state_release();
return 0;
@@ -2359,6 +2363,10 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
case RESUME_ABORT:
am_abort();
break;
+   case RESUME_QUIT:
+   am_rerere_clear();
+   am_destroy();
+   break;
default:
die("BUG: invalid resume value");
}
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 88813e9124..c7d5c7af29 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1077,7 +1077,7 @@ _git_am ()
 {
__git_find_repo_path
if [ -d "$__git_repo_path"/rebase-apply ]; then
-   __gitcomp "--skip --continue --resolved --abort"
+   __gitcomp "--skip --continue --resolved --abort --quit"
return
fi
case "$cur" in
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 73b67b4280..512c754e02 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -1045,4 +1045,16 @@ test_expect_success 'am works with multi-line in-body 
headers' '
git cat-file commit HEAD | grep "^$LONG$"
 '
 
+test_expect_success 'am --quit keeps HEAD where it is' '
+   mkdir .git/rebase-apply &&
+   >.git/rebase-apply/last &&
+   >.git/rebase-apply/next &&
+   git rev-parse HEAD^ >.git/ORIG_HEAD &&
+   git rev-parse HEAD >expected &&
+   git am --quit &&
+   test_path_is_missing .git/rebase-apply &&
+   git rev-parse HEAD >actual &&
+   test_cmp expected actual
+'
+
 test_done
-- 
2.16.1.435.g8f24da2e1a

[PATCH 2/2] init-db: change --template type to OPTION_FILENAME

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

OPTION_FILENAME has some magic behind the scene, like prefixing which is
useless for init-db. The $HOME expansion though does come handy and
makes --template more consistent with the rest (both env and config var
get $HOME expansion).

Noticed-by: Doron Behar 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/init-db.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 68ff4ad75a..d6bd9f19cb 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -473,8 +473,9 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
const char *template_dir = NULL;
unsigned int flags = 0;
const struct option init_db_options[] = {
-   OPT_STRING(0, "template", _dir, 
N_("template-directory"),
-   N_("directory from which templates will be 
used")),
+   { OPTION_FILENAME, 0, "template", _dir,
+   N_("template-directory"),
+   N_("directory from which templates will be used")},
OPT_SET_INT(0, "bare", _bare_repository_cfg,
N_("create a bare repository"), 1),
{ OPTION_CALLBACK, 0, "shared", _shared_repository,
-- 
2.16.1.435.g8f24da2e1a

[PATCH 1/2] parse-options: expand $HOME on filename options

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

When you specify "--path ~/foo", the shell will automatically expand
~/foo to $HOME/foo before it's passed to git. The expansion is not done
on "--path=~/foo". An experienced user sees the difference but it could
still be confusing for others (especially when tab-completion still
works on --path=~/foo).

Support $HOME expansion for all filename options. There are about seven
of them.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 parse-options.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index d265a756b5..c33f14c74e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -38,10 +38,13 @@ static int get_arg(struct parse_opt_ctx_t *p, const struct 
option *opt,
 
 static void fix_filename(const char *prefix, const char **file)
 {
-   if (!file || !*file || !prefix || is_absolute_path(*file)
-   || !strcmp("-", *file))
+   if (!file || !*file || is_absolute_path(*file) ||
+   !strcmp("-", *file))
return;
-   *file = prefix_filename(prefix, *file);
+   if (**file == '~')
+   *file = expand_user_path(*file, 0);
+   else if (prefix)
+   *file = prefix_filename(prefix, *file);
 }
 
 static int opt_command_mode_error(const struct option *opt,
-- 
2.16.1.435.g8f24da2e1a

Regression in memory consumption of git fsck

[Dominic Sacré]

Hi,

I've noticed that recent versions of git consume a lot of memory during
"git fsck", to the point where I've regularly had git fall victim to
Linux's OOM killer.

For example, if I clone torvalds/linux.git, and then run "git fsck
--connectivity-only" in the newly cloned repository, git will consume
more than 6GB of physical memory, while older versions peak at about 2GB.

I've managed to bisect this down to this commit in v2.14:

ad2db4030e42890e569de529e3cd61a8d03de497
fsck: remove redundant parse_tree() invocation

If I revert that commit (on top of current master) the memory
consumption goes down to 2GB again. The change looks relatively harmless
to me, so does anyone know what's going on here?


Thanks,

Dominic

[BUG] git init doesn't respect `--template` like configuration variable init.templateDir and $GIT_TEMPLATE_DIR

[Doron Behar]

The title says it all.

If I run `git init --template=~/path/to/my/template` I get the following
message:

warning: templates not found ~/path/to/my/template

But, if I run:

$ env GIT_TEMPLATE_DIR=~/path/to/my/template git init

I don't get a warning and the template is used just fine.

If I configure the configuration variable `init.templateDir` to
`~/path/to/my/template` and run `git init` I don't get a warning and the
template is used just fine.




signature.asc
Description: PGP signature

Re: What's cooking in git.git (Feb 2018, #02; Tue, 13)

[Duy Nguyen]

On Wed, Feb 14, 2018 at 8:51 AM, Junio C Hamano  wrote:
> * nd/parseopt-completion (2018-02-09) 42 commits
>  - git-completion.bash: add GIT_COMPLETION_OPTIONS=all config
>  - completion: use __gitcomp_builtin in _git_worktree
>  - completion: use __gitcomp_builtin in _git_tag
>  - completion: use __gitcomp_builtin in _git_status
>  - completion: use __gitcomp_builtin in _git_show_branch
>  - completion: use __gitcomp_builtin in _git_rm
>  - completion: use __gitcomp_builtin in _git_revert
>  - completion: use __gitcomp_builtin in _git_reset
>  - completion: use __gitcomp_builtin in _git_replace
>  - remote: force completing --mirror= instead of --mirror
>  - completion: use __gitcomp_builtin in _git_remote
>  - completion: use __gitcomp_builtin in _git_push
>  - completion: use __gitcomp_builtin in _git_pull
>  - completion: use __gitcomp_builtin in _git_notes
>  - completion: use __gitcomp_builtin in _git_name_rev
>  - completion: use __gitcomp_builtin in _git_mv
>  - completion: use __gitcomp_builtin in _git_merge_base
>  - completion: use __gitcomp_builtin in _git_merge
>  - completion: use __gitcomp_builtin in _git_ls_remote
>  - completion: use __gitcomp_builtin in _git_ls_files
>  - completion: use __gitcomp_builtin in _git_init
>  - completion: use __gitcomp_builtin in _git_help
>  - completion: use __gitcomp_builtin in _git_grep
>  - completion: use __gitcomp_builtin in _git_gc
>  - completion: use __gitcomp_builtin in _git_fsck
>  - completion: use __gitcomp_builtin in _git_fetch
>  - completion: use __gitcomp_builtin in _git_difftool
>  - completion: use __gitcomp_builtin in _git_describe
>  - completion: use __gitcomp_builtin in _git_config
>  - completion: use __gitcomp_builtin in _git_commit
>  - completion: use __gitcomp_builtin in _git_clone
>  - completion: use __gitcomp_builtin in _git_clean
>  - completion: use __gitcomp_builtin in _git_cherry_pick
>  - completion: use __gitcomp_builtin in _git_checkout
>  - completion: use __gitcomp_builtin in _git_branch
>  - completion: use __gitcomp_builtin in _git_apply
>  - completion: use __gitcomp_builtin in _git_am
>  - completion: use __gitcomp_builtin in _git_add
>  - git-completion.bash: introduce __gitcomp_builtin
>  - parse-options: let OPT__FORCE take optional flags argument
>  - parse-options: add OPT_xxx_F() variants
>  - parse-options: support --git-completion-helper
>
>  Will see another reroll.
>  cf. 

Re: [PATCH 3/7] worktree move: new command

[Duy Nguyen]

On Wed, Feb 14, 2018 at 10:16 AM, Jeff King  wrote:
> Hmm. That is not too bad, but somehow it feels funny to me to be
> polluting each test script with these annotations. And to be driving it
> from inside the test scripts.
>
> It seems like:
>
>   make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"
>
> would be sufficient.

And all new test files are considered leak-free by default? I like that!

> And updating the list would just be:
>
>   # assume we're using prove, which will keep running after failure,
>   # and will record the results for us to parse (using "--state=").
>   # Otherwise use "make -k" and grep in t/test-results.
>   make SANITIZE=leak test
>   (cd t && prove --dry --state=failed) |
>   perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
>   sort >known-leaky
>
> That would update both now-passing and now-failing tests. Presumably
> we'd keep it checked in, so "git diff" would show you the changes.
>
> -Peff



-- 
Duy

[PATCH] t/: correct obvious typo "detahced"

[Robert P. J. Day]

Signed-off-by: Robert P. J. Day 

---

diff --git a/t/t7409-submodule-detached-work-tree.sh 
b/t/t7409-submodule-detached-work-tree.sh
index c20717181..fc018e363 100755
--- a/t/t7409-submodule-detached-work-tree.sh
+++ b/t/t7409-submodule-detached-work-tree.sh
@@ -6,7 +6,7 @@
 test_description='Test submodules on detached working tree

 This test verifies that "git submodule" initialization, update and addition 
works
-on detahced working trees
+on detached working trees
 '

 TEST_NO_CREATE_REPO=1

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH v2] dir.c: ignore paths containing .git when invalidating untracked cache

[Junio C Hamano]

On Tue, Feb 13, 2018 at 5:24 PM, Duy Nguyen  wrote:
> I am worried that always doing the right thing may carry performance
> penalty (this is based purely on reading verify_path() code, no actual
> benchmarking). For safety, you can always set safe_path to zero. But
> if you do a lot of invalidation and something starts to slow down,
> then you can consider setting safe_path to 1 (if it's actually safe to
> do so).

Fair enough. Thanks for articulating the reasoning.