Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

[Duy Nguyen]

On Wed, Dec 27, 2017 at 07:50:29PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > This needs SYMLINKS prereq, which in turn means it cannot be tested
> > on certain platforms.  I thought Duy's answer was that this does not
> > need to involve a symbolic link at all?  If so, perhaps we can have
> > another test that does not need symlink?
> 
> ... as soon as I figure out how to add such a non-symlink test as well
> (seems sensible to test both), but I can't bring it to fail without
> symlinks, I just adjusted my test script to do this instead:
> 
> (
> rm -rf /tmp/testrepo &&
> git init /tmp/testrepo &&
> cd /tmp/testrepo &&
> mkdir x y &&
> touch x/a y/b &&
> git add x/a y/b &&
> git commit -msnap &&
> git rm -rf y &&
> mkdir y &&
> touch y/c &&
> git add y &&
> git commit -msnap2 &&
> git checkout HEAD~ &&
> git status &&
> git checkout master &&
> sleep 1 &&
> git status &&
> git status
> )
> 
> Duy, what am I missing here?

The problem is there, it's just easier to see or verify with
symlinks. Below is my test patch on top of your original one. Two
points:

- if you look at the test-dump-untracked-cache output, you can see the
  saved cache is wrong. The line

/one/  recurse valid

  should not be there because that implies that cached travesal of
  root includes the directory "one" which does not exist on disk
  anymore. With the fix, this line is gone.

- We silently ignore opendir() error, the changes in dir.c shows this

warning: could not open directory 'one/': Not a directory

  It opendir() again because it finds out the stat data of directory
  "one" in the cache does not match stat data of the (real) file
  "one".

  If "one" is a symlink, opendir() would be succesful and we go in
  anyway. If it's a file, we ignore it, accidentally make the second
  git-status output clean and pass the test.

Report opendir() errors is a good and should be done regardless (i'm
just not sure if it should be a fatal error or a warning like this, I
guess die() is a bit too much).

The remaining question is how we write this test. Verify with
test-dump-untracked-cache is easiest but less intuitive, I
guess. While using symlinks shows the problem clearly but not
portable. Or the third option, if we error something out, you could
check that git-status has clean stderr.

Which way to go?

-- 8< --
diff --git a/dir.c b/dir.c
index 3c54366a17..868f544d72 100644
--- a/dir.c
+++ b/dir.c
@@ -1783,15 +1783,20 @@ static int open_cached_dir(struct cached_dir *cdir,
   struct strbuf *path,
   int check_only)
 {
+   const char *c_path;
+
memset(cdir, 0, sizeof(*cdir));
cdir->untracked = untracked;
if (valid_cached_dir(dir, untracked, istate, path, check_only))
return 0;
-   cdir->fdir = opendir(path->len ? path->buf : ".");
+   c_path = path->len ? path->buf : ".";
+   cdir->fdir = opendir(c_path);
if (dir->untracked)
dir->untracked->dir_opened++;
-   if (!cdir->fdir)
+   if (!cdir->fdir) {
+   warning_errno(_("could not open directory '%s'"), c_path);
return -1;
+   }
return 0;
 }
 
diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index 7cf1e2c091..ca63b80ca7 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -702,7 +702,7 @@ test_expect_success 'setup worktree for symlink test' '
git add one/file two/file &&
git commit -m"first commit" &&
git rm -rf one &&
-   ln -s two one &&
+   cp two/file one &&
git add one &&
git commit -m"second commit"
 '
@@ -714,6 +714,7 @@ test_expect_failure '"status" after symlink replacement 
should be clean with UC=
git checkout master &&
avoid_racy &&
status_is_clean &&
+   test-dump-untracked-cache &&
status_is_clean
 '
 
-- 8< --

Re: Bring together merge and rebase

[Carl Baldwin]

On Wed, Dec 27, 2017 at 03:35:58PM +0200, Alexei Lozovsky wrote:
> I think the reasoning behind Theo's words is that it would be better
> to first implement the commit relationship tracking as an add-in which
> uses commit messages for data storage, then evaluate its usefulness
> when it's actually available (including extensions to gitk and stuff
> to support the new metadata), and then it could be moved into core git
> data structures, when it has proven itself useful. It's not a trivial
> feature which warrants immediate addition to git and its design can
> change when faced with real- world use-cases, so it would be bad for
> compatibility to rush its addition. Storage location for metadata
> seems to be an implementation detail which could be technically
> changed more or less easily. But it's much easier to ignore a trailer
> in commit message in the favor of a commit header field than to
> replace a deprecated commit header field with a better one, which
> could cause massive headache for all git repositories in the world.

Yeah, this is a point that everyone is eager to make instead of really
trying to understand what I'm trying to do and offering constructive
suggestions. It's not that I'm not listening. I'm not really concerned
about headers vs trailers or the asthetics of the whole thing as much as
I'm concerned about how the server / client interaction will be. I worry
that anything that I come up with that isn't implemented in the regular
git core push and fetch will end up being awkward or end up needing to
reimplement a lot of what's already in git. But, maybe it just needs a
little more thought. Let me try to think through it...

Imagine John posts a new change up for review to a review server. The
current master points at commit A and so he grabs it and drafts his
first proposal, B1.

digraph history {
B1 -> A
}

Soon after posting, he notices a couple of simple errors and uses the
web UI to correct them. This creates B2. (Dashed edges are replaces
references).

digraph history {
B1 -> A
B2 -> A
B2 -> B1 [ style="dashed"; ]
}

Anna reviews B2 and finds a small nit. She asks John if she can just fix
it and push up a new review. He agrees. She pushes up B3.

digraph history {
B1 -> A
B2 -> A
B3 -> A
B2 -> B1 [ style="dashed"; ]
B3 -> B2 [ style="dashed"; ]
}

John goes back to his workspace and does a little more work on B. He
creates the fourth revision, B4 but since he didn't update his workspace
with the other two most recent revisions, his new revision is derived
from B1.

digraph history {
B1 -> A
B2 -> A
B3 -> A
B4 -> A
B2 -> B1 [ style="dashed"; ]
B3 -> B2 [ style="dashed"; ]
B4 -> B1 [ style="dashed"; ]
}

John then pushes to the server. I imagined that would be a command
similar to what gerrit does.

git push codereview refs/for/master

At this point, I want a couple of things to happen. First, the server
should be able to match the new revision to the change by following the
replaces references to the commits it already has. Then it should
recognize that this is not a fast forward update to the change and
reject it on those grounds.

After that, John needs to be able to fetch B2 and B3 so that his local
client can perform a merge. I guess John needs to know what change he's
trying to fetch. In this case, he needs to fetch both B2 and B3 in order
get the full history graph of the change. The problem I see here is that
today's git fetch would see B2 and B3 as unrelated branches. There could
be any number of them to fetch. So, how does he ask for everything
related to the change? Does he do a wild card or something?

git fetch codereview refs/changes/123/*

Or does he just fetch all refs (this could be many on a busy review
server)? Or do we need to do something out of band to discover the list
of references that need to be fetched?

I've been thinking out loud a bit. I guess this could be a path forward.
I guess to make gc happy, I've got to keep around a ref pointing at each
new revision so that it doesn't get garbage collected.

Carl

[PATCH v5 26/34] 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.

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 9e31baaf3..78f707d0d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1508,6 +1508,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
@@ -1757,6 +1842,59 @@ static void compute_collisions(struct hashmap 
*collisions,
}
 }
 
+static char *check_for_directory_rename(struct merge_options *o,
+   const char *path,
+   struct tree 

[PATCH v5 17/34] 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.

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 e95eac2c7..cdd0afa04 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1653,13 +1653,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.15.0.408.g8e199d483

[PATCH v5 15/34] merge-recursive: move the get_renames() function

[Elijah Newren]

I want to re-use some other functions in the file without moving those
other functions or dealing with a handful of annoying split function
declarations and definitions.

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 d78853d5e..08bf26b9c 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,
@@ -1380,6 +1311,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 = DIFF_DETECT_RENAME;
+   opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
+   

[PATCH v5 05/34] directory rename detection: directory splitting testcases

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index d8ead7c56..acf49d6b4 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -427,4 +427,141 @@ 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 3 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 1 -eq $(git ls-files -o | wc -l) &&
+
+   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 &&
+   test_i18ngrep "CONFLICT.*directory rename split" out
+   )
+'
+
+# 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 &&
+
+   test 3 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 1 -eq $(git ls-files -o | wc -l) &&
+
+   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
+   )
+'
+
+###
+# Rules suggested by section 2:
+#
+#   None; the rule was already covered in section 1.  These testcases are
+#   here just to make sure the conflict resolution and necessary warning
+#   messages are 

[PATCH v5 06/34] directory rename detection: testcases to avoid taking detection too far

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index acf49d6b4..433d99584 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -564,4 +564,154 @@ 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 &&
+
+   test 3 -eq $(git ls-files -s | wc -l) &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:x/d &&
+   git rev-parse >expect \
+   O:z/b O:z/c O: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 5 -eq $(git ls-files -s | wc -l) &&
+   test 3 -eq $(git ls-files -u | wc -l) &&
+   test 1 -eq $(git ls-files -o | wc -l) &&
+
+   git rev-parse >actual \
+   :0:y/b :0:y/c :1:z/d :2:x/d :3:w/d &&
+   git rev-parse >expect \
+   O:z/b O:z/c O:z/d O:z/d O:z/d &&
+   test_cmp expect actual &&
+
+   test ! -f z/d &&
+   git hash-object >actual \
+ 

[PATCH v5 27/34] 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.

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 78f707d0d..01934bc1e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -418,6 +418,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.
@@ -428,12 +443,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.15.0.408.g8e199d483

[PATCH v5 16/34] 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().

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 08bf26b9c..e95eac2c7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1636,6 +1636,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;
@@ -1988,7 +2014,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
@@ -2002,9 +2029,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;
@@ -2029,16 +2055,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.15.0.408.g8e199d483

[PATCH v5 09/34] directory rename detection: testcases checking which side did the rename

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 468d6d537..50fb8f41e 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1138,4 +1138,325 @@ 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 &&
+
+   test 2 -eq $(git ls-files -s | wc -l) &&
+   test 1 -eq $(git ls-files -u | wc -l) &&
+   test 1 -eq $(git ls-files -o | wc -l) &&
+
+   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 &&
+
+   test 3 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 0 -eq $(git ls-files -o | wc -l) &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:z/d &&
+   git rev-parse >expect \
+   O:z/b O:z/c B:z/d &&
+   test_cmp expect actual
+   )
+'
+
+# Testcase 6c, 

[PATCH v5 01/34] Tighten and correct a few testcases for merging and cherry-picking

[Elijah Newren]

t3501 had a testcase originally added in 05f2dfb965 (cherry-pick:
demonstrate a segmentation fault, 2016-11-26) to ensure cherry-pick
wouldn't segfault when working with a dirty file involved in a rename.
While the segfault was fixed, there was another problem this test
demonstrated: namely, that git would overwrite a dirty file involved in a
rename.  Further, the test encoded a "successful merge" and overwriting of
this file as correct behavior.  Modify the test so that it would still
catch the segfault, but to require the correct behavior.  Mark it as
test_expect_failure for now too, since this second bug is not yet fixed.

t7607 had a test added in 30fd3a5425 (merge overwrites unstaged changes in
renamed file, 2012-04-15) specific to looking for a merge overwriting a
dirty file involved in a rename, but it too actually encoded what I would
term incorrect behavior: it expected the merge to succeed.  Fix that, and
add a few more checks to make sure that the merge really does produce the
expected results.

Signed-off-by: Elijah Newren 
---
 t/t3501-revert-cherry-pick.sh | 7 +--
 t/t7607-merge-overwrite.sh| 5 -
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 4f2a263b6..783bdbf59 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' '
test_cmp expect actual
 '
 
-test_expect_success 'cherry-pick works with dirty renamed file' '
+test_expect_failure 'cherry-pick works with dirty renamed file' '
test_commit to-rename &&
git checkout -b unrelated &&
test_commit unrelated &&
@@ -150,7 +150,10 @@ test_expect_success 'cherry-pick works with dirty renamed 
file' '
test_tick &&
git commit -m renamed &&
echo modified >renamed &&
-   git cherry-pick refs/heads/unrelated
+   test_must_fail git cherry-pick refs/heads/unrelated >out &&
+   test_i18ngrep "Refusing to lose dirty file at renamed" out &&
+   test $(git rev-parse :0:renamed) = $(git rev-parse HEAD^:to-rename.t) &&
+   grep -q "^modified$" renamed
 '
 
 test_done
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 9444d6a9b..00617dadf 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -97,7 +97,10 @@ test_expect_failure 'will not overwrite unstaged changes in 
renamed file' '
git mv c1.c other.c &&
git commit -m rename &&
cp important other.c &&
-   git merge c1a &&
+   test_must_fail git merge c1a >out &&
+   test_i18ngrep "Refusing to lose dirty file at other.c" out &&
+   test -f other.c~HEAD &&
+   test $(git hash-object other.c~HEAD) = $(git rev-parse c1a:c1.c) &&
test_cmp important other.c
 '
 
-- 
2.15.0.408.g8e199d483

[PATCH v5 13/34] directory rename detection: tests for handling overwriting untracked files

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index c731a1b03..7248c3807 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2768,4 +2768,341 @@ 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 &&
+
+   test 1 -eq $(git ls-files -s | wc -l) &&
+   test 4 -eq $(git ls-files -o | wc -l) &&
+
+   test "very" = "$(cat z/c)" &&
+   test "important" = "$(cat z/d)" &&
+   test $(git rev-parse HEAD:z/b) = $(git rev-parse O:z/b)
+   )
+'
+
+# 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 &&
+
+   test 3 -eq $(git ls-files -s | wc -l) &&
+   test 2 -eq $(git ls-files -u | wc -l) &&
+   test 5 -eq $(git ls-files -o | wc -l) &&
+
+   test $(git rev-parse :0:y/b) = $(git rev-parse O:z/b) &&
+   test "very" = "$(cat y/c)" &&
+
+   

[PATCH v5 02/34] merge-recursive: fix logic ordering issue

[Elijah Newren]

merge_trees() did a variety of work, including:
  * Calling get_unmerged() to get unmerged entries
  * Calling record_df_conflict_files() with all unmerged entries to
do some work to ensure we could handle D/F conflicts correctly
  * Calling get_renames() to check for renames.

An easily overlooked issue is that get_renames() can create more
unmerged entries and add them to the list, which have the possibility of
being involved in D/F conflicts.  So the call to
record_df_conflict_files() should really be moved after all the rename
detection.  I didn't come up with any testcases demonstrating any bugs
with the old ordering, but I suspect there were some for both normal
renames and for directory renames.  Fix the ordering.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index d00b27438..98c84e73d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1982,10 +1982,10 @@ int merge_trees(struct merge_options *o,
get_files_dirs(o, merge);
 
entries = get_unmerged();
-   record_df_conflict_files(o, entries);
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);
+   record_df_conflict_files(o, entries);
if (clean < 0)
goto cleanup;
for (i = entries->nr-1; 0 <= i; i--) {
-- 
2.15.0.408.g8e199d483

[PATCH v5 12/34] 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.  :-)

Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 536 +++-
 1 file changed, 535 insertions(+), 1 deletion(-)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 7c75363e5..c731a1b03 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -295,6 +295,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}
@@ -575,7 +576,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
@@ -2234,4 +2235,537 @@ 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 &&
+
+   test 7 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 0 -eq $(git ls-files -o | wc -l) &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/i &&
+   git rev-parse >expect \
+   O:z/b O:z/c B: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/e O:z/d/f O:z/d/g B: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" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+ 

[PATCH v5 18/34] 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.

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 cdd0afa04..da7c67eb8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1329,8 +1329,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;
@@ -1648,6 +1646,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);
@@ -1658,6 +1662,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.15.0.408.g8e199d483

[PATCH v5 25/34] 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.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 123 --
 1 file changed, 120 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 6bd4f34d5..9e31baaf3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1416,6 +1416,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)
 {
@@ -1654,6 +1679,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 = check_dir_renamed(pair->two->path,
+  dir_renames);
+   if (!dir_rename_ent)
+   continue;
+
+   new_path = apply_dir_rename(dir_rename_ent, pair->two->path);
+   if (!new_path)
+   /*
+* dir_rename_ent->non_unique_new_path is true, which
+* means there is no directory rename for us to use,
+* which means it won't cause us any additional
+* collisions.
+*/
+   continue;
+   collision_ent = collision_find_entry(collisions, new_path);
+   if (!collision_ent) {
+   collision_ent = xcalloc(1,
+   sizeof(struct collision_entry));
+   hashmap_entry_init(collision_ent, strhash(new_path));
+   hashmap_put(collisions, collision_ent);
+   collision_ent->target_file = new_path;
+ 

[PATCH v5 21/34] 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.

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 0cb27c66e..c5932d5c5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1695,6 +1695,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,
@@ -1725,16 +1731,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;
@@ -1750,10 +1753,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)
@@ -2149,7 +2152,7 @@ int merge_trees(struct merge_options *o,
}
 
 cleanup:
-   cleanup_renames(_info);
+   final_cleanup_renames(_info);
 
string_list_clear(entries, 1);
free(entries);
-- 
2.15.0.408.g8e199d483

[PATCH v5 20/34] merge-recursive: add a new hashmap for storing directory renames

[Elijah Newren]

This just adds dir_rename_entry and the associated functions; code using
these will be added in subsequent commits.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 35 +++
 merge-recursive.h |  8 
 2 files changed, 43 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index 4adff2d53..0cb27c66e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -49,6 +49,41 @@ 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(void *unused_cmp_data,
+ const struct dir_rename_entry *e1,
+ const struct dir_rename_entry *e2,
+ const void *unused_keydata)
+{
+   return strcmp(e1->dir, e2->dir);
+}
+
+static void dir_rename_init(struct hashmap *map)
+{
+   hashmap_init(map, (hashmap_cmp_fn) 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) {
diff --git a/merge-recursive.h b/merge-recursive.h
index 80d69d140..d7f4cc80c 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -29,6 +29,14 @@ struct merge_options {
struct string_list df_conflict_file_set;
 };
 
+struct dir_rename_entry {
+   struct hashmap_entry ent; /* must be the first member! */
+   char *dir;
+   unsigned non_unique_new_dir:1;
+   struct strbuf new_dir;
+   struct string_list possible_new_dirs;
+};
+
 /* merge_trees() but with recursive ancestor consolidation */
 int merge_recursive(struct merge_options *o,
struct commit *h1,
-- 
2.15.0.408.g8e199d483

[PATCH v5 24/34] merge-recursive: add a new hashmap for storing file collisions

[Elijah Newren]

Directory renames with the ability to merge directories opens up the
possibility of add/add/add/.../add conflicts, if each of the N
directories being merged into one target directory all had a file with
the same name.  We need a way to check for and report on such
collisions; this hashmap will be used for this purpose.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 23 +++
 merge-recursive.h |  7 +++
 2 files changed, 30 insertions(+)

diff --git a/merge-recursive.c b/merge-recursive.c
index d92fba277..6bd4f34d5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -84,6 +84,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) {
diff --git a/merge-recursive.h b/merge-recursive.h
index d7f4cc80c..e1be27f57 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -37,6 +37,13 @@ struct dir_rename_entry {
struct string_list possible_new_dirs;
 };
 
+struct collision_entry {
+   struct hashmap_entry ent; /* must be the first member! */
+   char *target_file;
+   struct string_list source_files;
+   unsigned reported_already:1;
+};
+
 /* merge_trees() but with recursive ancestor consolidation */
 int merge_recursive(struct merge_options *o,
struct commit *h1,
-- 
2.15.0.408.g8e199d483

[PATCH v5 00/34] 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.), and follow-up
series can 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/

Changes since v4:
  * Squashed Junio's GETTEXT_POISON fixes into the appropriate commits

Elijah Newren (34):
  Tighten and correct a few testcases for merging and cherry-picking
  merge-recursive: fix logic ordering issue
  merge-recursive: add explanation for src_entry and dst_entry
  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: add a new hashmap for storing directory renames
  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 a new hashmap for storing file collisions
  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   | 1231 ++-
 merge-recursive.h   |   17 +
 strbuf.c|   16 +
 strbuf.h|   16 +
 t/t3501-revert-cherry-pick.sh   |5 +-
 t/t6043-merge-rename-directories.sh | 3823 +++
 t/t7607-merge-overwrite.sh  |7 +-
 unpack-trees.c  |4 +-
 unpack-trees.h  |4 +
 9 files changed, 5007 insertions(+), 116 deletions(-)
 create mode 100755 t/t6043-merge-rename-directories.sh

-- 
2.15.0.408.g8e199d483

[PATCH v5 04/34] directory rename detection: basic testcases

[Elijah Newren]

Signed-off-by: Elijah Newren 
---
 t/t6043-merge-rename-directories.sh | 430 
 1 file changed, 430 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 0..d8ead7c56
--- /dev/null
+++ b/t/t6043-merge-rename-directories.sh
@@ -0,0 +1,430 @@
+#!/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 &&
+
+   test 4 -eq $(git ls-files -s | wc -l) &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/d HEAD:y/e/f &&
+   git rev-parse >expect \
+   O:z/b O:z/c B:z/d B:z/e/f &&
+   test_cmp expect actual &&
+   test "$(git hash-object y/d)" = $(git rev-parse B:z/d) &&
+   test_must_fail git rev-parse HEAD:z/d &&
+   test_must_fail git rev-parse HEAD:z/e/f &&
+   test ! -d z/d &&
+   test ! -d 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 &&
+
+   test 4 -eq $(git ls-files -s | wc -l) &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/d HEAD:y/e &&
+   git rev-parse >expect \
+   O:z/b O:z/c O:y/d A:z/e &&
+   test_cmp expect actual &&
+   test_must_fail git rev-parse HEAD:z/e
+   )
+'
+
+# 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?)
+#   

[PATCH v5 11/34] directory rename detection: testcases exploring possibly suboptimal merges

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index cfb53295b..7c75363e5 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1840,4 +1840,398 @@ 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 &&
+
+   test 6 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 0 -eq $(git ls-files -o | wc -l) &&
+
+   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/a O:x/b A:x/e A:y/f O:y/c O: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 &&
+   echo b1 >x/b &&
+   echo a2 >y/a &&
+   echo b2 >y/b &&
+   git add x y &&
+   test_tick &&
+ 

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

[Elijah Newren]

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 cdf8588c7..e77d2b043 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1138,6 +1138,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;
@@ -1156,7 +1176,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);
}
 
@@ -1282,6 +1303,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)))
@@ -1407,7 +1434,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 b12193e75..dd9b94266 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2873,7 +2873,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 &&
 
@@ -2944,7 +2944,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 &&
 
@@ -3013,7 +3013,7 @@ test_expect_success '10d-setup: Delete untracked with dir 
rename/rename(2to1)' '
)
 '
 
-test_expect_failure '10d-check: Delete 

[PATCH v5 19/34] 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.

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 86 +--
 1 file changed, 64 insertions(+), 22 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index da7c67eb8..4adff2d53 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1312,24 +1312,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;
@@ -1345,10 +1336,43 @@ 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 = malloc(sizeof(struct diff_queue_struct));
+   ret->queue = diff_queued_diff.queue;
+   ret->nr = diff_queued_diff.nr;
+   /* Ignore diff_queued_diff.alloc; we won't be changing size at all */
+
+   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);
@@ -1373,9 +1397,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;
 }
 
@@ -1646,15 +1667,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,
+

[PATCH v5 08/34] directory rename detection: files/directories in the way of some renames

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 28b2d2a2b..468d6d537 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -818,4 +818,324 @@ 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 6 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 1 -eq $(git ls-files -o | wc -l) &&
+
+   git rev-parse >actual \
+   :0:y/b :0:y/c :0:y/d :0:y/e :0:z/e &&
+   git rev-parse >expect \
+   O:z/b O:z/c O:y/d A:y/e A:z/e &&
+   test_cmp expect actual &&
+
+   test $(git rev-parse :0:y/f) = $(git rev-parse A:z/f) &&
+
+   test_i18ngrep "CONFLICT.*implicit dir rename" out
+   )
+'
+
+# 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 &&
+   git add y/d &&
+   test_tick &&
+   

[PATCH v5 31/34] merge-recursive: fix remaining directory rename + dirty overwrite cases

[Elijah Newren]

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 2b8a5ca03..fe42cabad 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1311,11 +1311,23 @@ 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);
+   }
+   /*
+* Stupid double negatives in remove_file; it somehow manages
+* to repeatedly mess me up.  So, just for myself:
+*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)) {
@@ -1453,7 +1465,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
@@ -2033,6 +2048,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,
@@ -2043,7 +2059,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 86c215e13..556029ef5 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3224,7 +3224,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 &&
 
@@ -3358,7 +3358,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 &&
 
@@ -3432,7 +3432,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 &&
 
@@ -3506,7 +3506,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.15.0.408.g8e199d483

[PATCH v5 28/34] 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.

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 01934bc1e..cdf8588c7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -177,6 +177,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,
@@ -607,6 +608,7 @@ struct rename {
 */
struct stage_data *src_entry;
struct stage_data *dst_entry;
+   unsigned add_turned_into_rename:1;
unsigned processed:1;
 };
 
@@ -641,6 +643,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,
@@ -1108,6 +1131,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,
@@ -1377,6 +1412,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);
}
@@ -1910,6 +1963,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,
+int *clean)
+{
+   struct 

[PATCH v5 22/34] merge-recursive: add get_directory_renames()

[Elijah Newren]

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

Signed-off-by: Elijah Newren 
---
 merge-recursive.c | 155 --
 1 file changed, 152 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index c5932d5c5..6aef357e7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1384,6 +1384,138 @@ 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/foo.c" -> "a/b/something-else/d/foo.c"
+* the "d/foo.c" part is the same, we just want to know that
+*"a/b/c" was renamed to "a/b/something-else"
+* so, for this example, this function returns "a/b/c" in
+* *old_dir and "a/b/something-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;
+
+   dir_renames = malloc(sizeof(struct hashmap));
+   dir_rename_init(dir_renames);
+   for (i = 0; i < pairs->nr; ++i) {
+   struct string_list_item *item;
+   int *count;
+   struct diff_filepair *pair = pairs->queue[i];
+   char *old_dir, *new_dir;
+
+   /* File not part of directory rename if it wasn't renamed */
+   if (pair->status != 'R')
+   continue;
+
+   get_renamed_dir_portion(pair->one->path, pair->two->path,
+   _dir,_dir);
+   if (!old_dir)
+   /* Directory didn't change at all; ignore this one. */
+   continue;
+
+   entry = dir_rename_find_entry(dir_renames, old_dir);
+   if (!entry) {
+   entry = xmalloc(sizeof(struct dir_rename_entry));
+   dir_rename_entry_init(entry, old_dir);
+   hashmap_put(dir_renames, entry);
+   } else {
+   free(old_dir);
+   }
+   item = string_list_lookup(>possible_new_dirs, new_dir);
+   if (!item) {
+   item = string_list_insert(>possible_new_dirs,
+ new_dir);
+   item->util = xcalloc(1, sizeof(int));
+   } else {
+   free(new_dir);
+   }
+   count = item->util;
+   *count += 1;
+   }
+
+   hashmap_iter_init(dir_renames, );
+   while ((entry = hashmap_iter_next())) {
+   int max = 0;
+   int bad_max = 0;
+   char *best = NULL;
+
+   for (i = 0; i < entry->possible_new_dirs.nr; i++) {
+   int *count = entry->possible_new_dirs.items[i].util;
+
+   if (*count == max)
+   bad_max = max;
+   else if (*count > max) {
+ 

[PATCH v5 10/34] directory rename detection: more involved edge/corner testcases

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 50fb8f41e..cfb53295b 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1459,4 +1459,385 @@ 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 &&
+
+   test 7 -eq $(git ls-files -s | wc -l) &&
+   test 6 -eq $(git ls-files -u | wc -l) &&
+   test 1 -eq $(git ls-files -o | wc -l) &&
+
+   test $(git rev-parse :0:y/d) = $(git rev-parse B:z/d) &&
+
+   git rev-parse >actual \
+   :1:z/b :2:y/b :3:w/b :1:z/c :2:y/c :3:x/c &&
+   git rev-parse >expect \
+   O:z/b O:z/b O:z/b O:z/c O:z/c O:z/c &&
+   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 w &&
+   test_tick &&
+   git commit -m "O" &&
+
+ 

[PATCH v5 23/34] 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.

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 6aef357e7..d92fba277 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1384,6 +1384,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[20];
+   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)
 {
@@ -1438,6 +1447,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 = dir_rename_find_entry(dir_re_head, merge_ent->dir);
+   

[PATCH v5 34/34] 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.

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 d00786f71..16c52a434 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2732,7 +2732,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
@@ -2740,8 +2739,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 ccfe48596..f0af66b8a 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3713,7 +3713,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.15.0.408.g8e199d483

[PATCH v5 30/34] 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 directy files that are involved in directory rename
detection will be added in a subsequent commit.

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 e77d2b043..2b8a5ca03 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -334,32 +334,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;
 }
@@ -792,6 +797,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;
@@ -2645,6 +2664,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,
@@ -2719,7 +2739,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);
@@ -2753,6 +2773,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, file_in_the_way,
+   o_oid, 

[PATCH v5 33/34] 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.

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 fe42cabad..d00786f71 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1951,7 +1951,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);
@@ -2178,7 +2178,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 0c205fac5..ccfe48596 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2000,18 +2000,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 &&
@@ -2044,29 +2049,29 @@ 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 &&
 
-   test 4 -eq $(git ls-files -s | wc -l) &&
-   test 1 -eq $(git ls-files -u | wc -l) &&
+   test 5 -eq $(git ls-files -s | wc -l) &&
+   test 2 -eq $(git ls-files -u | wc -l) &&
test 1 -eq $(git ls-files -o | wc -l) &&
 
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 -f y/d
+   test_must_fail git rev-parse :2:z/d &&
+   git ls-files -s z/d | grep ^100755 &&
+   test -f z/d &&
+   ! test -f y/d
)
 '
 
@@ -2080,16 +2085,6 @@ test_expect_success '8c-check: rename+modify/delete' '
 #
 #   Note: It would also be somewhat reasonable to resolve this as
 # 

[PATCH v5 32/34] 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.

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 556029ef5..0c205fac5 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -153,6 +153,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}
@@ -2760,6 +2761,67 @@ 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 &&
+
+   test 3 -eq $(git ls-files -s | wc -l) &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:x/d &&
+   git rev-parse >expect \
+   O:z/b O:z/c A:z/d &&
+   test_cmp expect actual
+   )
+'
+
 ###
 # Rules suggested by section 9:
 #
@@ -3541,4 +3603,226 @@ 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 >node2/leaf6 &&
+   git add node1 node2 &&
+   test_tick &&
+  

[PATCH v5 14/34] directory rename detection: tests for handling overwriting dirty files

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 7248c3807..bdffdb70b 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3105,4 +3105,440 @@ 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 2 -eq $(git ls-files -s | wc -l) &&
+   test 1 -eq $(git ls-files -u | wc -l) &&
+   test 4 -eq $(git ls-files -o | wc -l) &&
+
+   git rev-parse >actual \
+   :0:z/a :2:z/c &&
+   git rev-parse >expect \
+   O:z/a B:z/b &&
+   test_cmp expect actual &&
+
+   test "$(git hash-object z/c~HEAD)" = $(git rev-parse B:z/b) &&
+   test_cmp expected z/c
+   )
+'
+
+# 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 &&
+   echo stuff >>z/c &&
+
+   git merge -s recursive B^0 >out 2>err &&
+   test_i18ngrep "Refusing to lose dirty file at z/c" out &&
+
+   grep -q stuff */* 

[PATCH v5 07/34] directory rename detection: partially renamed directory testcase/discussion

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 433d99584..28b2d2a2b 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -714,4 +714,108 @@ 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.
+#
+# This section contains a test for this 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 &&
+
+   test 5 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 0 -eq $(git ls-files -o | wc -l) &&
+
+   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/b O:z/c O:z/d O:z/e B:z/f &&
+   test_cmp expect actual
+   )
+'
+
+###
+# Rules suggested by section 4:
+#
+#   Directory-rename-detection should be turned off for any directories (as
+#   a source for renames) that exist on both sides of the merge.  (The "as
+#   a source for renames" clarification is due to cases like 1c where
+#   the target directory exists on both sides and we do want the rename
+#   detection.)  But, sadly, see testcase 8b.
+###
+
 test_done
-- 
2.15.0.408.g8e199d483

[PATCH v5 03/34] merge-recursive: add explanation for src_entry and dst_entry

[Elijah Newren]

If I have to walk through the debugger and inspect the values found in
here in order to figure out their meaning, despite having known these
things inside and out some years back, then they probably need a comment
for the casual reader to explain their purpose.

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 98c84e73d..d78853d5e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -513,6 +513,25 @@ static void record_df_conflict_files(struct merge_options 
*o,
 
 struct rename {
struct diff_filepair *pair;
+   /*
+* Purpose of src_entry and dst_entry:
+*
+* If 'before' is renamed to 'after' then src_entry will contain
+* the versions of 'before' from the merge_base, HEAD, and MERGE in
+* stages 1, 2, and 3; dst_entry will contain the respective
+* versions of 'after' in corresponding locations.  Thus, we have a
+* total of six modes and oids, though some will be null.  (Stage 0
+* is ignored; we're interested in handling conflicts.)
+*
+* Since we don't turn on break-rewrites by default, neither
+* src_entry nor dst_entry can have all three of their stages have
+* non-null oids, meaning at most four of the six will be non-null.
+* Also, since this is a rename, both src_entry and dst_entry will
+* have at least one non-null oid, meaning at least two will be
+* non-null.  Of the six oids, a typical rename will have three be
+* non-null.  Only two implies a rename/delete, and four implies a
+* rename/add.
+*/
struct stage_data *src_entry;
struct stage_data *dst_entry;
unsigned processed:1;
-- 
2.15.0.408.g8e199d483

Re: [PATCH v2 6/7] wt-status.c: handle worktree renames

[Igor Djordjevic]

p.s. An extra note for the casual reader, just in case:

On 28/12/2017 01:50, Igor Djordjevic wrote:
> 
> ... it totally slipped me that documentation is/was pretty strict 
> about  being HEAD path (exclusively), where I was still 
> expecting it to show renamed working tree "from" value as  
> in case of working tree (double) rename, too - where that exact 
> (already renamed in index) path wasn`t to be found inside HEAD at 
> all, so the working tree rename couldn`t really be shown as "source" 
> and "target" rename pair, strictly following the "porcelain v2" 
> specification... :/
> 
> ...
> 
> I repeat that this looks like the correct approach, making fully 
> described working tree rename detection possible in porcelain in the 
> first place, but also aligning output of "status" --porcelain 
> variants with its default (--long) form.

Wherever "working tree rename detection" is discussed above, it`s all 
still about renamed file `git add -N` case only (where old name, 
appearing "deleted" from HEAD/index, is not staged yet), not some new 
functionality where renamed file inside working tree is instantly / 
magically recognized without being staged in the first place.

Re: What's cooking in git.git (Dec 2017, #05; Wed, 27)

[Elijah Newren]

On Wed, Dec 27, 2017 at 1:34 PM, Junio C Hamano  wrote:

> * ew/empty-merge-with-dirty-index (2017-12-22) 1 commit
>  - Merge branch 'ew/empty-merge-with-dirty-index-maint' into 
> ew/empty-merge-with-dirty-index
>  (this branch uses ew/empty-merge-with-dirty-index-maint.)

> * ew/empty-merge-with-dirty-index-maint (2017-12-22) 3 commits
>  - merge-recursive: avoid incorporating uncommitted changes in a merge
>  - move index_has_changes() from builtin/am.c to merge.c for reuse
>  - t6044: recursive can silently incorporate dirty changes in a merge
>  (this branch is used by ew/empty-merge-with-dirty-index.)
>
>  "git merge -s recursive" did not correctly abort when the index is
>  dirty, if the merged tree happened to be the same as the current
>  HEAD, which has been fixed.

As promised, I looked through both to check for mis-merges or problems
in applying to maint.  The changes all look good to me.  I was
surprised by the branch name, though.  Was 'ew/' a typo, or does that
part of the branch name mean something other than what I always
assumed?

Re: [PATCH v3 0/6] Renames in git-status "changed not staged" section

[Igor Djordjevic]

Hi Duy,

On 27/12/2017 11:18, Nguyễn Thái Ngọc Duy wrote:
> 
> v3 more or less goes back to v1 after my discussion with Igor about
> porcelain formats. So 7/7 is not needed anymore. 4/7 becomes 5/6. The
> meat is still in 6/6, now with some more updates in git-status.txt and
> to address the comment from Torsten.

Albeit a tiny concern expressed in that last e-mail[1], this now 
seems fine, and a few tests I did came back as expected. Thanks!

Regards, Buga

[1] 
https://public-inbox.org/git/CACsJy8A=jz9laum50gvjnt5gtdiyymymupbsrjfo4lmkvqs...@mail.gmail.com/T/#m18b4e2cb2b7685fcc9650f3fb71b2191ef74cbe1

Re: [PATCH v2 6/7] wt-status.c: handle worktree renames

[Igor Djordjevic]

On 27/12/2017 02:06, Duy Nguyen wrote:
> 
> >   ... 
> >
> >The pathname. In a renamed/copied entry, this
> >  is the path in the index and in the working tree.
> 
> Gaah.. as you may see in the other mail when I quoted this
> (incorrectly). I must have modified this file at some point and
> thought it was true (my version did not have "and in the worktree").

Ah, this explains a lot... :) I got really confused with your v2, it 
felt as the series took a strange turn, and in a kind of a subtle way :P

> The "and" is still problematic if you take this very seriously
> (because in this case index name and worktree name are different) but
> I think it's ok to ignore that "and" and switch it to "or".

Yes, I agree, and the change does feel like a good thing. But, I also 
now hope this doesn`t break any expectations, because... (read below)

> >The pathname in the commit at HEAD. This is only
> >  present in a renamed/copied entry, and tells
> >  where the renamed/copied contents came from.
> >
> > If I`m reading this correctly, it should be vice-versa - value from
> > HEAD, being "original-file", should come last, where value from
> > working tree ("new-file") should be first.

... it totally slipped me that documentation is/was pretty strict 
about  being HEAD path (exclusively), where I was still 
expecting it to show renamed working tree "from" value as  
in case of working tree (double) rename, too - where that exact 
(already renamed in index) path wasn`t to be found inside HEAD at 
all, so the working tree rename couldn`t really be shown as "source" 
and "target" rename pair, strictly following the "porcelain v2" 
specification... :/

I see now that your initial reply[1] was talking about this, but I 
didn`t focus on it much as you replied to it yourself shortly 
afterwards, and later v2 of the series came up.

Might be this is where you changed your offline documentation 
version, too, as that is what the sample patch was about :)

> Yeah I think the "where the renamed/copied contents came from" clears
> up my confusion in this format. Back to v1 it is!

I see you addressed this by loosening the restriction here a bit, too,
making  be "pathname in the commit at HEAD _or in the index_",
in your "[PATCH v3 6/6] wt-status.c: handle worktree renames"[2].

I repeat that this looks like the correct approach, making fully 
described working tree rename detection possible in porcelain in the 
first place, but also aligning output of "status" --porcelain 
variants with its default (--long) form.

Hopefully, on top of everything positive, it also doesn`t break 
anything (too much?)... :P Latest revision should now provide all the 
necessary ingredients to resolve what happened, for the (small?) 
price of tweaking previous expectations a bit.

Regards, Buga

[1] 
https://public-inbox.org/git/CACsJy8A=jz9laum50gvjnt5gtdiyymymupbsrjfo4lmkvqs...@mail.gmail.com/T/#mf2f5ae672ec6f4e1abecbd5fe65283e9d8fbed57
[2] https://public-inbox.org/git/20171227101839.26427-7-pclo...@gmail.com/T/#u

[PATCH] dir.c: avoid stat() in valid_cached_dir()

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

stat() may follow a symlink and return stat data of the link's target
instead of the link itself. We are concerned about the link itself.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I noticed this while looking at the untracked cache bug [1] but
 because it's not directly related to that bug, I'm submitting it
 separately here.

 [1] 
https://public-inbox.org/git/cacsjy8ambksp0mdlradcwn45veenc03b-gq8r8pqprdt9bm...@mail.gmail.com/

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

diff --git a/dir.c b/dir.c
index 7c4b45e30e..edcb7bb462 100644
--- a/dir.c
+++ b/dir.c
@@ -1809,7 +1809,7 @@ static int valid_cached_dir(struct dir_struct *dir,
 */
refresh_fsmonitor(istate);
if (!(dir->untracked->use_fsmonitor && untracked->valid)) {
-   if (stat(path->len ? path->buf : ".", )) {
+   if (lstat(path->len ? path->buf : ".", )) {
invalidate_directory(dir->untracked, untracked);
memset(>stat_data, 0, 
sizeof(untracked->stat_data));
return 0;
-- 
2.15.0.320.g0453912d77

[PATCH 0/1] Add blob find pickaxe option

[Stefan Beller]

This is a resend for sb/diff-blobfind, but now the option --find-object
is a native of the pickaxe family.

Stefan Beller (1):
  diffcore: add a pickaxe option to find a specific blob

 Documentation/diff-options.txt | 10 +++
 diff.c | 21 -
 diff.h |  3 ++
 diffcore-pickaxe.c | 14 +++--
 revision.c |  3 ++
 t/t4064-diff-oidfind.sh| 68 ++
 6 files changed, 115 insertions(+), 4 deletions(-)
 create mode 100755 t/t4064-diff-oidfind.sh

-- 
2.15.1.620.gb9897f4670-goog

diff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt
index 21d1776996..f9cf85db05 100644
--- c/Documentation/diff-options.txt
+++ w/Documentation/diff-options.txt
@@ -492,6 +492,15 @@ occurrences of that string did not change).
 See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
 information.
 
+--find-object=::
+   Look for differences that change the number of occurrences of
+   the specified object. Similar to `-S`, just the argument is different
+   in that it doesn't search for a specific string but for a specific
+   object id.
++
+The object can be a blob or a submodule commit. It implies the `-t` option in
+`git-log` to also find trees.
+
 --pickaxe-all::
When `-S` or `-G` finds a change, show all the changes in that
changeset, not just the files that contain the change
@@ -501,11 +510,6 @@ information.
Treat the  given to `-S` as an extended POSIX regular
expression to match.
 
---find-object=::
-   Restrict the output such that one side of the diff
-   matches the given object id. The object can be a blob,
-   gitlink entry or tree (when `-t` is given).
-
 endif::git-format-patch[]
 
 -O::
diff --git c/Makefile w/Makefile
index c26596c30a..ee9d5eb11e 100644
--- c/Makefile
+++ w/Makefile
@@ -775,7 +775,6 @@ LIB_OBJS += date.o
 LIB_OBJS += decorate.o
 LIB_OBJS += diffcore-break.o
 LIB_OBJS += diffcore-delta.o
-LIB_OBJS += diffcore-objfind.o
 LIB_OBJS += diffcore-order.o
 LIB_OBJS += diffcore-pickaxe.o
 LIB_OBJS += diffcore-rename.o
diff --git c/builtin/log.c w/builtin/log.c
index 08ea82d69f..6c1fa896ad 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -181,7 +181,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
init_display_notes(>notes_opt);
 
if (rev->diffopt.pickaxe || rev->diffopt.filter ||
-   rev->diffopt.flags.follow_renames || rev->diffopt.objfind)
+   rev->diffopt.flags.follow_renames)
rev->always_show_header = 0;
 
if (source)
diff --git c/diff.c w/diff.c
index e13b8229d3..7acddaaee7 100644
--- c/diff.c
+++ w/diff.c
@@ -4497,6 +4497,10 @@ static int parse_objfind_opt(struct diff_options *opt, 
const char *arg)
 
if (!opt->objfind)
opt->objfind = xcalloc(1, sizeof(*opt->objfind));
+
+   opt->pickaxe = ""; /* trigger pickaxe */
+   opt->flags.recursive = 1;
+   opt->flags.tree_in_recursive = 1;
oidset_insert(opt->objfind, );
return 1;
 }
@@ -5785,9 +5789,6 @@ void diffcore_std(struct diff_options *options)
diffcore_skip_stat_unmatch(options);
if (!options->found_follow) {
/* See try_to_follow_renames() in tree-diff.c */
-
-   if (options->objfind)
-   diffcore_objfind(options);
if (options->break_opt != -1)
diffcore_break(options->break_opt);
if (options->detect_rename)
diff --git c/diffcore-objfind.c w/diffcore-objfind.c
deleted file mode 100644
index 676bbfff00..00
--- c/diffcore-objfind.c
+++ /dev/null
@@ -1,42 +0,0 @@
-/*
- * Copyright (c) 2017 Google Inc.
- */
-#include "cache.h"
-#include "diff.h"
-#include "diffcore.h"
-
-static void diffcore_filter_blobs(struct diff_queue_struct *q,
- struct diff_options *options)
-{
-   int src, dst;
-
-   if (!options->objfind)
-   BUG("objfind oidset not initialized???");
-
-   for (src = dst = 0; src < q->nr; src++) {
-   struct diff_filepair *p = q->queue[src];
-
-   if (!DIFF_PAIR_UNMERGED(p) &&
-   ((DIFF_FILE_VALID(p->one) &&
-oidset_contains(options->objfind, >one->oid)) ||
-   (DIFF_FILE_VALID(p->two) &&
-oidset_contains(options->objfind, >two->oid {
-   q->queue[dst] = p;
-   dst++;
-   } else {
-   diff_free_filepair(p);
-   }
-   }
-
-   if (!dst) {
-   free(q->queue);
-   DIFF_QUEUE_CLEAR(q);
-   } else {
-   q->nr = dst;
-   }
-}
-
-void diffcore_objfind(struct diff_options *options)
-{
-   diffcore_filter_blobs(_queued_diff, options);
-}
diff --git 

[PATCH 1/1] diffcore: add a pickaxe option to find a specific blob

[Stefan Beller]

Sometimes users are given a hash of an object and they want to
identify it further (ex.: Use verify-pack to find the largest blobs,
but what are these? or [1])

One might be tempted to extend git-describe to also work with blobs,
such that `git describe ` gives a description as
':'.  This was implemented at [2]; as seen by the sheer
number of responses (>110), it turns out this is tricky to get right.
The hard part to get right is picking the correct 'commit-ish' as that
could be the commit that (re-)introduced the blob or the blob that
removed the blob; the blob could exist in different branches.

Junio hinted at a different approach of solving this problem, which this
patch implements. Teach the diff machinery another flag for restricting
the information to what is shown. For example:

  $ ./git log --oneline --find-object=v2.0.0:Makefile
  b2feb64309 Revert the whole "ask curl-config" topic for now
  47fbfded53 i18n: only extract comments marked with "TRANSLATORS:"

we observe that the Makefile as shipped with 2.0 was appeared in
v1.9.2-471-g47fbfded53 and in v2.0.0-rc1-5-gb2feb6430b.  The
reason why these commits both occur prior to v2.0.0 are evil
merges that are not found using this new mechanism.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob
[2] https://public-inbox.org/git/20171028004419.10139-1-sbel...@google.com/

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/diff-options.txt | 10 +++
 diff.c | 21 -
 diff.h |  3 ++
 diffcore-pickaxe.c | 14 +++--
 revision.c |  3 ++
 t/t4064-diff-oidfind.sh| 68 ++
 6 files changed, 115 insertions(+), 4 deletions(-)
 create mode 100755 t/t4064-diff-oidfind.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1d..f9cf85db05 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -492,6 +492,15 @@ occurrences of that string did not change).
 See the 'pickaxe' entry in linkgit:gitdiffcore[7] for more
 information.
 
+--find-object=::
+   Look for differences that change the number of occurrences of
+   the specified object. Similar to `-S`, just the argument is different
+   in that it doesn't search for a specific string but for a specific
+   object id.
++
+The object can be a blob or a submodule commit. It implies the `-t` option in
+`git-log` to also find trees.
+
 --pickaxe-all::
When `-S` or `-G` finds a change, show all the changes in that
changeset, not just the files that contain the change
@@ -500,6 +509,7 @@ information.
 --pickaxe-regex::
Treat the  given to `-S` as an extended POSIX regular
expression to match.
+
 endif::git-format-patch[]
 
 -O::
diff --git a/diff.c b/diff.c
index 0763e89263..7acddaaee7 100644
--- a/diff.c
+++ b/diff.c
@@ -4082,6 +4082,7 @@ void diff_setup(struct diff_options *options)
options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
options->flags.rename_empty = 1;
+   options->objfind = NULL;
 
/* pathchange left =NULL by default */
options->change = diff_change;
@@ -4487,6 +4488,23 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
+static int parse_objfind_opt(struct diff_options *opt, const char *arg)
+{
+   struct object_id oid;
+
+   if (get_oid(arg, ))
+   return error("unable to resolve '%s'", arg);
+
+   if (!opt->objfind)
+   opt->objfind = xcalloc(1, sizeof(*opt->objfind));
+
+   opt->pickaxe = ""; /* trigger pickaxe */
+   opt->flags.recursive = 1;
+   opt->flags.tree_in_recursive = 1;
+   oidset_insert(opt->objfind, );
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options,
   const char **av, int ac, const char *prefix)
 {
@@ -4736,7 +4754,8 @@ int diff_opt_parse(struct diff_options *options,
else if ((argcount = short_opt('O', av, ))) {
options->orderfile = prefix_filename(prefix, optarg);
return argcount;
-   }
+   } else if (skip_prefix(arg, "--find-object=", ))
+   return parse_objfind_opt(options, arg);
else if ((argcount = parse_long_opt("diff-filter", av, ))) {
int offending = parse_diff_filter_opt(optarg, options);
if (offending)
diff --git a/diff.h b/diff.h
index 0fb18dd735..8fa2ad8e2d 100644
--- a/diff.h
+++ b/diff.h
@@ -7,6 +7,7 @@
 #include "tree-walk.h"
 #include "pathspec.h"
 #include "object.h"
+#include "oidset.h"
 
 struct rev_info;
 struct diff_options;
@@ -174,6 +175,8 @@ struct diff_options {
enum diff_words_type word_diff;
enum diff_submodule_format submodule_format;
 
+   

Re: [PATCH v2 7/7] wt-status.c: avoid double renames in short/porcelain format

[Igor Djordjevic]

On 27/12/2017 01:49, Duy Nguyen wrote:
> 
> > I lost you a bit here, partially because of what seems to be an
> > incomplete setup script, partially because of this last sentence, as
> > Git v2.15.1 doesn`t seem to be showing this, so not sure about "like
> > we have been showing until now" part...?
> 
> Yeah I missed a "git add -N third" in the setup. And "until now" was a
> poor choice of words. It should have been "before 425a28e0a4", where
> "new files" could not show up, which prevented rename detection in the
> "Changed bot not staged for commit" section in the first place.
> ...
> Sorry again about "now". Before 425a28e0a4 rename detection would not
> kick in to find "second -> third" so people wouldn't know about rename
> anyway.

Yeah, no worries, I had I hunch that it might be what you meant, but 
got too involved in all the rest that I forgot to bring that one up, 
too. Thanks for clarifying, though.

Regards, Buga

Re: [PATCH 2/2] Windows: stop supplying BLK_SHA1=YesPlease by default

[Jonathan Nieder]

+git-for-windows
Ævar Arnfjörð Bjarmason wrote:

> Using BLK_SHA1 in lieu of the OpenSSL routines was done in [1], but
> since DC_SHA1 is now the default for git in general it makes sense for
> Windows to use that too, this looks like something that was missed
> back in [2].
>
> As noted in [3] OpenSSL has a performance benefit compared to BLK_SHA1
> on MinGW, so perhaps that and the Windows default should be changed
> around again, but that's a topic for another series, it seems clear
> that this specific flag is nobody's explicit intention.

I have some memory of performance issues on Windows when DC_SHA1 was
introduced leading to interest in a mixed configuration with DC_SHA1
only being used where it is security sensitive (e.g. for object naming
but not for packfile trailers).

Did anything come of that?

In any event removing this BLK_SHA1 setting looks like a good change
to me, but I'd rather that Windows folks weigh in.

Thanks,
Jonathan

[...]
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -361,7 +361,6 @@ ifeq ($(uname_S),Windows)
>   NO_REGEX = YesPlease
>   NO_GETTEXT = YesPlease
>   NO_PYTHON = YesPlease
> - BLK_SHA1 = YesPlease
>   ETAGS_TARGET = ETAGS
>   NO_INET_PTON = YesPlease
>   NO_INET_NTOP = YesPlease

Re: [PATCH 1/2] Makefile: NO_OPENSSL=1 should no longer imply BLK_SHA1=1

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Stop supplying BLK_SHA1=YesPlease when NO_OPENSSL=UnfortunatelyYes is
> supplied. This has been done ever since [1], when switching to DC_SHA1
> by default in [2] this should have been changed as well.

I had trouble parsing this, I think for a few reasons:

 1. Too much 'this', so I end up not being able to keep track of what
has been done ever since (1) and (2)

 2. The ',' should be a ';'

 3. Can the commit names go inline in the message instead of being
footnotes?  That way, my eye doesn't have to chase so far to
find what kind of dates you are talking about.

 4. Why should switching to DC_SHA1 by default have resulted in
simply dropping the NO_OPENSSL => BLK_SHA1 behavior?  At first
glance it would be more intuitive to change it to
NO_OPENSSL => DC_SHA1 instead.

Putting those all together, I end up with

 Use the collision detecting SHA-1 implementation by default even
 when NO_OPENSSL is set.

 Setting NO_OPENSSL=UnfortunatelyYes has implied BLK_SHA1=1 ever since
 the former was introduced in dd53c7ab29 (Support for NO_OPENSSL,
 2005-07-29).  That implication should have been removed when the
 default SHA-1 implementation changed from OpenSSL to DC_SHA1 in
 e6b07da278 (Makefile: make DC_SHA1 the default, 2017-03-17).  Finish
 what that commit started by removing the BLK_SHA1 fallback setting so
 the default DC_SHA1 implementation can be used.

What happens if I set both OPENSSL_SHA1 and NO_OPENSSL?  Should this
block set

OPENSSL_SHA1 =

so that the latter wins, or should we detect it as an error?

[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -23,7 +23,6 @@ all::
>  # it at all).
>  #
>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.
> -# This also implies BLK_SHA1.
>  #
>  # Define USE_LIBPCRE if you have and want to use libpcre. Various
>  # commands such as log and grep offer runtime options to use
> @@ -1260,7 +1259,6 @@ ifndef NO_OPENSSL
>   endif
>  else
>   BASIC_CFLAGS += -DNO_OPENSSL
> - BLK_SHA1 = 1
>   OPENSSL_LIBSSL =
>  endif
>  ifdef NO_OPENSSL
> diff --git a/configure.ac b/configure.ac
> index 2f55237e65..7f8415140f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -241,7 +241,6 @@ AC_MSG_NOTICE([CHECKS for site configuration])
>  # a bundled SHA1 routine optimized for PowerPC.
>  #
>  # Define NO_OPENSSL environment variable if you do not have OpenSSL.
> -# This also implies BLK_SHA1.

With or without some of those commit message tweaks
Reviewed-by: Jonathan Nieder 

Thanks,
Jonathan

Re: [PATCH 2/2] travis-ci: record and skip successfully built trees

[SZEDER Gábor]

On Thu, Dec 28, 2017 at 12:00 AM, SZEDER Gábor  wrote:
> On Wed, Dec 27, 2017 at 8:15 PM, Lars Schneider  
> wrote:
>>> + then
>>> + cat <<-EOF
>>> + Skipping build job for commit $TRAVIS_COMMIT.
>>> + This commit has already been built and tested 
>>> successfully by this build job.
>>> + To force a re-build delete the branch's cache and 
>>> then hit 'Restart job'.
>>> + EOF
>>> + else
>>> + cat <<-EOF
>>> + Skipping build job for commit $TRAVIS_COMMIT.
>>> + This commit's tree has already been built and tested 
>>> successfully in build job $prev_good_job_number for commit 
>>> $prev_good_commit.
>>> + The log of that build job is available at 
>>> https://travis-ci.org/$TRAVIS_REPO_SLUG/jobs/$prev_good_job_id
>>> + To force a re-build delete the branch's cache and 
>>> then hit 'Restart job'.
>>> + EOF
>>
>> Maybe add a few newlines before and after EOF to make the text more stand 
>> out?
>> Or print it in a different color? Maybe red?
>>
>> See: https://travis-ci.org/szeder/git/jobs/322247836#L622-L625
>
> I considered using color for the first line, but then didn't do it,
> because I didn't want to decide the color :)
> Anyway, red is the general error/failure color, but this is neither.  It
> could either be green, to match the color of the build job's result, or
> something neutral like blue or yellow.

OTOH, the message printed in skip_branch_tip_with_tag() is not
colorized, either.

[PATCH 0/2] When DC_SHA1 was made the default we missed a spot

[Ævar Arnfjörð Bjarmason]

When SHADC was made the default in 2.13.0 we forgot that NO_OPENSSL=1
would still turn it off implicitly, and that Windows would use
BLK_SHA1=1 by default thinking it would be getting the OpenSSL
version.

Ævar Arnfjörð Bjarmason (2):
  Makefile: NO_OPENSSL=1 should no longer imply BLK_SHA1=1
  Windows: stop supplying BLK_SHA1=YesPlease by default

 Makefile | 2 --
 config.mak.uname | 1 -
 configure.ac | 1 -
 3 files changed, 4 deletions(-)

-- 
2.15.1.424.g9478a66081

[PATCH 2/2] Windows: stop supplying BLK_SHA1=YesPlease by default

[Ævar Arnfjörð Bjarmason]

Using BLK_SHA1 in lieu of the OpenSSL routines was done in [1], but
since DC_SHA1 is now the default for git in general it makes sense for
Windows to use that too, this looks like something that was missed
back in [2].

As noted in [3] OpenSSL has a performance benefit compared to BLK_SHA1
on MinGW, so perhaps that and the Windows default should be changed
around again, but that's a topic for another series, it seems clear
that this specific flag is nobody's explicit intention.

1. 9bccfcdbff ("Windows: use BLK_SHA1 again", 2009-10-22)

2. e6b07da278 ("Makefile: make DC_SHA1 the default", 2017-03-17)

3. 2cfc70f0de ("mingw: use OpenSSL's SHA-1 routines", 2017-02-09)

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 config.mak.uname | 1 -
 1 file changed, 1 deletion(-)

diff --git a/config.mak.uname b/config.mak.uname
index 685a80d138..6a862abd35 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -361,7 +361,6 @@ ifeq ($(uname_S),Windows)
NO_REGEX = YesPlease
NO_GETTEXT = YesPlease
NO_PYTHON = YesPlease
-   BLK_SHA1 = YesPlease
ETAGS_TARGET = ETAGS
NO_INET_PTON = YesPlease
NO_INET_NTOP = YesPlease
-- 
2.15.1.424.g9478a66081

[PATCH 1/2] Makefile: NO_OPENSSL=1 should no longer imply BLK_SHA1=1

[Ævar Arnfjörð Bjarmason]

Stop supplying BLK_SHA1=YesPlease when NO_OPENSSL=UnfortunatelyYes is
supplied. This has been done ever since [1], when switching to DC_SHA1
by default in [2] this should have been changed as well.

1. dd53c7ab29 ("[PATCH] Support for NO_OPENSSL", 2005-07-29)

2. e6b07da278 ("Makefile: make DC_SHA1 the default", 2017-03-17)

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Makefile | 2 --
 configure.ac | 1 -
 2 files changed, 3 deletions(-)

diff --git a/Makefile b/Makefile
index 32c170687c..7e1da28b6f 100644
--- a/Makefile
+++ b/Makefile
@@ -23,7 +23,6 @@ all::
 # it at all).
 #
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
-# This also implies BLK_SHA1.
 #
 # Define USE_LIBPCRE if you have and want to use libpcre. Various
 # commands such as log and grep offer runtime options to use
@@ -1260,7 +1259,6 @@ ifndef NO_OPENSSL
endif
 else
BASIC_CFLAGS += -DNO_OPENSSL
-   BLK_SHA1 = 1
OPENSSL_LIBSSL =
 endif
 ifdef NO_OPENSSL
diff --git a/configure.ac b/configure.ac
index 2f55237e65..7f8415140f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -241,7 +241,6 @@ AC_MSG_NOTICE([CHECKS for site configuration])
 # a bundled SHA1 routine optimized for PowerPC.
 #
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
-# This also implies BLK_SHA1.
 #
 # Define OPENSSLDIR=/foo/bar if your openssl header and library files are in
 # /foo/bar/include and /foo/bar/lib directories.
-- 
2.15.1.424.g9478a66081

Re: [PATCH 2/2] travis-ci: record and skip successfully built trees

[SZEDER Gábor]

On Wed, Dec 27, 2017 at 8:15 PM, Lars Schneider
 wrote:
>
>> On 27 Dec 2017, at 17:49, SZEDER Gábor  wrote:
>> +# Skip the build job if the same tree has already been built and tested
>> +# successfully before (e.g. because the branch got rebased, changing only
>> +# the commit messages).
>> +skip_good_tree () {
>> + if ! good_tree_info="$(grep "^$(git rev-parse $TRAVIS_COMMIT^{tree}) " 
>> "$good_trees_file")"
>> + then
>> + # haven't seen this tree yet; continue the build job
>> + return
>> + fi
>> +
>> + echo "$good_tree_info" | {
>> + read tree prev_good_commit prev_good_job_number 
>> prev_good_job_id
>> +
>> + if test "$TRAVIS_JOB_ID" =  "$prev_good_job_id"
>
> Under what circumstances would that be true?

When the user hits 'Restart job' on the Travis CI web interface,
$TRAVI_JOB_NUMBER and _ID remain the same in the restarted build job as
they were in the original.
So the condition is true when the user hits 'Restart job' on a build job
that was the first to successfully build and test the current tree.

> Nit: One unintended space after = ?!

Ok.

>> + then
>> + cat <<-EOF
>> + Skipping build job for commit $TRAVIS_COMMIT.
>> + This commit has already been built and tested 
>> successfully by this build job.
>> + To force a re-build delete the branch's cache and then 
>> hit 'Restart job'.
>> + EOF
>> + else
>> + cat <<-EOF
>> + Skipping build job for commit $TRAVIS_COMMIT.
>> + This commit's tree has already been built and tested 
>> successfully in build job $prev_good_job_number for commit $prev_good_commit.
>> + The log of that build job is available at 
>> https://travis-ci.org/$TRAVIS_REPO_SLUG/jobs/$prev_good_job_id
>> + To force a re-build delete the branch's cache and then 
>> hit 'Restart job'.
>> + EOF
>
> Maybe add a few newlines before and after EOF to make the text more stand out?
> Or print it in a different color? Maybe red?
>
> See: https://travis-ci.org/szeder/git/jobs/322247836#L622-L625

I considered using color for the first line, but then didn't do it,
because I didn't want to decide the color :)
Anyway, red is the general error/failure color, but this is neither.  It
could either be green, to match the color of the build job's result, or
something neutral like blue or yellow.

Gábor

[PATCHv2 1/5] t/helper/test-lazy-name-hash: fix compilation

[Stefan Beller]

I was compiling origin/master today with stricter compiler flags today
and was greeted by

t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:
t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
 printf("avg [size %8d] [single %f] %c [multi %f %d]\n",
 ^~~
 nr,
 ~~~
 (double)avg_single/10,
 ~~
 (avg_single < avg_multi ? '<' : '>'),
 ~
 (double)avg_multi/10,
 ~
 nr_threads_used);
 
t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared 
here
  int nr_threads_used;
  ^~~

I do not see how we can arrive at that line without having `nr_threads_used`
initialized, as we'd have `count > 1`  (which asserts that we ran the
loop above at least once, such that it *should* be initialized).

I do not have time to dive into further analysis.

Signed-off-by: Stefan Beller 
---
 t/helper/test-lazy-init-name-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-lazy-init-name-hash.c 
b/t/helper/test-lazy-init-name-hash.c
index 6368a89345..297fb01d61 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -112,7 +112,7 @@ static void analyze_run(void)
 {
uint64_t t1s, t1m, t2s, t2m;
int cache_nr_limit;
-   int nr_threads_used;
+   int nr_threads_used = 0;
int i;
int nr;
 
-- 
2.15.1.620.gb9897f4670-goog

[PATCHv2 0/5] Fix --recurse-submodules for submodule worktree changes

[Stefan Beller]

I dropped the patch to `same()` as I realized we only need to fix the
oneway_merge function, the others (two, three way merge) are fine as
they have the checks already in place.

The test added in the last patch got slightly larger as now we also test for
newly staged files to be blown away in the submodule.

v1:

The fix is in the last patch, the first patches are just massaging the code
base to make the fix easy.

The second patch fixes a bug in the test, which was ineffective at testing.
The third patch shows the problem this series addresses,
the fourth patch is a little refactoring, which I want to keep separate
as I would expect it to be a performance regression[1].
The first patch is unrelated, but improves the readability of submodule test
cases, which we'd want to improve further.

Thanks,
Stefan

Stefan Beller (5):
  t/helper/test-lazy-name-hash: fix compilation
  t/lib-submodule-update.sh: clarify test
  t/lib-submodule-update.sh: Fix test ignoring ignored files in
submodules
  unpack-trees: oneway_merge to update submodules
  submodule: submodule_move_head omits old argument in forced case

 submodule.c |  4 +++-
 t/helper/test-lazy-init-name-hash.c |  2 +-
 t/lib-submodule-update.sh   | 19 +--
 unpack-trees.c  |  3 +++
 4 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.15.1.620.gb9897f4670-goog

[PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller 
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..ac59524a7f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
ie_match_stat(o->src_index, old, , 
CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
update |= CE_UPDATE;
}
+   if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
+   !verify_uptodate(old, o))
+   update |= CE_UPDATE;
add_entry(o, old, update, 0);
return 0;
}
-- 
2.15.1.620.gb9897f4670-goog

[PATCHv2 5/5] submodule: submodule_move_head omits old argument in forced case

[Stefan Beller]

When using hard reset or forced checkout with the option to recurse into
submodules, the submodules need to be reset, too.

It turns out that we need to omit the duplicate old argument to read-tree
in all forced cases to omit the 2 way merge and use the more assertive
behavior of reading the specific new tree into the index and updating
the working tree.

Signed-off-by: Stefan Beller 
---
 submodule.c   |  4 +++-
 t/lib-submodule-update.sh | 14 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index fa25888783..db0f7ac51e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path,
else
argv_array_push(, "-m");
 
-   argv_array_push(, old ? old : EMPTY_TREE_SHA1_HEX);
+   if (!(flags & SUBMODULE_MOVE_HEAD_FORCE))
+   argv_array_push(, old ? old : EMPTY_TREE_SHA1_HEX);
+
argv_array_push(, new ? new : EMPTY_TREE_SHA1_HEX);
 
if (run_command()) {
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index fb0173ea87..1f38a85371 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -1015,4 +1015,18 @@ test_submodule_forced_switch_recursing_with_args () {
test_submodule_content sub1 origin/modify_sub1
)
'
+
+   test_expect_success "$command: changed submodule worktree is reset" '
+   prolog &&
+   reset_work_tree_to_interested add_sub1 &&
+   (
+   cd submodule_update &&
+   rm sub1/file1 &&
+   : >sub1/new_file &&
+   git -C sub1 add new_file &&
+   $command HEAD &&
+   test_path_is_file sub1/file1 &&
+   test_path_is_missing sub1/new_file
+   )
+   '
 }
-- 
2.15.1.620.gb9897f4670-goog

[PATCHv2 2/5] t/lib-submodule-update.sh: clarify test

[Stefan Beller]

Keep the local branch name as the upstream branch name to avoid confusion.

Signed-off-by: Stefan Beller 
---
 t/lib-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 38dadd2c29..d7699046f6 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -664,8 +664,8 @@ test_submodule_recursing_with_args_common() {
cd submodule_update &&
git -C sub1 checkout -b keep_branch &&
git -C sub1 rev-parse HEAD >expect &&
-   git branch -t check-keep origin/modify_sub1 &&
-   $command check-keep &&
+   git branch -t modify_sub1 origin/modify_sub1 &&
+   $command modify_sub1 &&
test_superproject_content origin/modify_sub1 &&
test_submodule_content sub1 origin/modify_sub1 &&
git -C sub1 rev-parse keep_branch >actual &&
-- 
2.15.1.620.gb9897f4670-goog

[PATCHv2 3/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules

[Stefan Beller]

It turns out that the test replacing a submodule with a file with
the submodule containing an ignored file is incorrectly titled,
because the test put the file in place, but never ignored that file.
When having an untracked file Instead of an ignored file in the
submodule, git should refuse to remove the submodule, but that is
a bug in the implementation of recursing into submodules, such that
the test just passed, removing the untracked file.

Fix the test first; in a later patch we'll fix gits behavior,
that will make sure untracked files are not deleted.

Signed-off-by: Stefan Beller 
---
 t/lib-submodule-update.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index d7699046f6..fb0173ea87 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () {
(
cd submodule_update &&
git branch -t replace_sub1_with_file 
origin/replace_sub1_with_file &&
+   echo ignored >.git/modules/sub1/info/exclude &&
: >sub1/ignored &&
$command replace_sub1_with_file &&
test_superproject_content origin/replace_sub1_with_file 
&&
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules

[Ævar Arnfjörð Bjarmason]

On Fri, Dec 22 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> ---
>
> Thanks, but I thought the patch was already in 'next' for a week or
> more and it's time to refine incrementally.
>
> Here is the difference as I see between what we already have and
> this update, and a proposed summary.
>
> -- >8 --
> From: Ævar Arnfjörð Bjarmason 
> Subject: perl: avoid *.pmc and fix Error.pm further
>
> The previous round tried to use *.pmc files but it confused RPM
> dependency analysis on some distros.  Install them as plain
> vanilla *.pm files instead.
>
> Also "local @_" construct did not properly work when goto 
> is used until recent versions of Perl.  Avoid it (and we do not
> need to localize it here anyway).

When I read this back on the 22nd I missed that you were waiting on my
feedback on this. Just saw What's Cooking now. Yes, LGTM:

Acked-by: Ævar Arnfjörð Bjarmason 
(if needed)
Signed-off-by: Ævar Arnfjörð Bjarmason 

> ---
> diff --git a/Makefile b/Makefile
> index ba6607b7e7..5c73cd208a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2274,14 +2274,14 @@ endif
>  po/build/locale/%/LC_MESSAGES/git.mo: po/%.po
>   $(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<
>
> -PMFILES := $(wildcard perl/*.pm perl/*/*.pm perl/*/*/*.pm perl/*/*/*/*.pm)
> -PMCFILES := $(patsubst perl/%.pm,perl/build/lib/%.pmc,$(PMFILES))
> +LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm 
> perl/Git/*/*/*.pm)
> +LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
>
>  ifndef NO_PERL
> -all:: $(PMCFILES)
> +all:: $(LIB_PERL_GEN)
>  endif
>
> -perl/build/lib/%.pmc: perl/%.pm
> +perl/build/lib/%.pm: perl/%.pm
>   $(QUIET_GEN)mkdir -p $(dir $@) && \
>   sed -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' < $< > $@
>
> diff --git a/perl/Git/Error.pm b/perl/Git/Error.pm
> index 5874672150..09bbc97390 100644
> --- a/perl/Git/Error.pm
> +++ b/perl/Git/Error.pm
> @@ -39,7 +39,7 @@ sub import {
>   require Error;
>  };
>
> -local @_ = ($caller, @_);
> +unshift @_, $caller;
>  goto ::import;
>  }
>

Re: [PATCH 5/5] sequencer: do not invent whitespace when transforming OIDs

[Liam Beguin]

Hi Johannes,

On 23 December 2017 at 00:56, Johannes Schindelin
 wrote:
> For commands that do not have an argument, there is no need to append a
> trailing space at the end of the line.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5632415ea2d..970842e3fcc 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2584,7 +2584,10 @@ int transform_todos(unsigned flags)
> strbuf_addf(, " %s", oid);
> }
> /* add all the rest */
> -   strbuf_addf(, " %.*s\n", item->arg_len, item->arg);
> +   if (!item->arg_len)
> +   strbuf_addch(, '\n');
> +   else
> +   strbuf_addf(, " %.*s\n", item->arg_len, 
> item->arg);

I also went with that when I was working on this but I thought leaving the
extra whitespace would make the code a little shorter.
Other than that, this change and the others look good.

> }
>
> i = write_message(buf.buf, buf.len, todo_file, 0);
> --
> 2.15.1.windows.2

Thanks,
Liam

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Liam Beguin]

Hi Junio,


On 27 December 2017 at 20:15, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin  wrote:
>>> +static const char command_to_char(const enum todo_command command)
>>> +{
>>> +   if (command < TODO_COMMENT && todo_command_info[command].c)
>>> +   return todo_command_info[command].c;
>>> +   return comment_line_char;
>>> +}
>>
>> CC sequencer.o
>> sequencer.c:798:19: error: type qualifiers ignored on function return
>> type [-Werror=ignored-qualifiers]
>>  static const char command_to_char(const enum todo_command command)
>>^
>>
>> Maybe drop the first const.
>
> Thanks.  This topic has been in 'next' for quite some time and I
> wanted to merge it down to 'master' soonish, so I've added the
> following before doing so.

Thanks for taking the time. I had prepared the patch but was waiting to get
home to send it.
Only comment I have, maybe s/sequencer.c/rebase -i/ in the subject line
so it matches with the rest.

Since this came up, would it be a good thing to add -Wignored-qualifiers
to the DEVELOPER flags?

>
> -- >8 --
> From: Junio C Hamano 
> Date: Wed, 27 Dec 2017 11:12:45 -0800
> Subject: [PATCH] sequencer.c: drop 'const' from function return type
>
> With -Werror=ignored-qualifiers, a function that claims to return
> "const char" gets this error:
>
> CC sequencer.o
> sequencer.c:798:19: error: type qualifiers ignored on function return
> type [-Werror=ignored-qualifiers]
>  static const char command_to_char(const enum todo_command command)
>^
>
> Reported-by: Nguyễn Thái Ngọc Duy 
> Signed-off-by: Junio C Hamano 
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 115085d39c..2a407cbe54 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -795,7 +795,7 @@ static const char *command_to_string(const enum 
> todo_command command)
> die("Unknown command: %d", command);
>  }
>
> -static const char command_to_char(const enum todo_command command)
> +static char command_to_char(const enum todo_command command)
>  {
> if (command < TODO_COMMENT && todo_command_info[command].c)
> return todo_command_info[command].c;
> --
> 2.15.1-597-g62d91a8972
>

Thanks,
Liam

Re: [PATCH v3 3/4] travis-ci: save prove state for the 32 bit Linux build

[SZEDER Gábor]

On Wed, Dec 27, 2017 at 7:46 PM, Lars Schneider
 wrote:
>> + --volume "${HOME}/travis-cache:/tmp/travis-cache" \
>
> I assume "${HOME}/travis-cache:/usr/src/git/t/.prove" would not
> work because that would be a mapping in another mapping?

't/.prove' is a file, but '.../travis-cache' is a directory.  It must
be, because Travis CI caches whole directories.

Gábor

What's cooking in git.git (Dec 2017, #05; Wed, 27)

[Junio C Hamano]

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

--
[Graduated to "master"]

* bw/submodule-sans-cache-compat (2017-12-12) 3 commits
  (merged to 'next' on 2017-12-14 at 6893bdeed3)
 + submodule: convert get_next_submodule to not rely on the_index
 + submodule: used correct index in is_staging_gitmodules_ok
 + submodule: convert stage_updated_gitmodules to take a struct index_state

 Code clean-up.


* db/doc-workflows-neuter-the-maintainer (2017-12-08) 1 commit
  (merged to 'next' on 2017-12-14 at 740788d890)
 + doc: reword gitworkflows.txt for neutrality

 Docfix.


* es/clone-shared-worktree (2017-12-11) 1 commit
  (merged to 'next' on 2017-12-14 at 248ef92bda)
 + clone: support 'clone --shared' from a worktree

 "git clone --shared" to borrow from a (secondary) worktree did not
 work, even though "git clone --local" did.  Both are now accepted.


* es/worktree-checkout-hook (2017-12-07) 1 commit
  (merged to 'next' on 2017-12-14 at 907d958351)
 + worktree: invoke post-checkout hook (unless --no-checkout)

 "git worktree add" learned to run the post-checkout hook, just like
 "git checkout" does, after the initial checkout.


* jh/object-filtering (2017-12-05) 9 commits
  (merged to 'next' on 2017-12-05 at 3a56b51085)
 + rev-list: support --no-filter argument
 + list-objects-filter-options: support --no-filter
 + list-objects-filter-options: fix 'keword' typo in comment
  (merged to 'next' on 2017-11-27 at e5008c3b28)
 + pack-objects: add list-objects filtering
 + rev-list: add list-objects filtering support
 + list-objects: filter objects in traverse_commit_list
 + oidset: add iterator methods to oidset
 + oidmap: add oidmap iterator methods
 + dir: allow exclusions from blob in addition to file
 (this branch is used by jh/fsck-promisors and jh/partial-clone.)

 In preparation for implementing narrow/partial clone, the object
 walking machinery has been taught a way to tell it to "filter" some
 objects from enumeration.


* jk/cvsimport-quoting (2017-12-08) 1 commit
  (merged to 'next' on 2017-12-14 at ea99dc966c)
 + cvsimport: apply shell-quoting regex globally

 Typo/Logico fix.


* jt/decorate-api (2017-12-08) 1 commit
  (merged to 'next' on 2017-12-14 at b14858df40)
 + decorate: clean up and document API

 A few structures and variables that are implementation details of
 the decorate API have been renamed and then the API got documented
 better.


* jt/transport-no-more-rsync (2017-12-12) 1 commit
  (merged to 'next' on 2017-12-14 at 456913ad25)
 + transport: remove unused "push" in vtable
 (this branch is used by jt/transport-hide-vtable.)

 Code clean-up.


* ks/branch-cleanup (2017-12-07) 4 commits
  (merged to 'next' on 2017-12-14 at af0a906984)
 + builtin/branch: strip refs/heads/ using skip_prefix
 + branch: update warning message shown when copying a misnamed branch
 + branch: group related arguments of create_branch()
 + branch: improve documentation and naming of create_branch() parameters

 Code clean-up.


* lb/rebase-i-short-command-names (2017-12-05) 9 commits
  (merged to 'next' on 2017-12-14 at 0637be0782)
 + t3404: add test case for abbreviated commands
 + rebase -i: learn to abbreviate command names
 + rebase -i -x: add exec commands via the rebase--helper
 + rebase -i: update functions to use a flags parameter
 + rebase -i: replace reference to sha1 with oid
 + rebase -i: refactor transform_todo_ids
 + rebase -i: set commit to null in exec commands
 + Documentation: use preferred name for the 'todo list' script
 + Documentation: move rebase.* configs to new file

 With a configuration variable rebase.abbreviateCommands set,
 "git rebase -i" produces the todo list with a single-letter
 command names.


* ot/pretty (2017-12-12) 2 commits
  (merged to 'next' on 2017-12-14 at d80fe80aed)
 + format: create docs for pretty.h
 + format: create pretty.h file

 Code clean-up.


* rb/quick-install-doc (2017-12-12) 1 commit
  (merged to 'next' on 2017-12-14 at 96c17a83fa)
 + install-doc-quick: allow specifying what ref to install

 The build procedure now allows not just the repositories but also
 the refs to be used to take pre-formatted manpages and html
 documents to install.


* rs/am-builtin-leakfix (2017-12-07) 1 commit
  (merged to 'next' on 2017-12-14 at 30bf70d2cb)
 + am: release strbuf after use in split_mail_mbox()

 Leakfix.


* rs/fmt-merge-msg-leakfix (2017-12-08) 1 commit
  (merged to 'next' on 2017-12-14 at b87794d837)
 + transport-helper: plug strbuf and string_list leaks

 Leakfix.


* rs/fmt-merge-msg-string-leak-fix (2017-12-07) 1 commit
  (merged to 

Re: [PATCH 0/5] A couple of sequencer cleanups

[Junio C Hamano]

Johannes Schindelin  writes:

> Johannes Schindelin (5):
>   rebase: do not continue when the todo list generation failed
>   sequencer: strip bogus LF at end of error messages
>   sequencer: remove superfluous conditional
>   sequencer: report when noop has an argument
>   sequencer: do not invent whitespace when transforming OIDs

All looked obviously correct and good clean-up changes.  

Thanks; will queue.

Re: [PATCH] diffcore: add a filter to find a specific blob

[Junio C Hamano]

Jonathan Nieder  writes:

> Stefan Beller wrote:
>> On Thu, Dec 14, 2017 at 6:18 PM, Junio C Hamano  wrote:
>
>>> I think it would make it a better companion to --pickaxe but we need
>>> to align its behaviour a little bit so that it plays better with the
>>> "--pickaxe-all" option, and also needs to hide mode and name only
>>> changes just like pickaxe.
>>
>> I looked into this, and the small changes needed led me to thinking
>> it could be integrated into the diffcore-pickaxe code completely,
>> roughly like (spaces mangled):
>
> Nice, this looks promising.

Indeed it is very simple, straight-forward and nice ;-)

>> I disagree, as the user doesn't have the content, but the hash
>> over the content only and wants to know more about it

(correcting Stefan)  The user can do "git cat-file blob $oid" to
learn about the contents, so from end user's point of view, the
"pickaxe with object ID" can be seen as merely a short-hand for

git log -S"$(git cat-file blob $oid)"

almost.

But that is a tangent in this response.

> Interesting --- I come to the opposite conclusion.
>
> The pickaxe-style behavior seems more consistent and simpler to
> explain and better matches the use cases I can think of.

Yeah, I am more leaning toward agreeing with you.

Re: [PATCH 1/2] travis-ci: don't try to create the cache directory unnecessarily

[Jonathan Nieder]

SZEDER Gábor wrote:

> Travis CI creates that directory for us anyway, even when a previous
> cache doesn't exist for the current build job.
>
> In itself it doesn't hurt to try, of course, but the following patch
> will access the Travis CI cache much earlier in the build process, and
> then creating the cache directory this late might cause confusion.
>
> Signed-off-by: SZEDER Gábor 
> ---
>  ci/run-tests.sh | 1 -
>  1 file changed, 1 deletion(-)

Is this behavior documented anywhere?
https://docs.travis-ci.com/user/caching#Arbitrary-directories doesn't
say anything about it.

Thanks,
Jonathan

Re: [PATCH] Fix confusing wording

[Junio C Hamano]

Ivan Pozdeev  writes:

> Not sure if I should add a CVE-2009-0037 reference as well.

Not in an end-user facing message like this one, I would say.

>
> ---

Sign off?

> http.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 215bebe..26b3386 100644
> --- a/http.c
> +++ b/http.c
> @@ -802,7 +802,7 @@ static CURL *get_curl_handle(void)
> get_curl_allowed_protocols(-1));
> #else
> warning("protocol restrictions not applied to curl redirects because\n"
> - "your curl version is too old (>= 7.19.4)");
> + "your libcurl version is too old (< 7.19.4)");
> #endif

Between 'curl' and 'libcurl', the latter certainly is more
technically correct.  I have a feeling that it would be better to
spell out ">=" as "requires at least" than replacing it with equally
cryptic "<" (it is OK to spell it out as "your libcurl version is
older than minimum required 7.19.4", too).

Thanks.

Re: [PATCH 2/2] travis-ci: record and skip successfully built trees

[Lars Schneider]

> On 27 Dec 2017, at 17:49, SZEDER Gábor  wrote:
> 
> Travis CI dutifully builds and tests each new branch tip, even if its
> tree has previously been successfully built and tested.  This happens
> often enough in contributors' workflows, when a work-in-progress
> branch is rebased changing e.g. only commit messages or the order or
> number of commits while leaving the resulting code intact, and is then
> pushed to a Travis CI-enabled GitHub fork.
> 
> This is wasting Travis CI's resources and is sometimes scary-annoying
> when the new tip commit with a tree identical to the previous,
> successfully tested one is suddenly reported in red, because one of
> the OSX build jobs happened to exceed the time limit yet again.
> 
> So extend our Travis CI build scripts to skip building commits whose
> trees have previously been successfully built and tested.  Use the
> Travis CI cache feature to keep a record of the object names of trees
> that tested successfully, in a plain and simple flat text file, one
> line per tree object name.  Append the current tree's object name at
> the end of every successful build job to this file, along with a bit
> of additional info about the build job (commit object name, Travis CI
> job number and id).  Check, using a simple grep invocation, in each
> build job whether the current commit's tree is already in there, and
> skip the build if it is.  Include a message in the skipped build job's
> trace log, containing the URL to the build job successfully testing
> that tree for the first time and instructions on how to force a
> re-build.  Catch the case when a build job, which successfully built
> and tested a particular tree for the first time, is restarted and omit
> the URL of the previous build job's trace log, as in this case it's
> the same build job and the trace log has just been overwritten.
> 
> Using an ever-growing flat text file might seem like asking for
> trouble on the long run, but it's perfectly adequate for this purpose.
> Contributors' topic branches are short-lived in general, so this file
> won't grow large enough to cause any issues.  Grepping through several
> tens of thousands such lines is sufficiently fast, so not even
> git/git's forever living integration branches will cause scalability
> issues with the current rate of ~1 push/day for a couple of decades.
> And even if we reach the point that this file grows too big, the
> caches can be deleted on Travis CI's web interface.

One more thing:
Maybe we could delete "$HOME/travis-cache/good-trees" if the file
has more than 1000 lines *before* we add a new tree?

Or we use something like this to cap the file:

  echo "$(tail -1000 $HOME/travis-cache/good-trees)" > 
$HOME/travis-cache/good-trees


I agree that the "always growing problem" is not a big one
but an approach like the one above would avoid any discussion
for sure.


Thanks,
Lars

> 
> Note: this won't kick in if two identical trees are on two different
> branches, because Travis CI caches are not shared between build jobs
> of different branches.
> 
> Signed-off-by: SZEDER Gábor 
> ---
> ci/lib-travisci.sh| 43 +++
> ci/run-linux32-docker.sh  |  2 ++
> ci/run-static-analysis.sh |  2 ++
> ci/run-tests.sh   |  2 ++
> ci/run-windows-build.sh   |  2 ++
> ci/test-documentation.sh  |  2 ++
> 6 files changed, 53 insertions(+)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 348fe3c3c..05e73123f 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -21,6 +21,48 @@ skip_branch_tip_with_tag () {
>   fi
> }
> 
> +good_trees_file="$HOME/travis-cache/good-trees"
> +
> +# Save some info about the current commit's tree, so we can skip the build
> +# job if we encounter the same tree again and can provide a useful info
> +# message.
> +save_good_tree () {
> + echo "$(git rev-parse $TRAVIS_COMMIT^{tree}) $TRAVIS_COMMIT 
> $TRAVIS_JOB_NUMBER $TRAVIS_JOB_ID" >>"$good_trees_file"
> +}
> +
> +# Skip the build job if the same tree has already been built and tested
> +# successfully before (e.g. because the branch got rebased, changing only
> +# the commit messages).
> +skip_good_tree () {
> + if ! good_tree_info="$(grep "^$(git rev-parse $TRAVIS_COMMIT^{tree}) " 
> "$good_trees_file")"
> + then
> + # haven't seen this tree yet; continue the build job
> + return
> + fi
> +
> + echo "$good_tree_info" | {
> + read tree prev_good_commit prev_good_job_number prev_good_job_id
> +
> + if test "$TRAVIS_JOB_ID" =  "$prev_good_job_id"
> + then
> + cat <<-EOF
> + Skipping build job for commit $TRAVIS_COMMIT.
> + This commit has already been built and tested 
> successfully by this build job.
> + To force a re-build delete the branch's cache and then 
> hit 'Restart job'.
> + 

Re: [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules

[Stefan Beller]

On Fri, Dec 22, 2017 at 11:34 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> It turns out that this buggy test passed due to the buggy implementation,
>> which will soon be corrected.  Let's fix the test first.
>
> Please clarify "buggy test".  Without the original discussion (I am
> imagining there is something that happened on the list recently),
> here is my guess:
>
> We tried to make sure that an ignored file is $distimmed by
> $gostak, but forgot to tell the exclude mechanism that the path
> 'sub1'ignored' we use for the test is actually ignored, so the
> fact that the file was not $distimmed when $gostak did its thing
> meant nothing---mark any path whose name is 'ignored' as ignored
> to really test the condition we want to test.
>
> but I do not exactly know what verb to replace $distim and what noun
> to replace $gostak above ;-)

Just rewrote that commit message without distims and gostaking.

> Also, wouldn't this expose a bug in the implementation if that is
> the case?

Yes it is, which will be fixed. The bug is:

If the submodule HEAD is at the superprojects recorded object
name, we don't bother looking into the submodule at all, neither for
deleting untracked files, nor resetting changed tracked files.

Thanks,

Stefan

Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0

[Jonathan Nieder]

Jonathan Nieder wrote:
> Ævar Arnfjörð Bjarmason wrote:

>> This is similar to Jeff King's jk/drop-ancient-curl series in that
>> we're dropping perl releases that are rarely tested anymore, however
>> unlike those patches git still works on e.g. 5.8.8 (I couldn't build
>> anything older).
[...]
> Reviewed-by: Jonathan Nieder 

One caveat I forgot: please also update the INSTALL file:

 - Git is reasonably self-sufficient, but does depend on a few external
   programs and libraries.  Git can be used without most of them by adding
   the approriate "NO_=YesPlease" to the make command line or
   config.mak file.
[...]
- "Perl" version 5.8 or later is needed to use some of the

Thanks again,
Jonathan

Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> This is similar to Jeff King's jk/drop-ancient-curl series in that
> we're dropping perl releases that are rarely tested anymore, however
> unlike those patches git still works on e.g. 5.8.8 (I couldn't build
> anything older).

FWIW I think this is a good change.  If someone is affected and offers
help with keeping git working well with older perl (e.g. setting up CI
for us) then we can keep supporting it, but it seems more likely to me
that there will just not be anyone affected.

And being able to assume a newer baseline perl version seems
beneficial both from a security point of view and from a developer
experience pov.

Some minor nits:

[...]
>  contrib/diff-highlight/DiffHighlight.pm | 2 +-
>  contrib/examples/git-difftool.perl  | 2 +-
>  contrib/mw-to-git/Git/Mediawiki.pm  | 2 +-

I'm less opinionated about these.  They're already using the same perl
baseline as the rest of git, so I suppose it's good to keep them
consistent, but if any of them gives you trouble then I think you can
just ignore it and let the relevant contrib maintainer take care of
it. :)

[...]
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1,6 +1,6 @@
>  #!/usr/bin/perl
>  
> -use 5.008;
> +use v5.10.0;

As brian carlson mentioned, 'use 5.010' would produce a clearer error
message on ancient perl, which could result in happier users on
ancient platforms or clearer bug reports.

That said, I don't have a strong opinion there.

Reviewed-by: Jonathan Nieder 

Re: [PATCH 2/2] travis-ci: record and skip successfully built trees

[Lars Schneider]

> On 27 Dec 2017, at 17:49, SZEDER Gábor  wrote:
> 
> Travis CI dutifully builds and tests each new branch tip, even if its
> tree has previously been successfully built and tested.  This happens
> often enough in contributors' workflows, when a work-in-progress
> branch is rebased changing e.g. only commit messages or the order or
> number of commits while leaving the resulting code intact, and is then
> pushed to a Travis CI-enabled GitHub fork.
> 
> This is wasting Travis CI's resources and is sometimes scary-annoying
> when the new tip commit with a tree identical to the previous,
> successfully tested one is suddenly reported in red, because one of
> the OSX build jobs happened to exceed the time limit yet again.
> 
> So extend our Travis CI build scripts to skip building commits whose
> trees have previously been successfully built and tested.  Use the
> Travis CI cache feature to keep a record of the object names of trees
> that tested successfully, in a plain and simple flat text file, one
> line per tree object name.  Append the current tree's object name at
> the end of every successful build job to this file, along with a bit
> of additional info about the build job (commit object name, Travis CI
> job number and id).  Check, using a simple grep invocation, in each
> build job whether the current commit's tree is already in there, and
> skip the build if it is.  Include a message in the skipped build job's
> trace log, containing the URL to the build job successfully testing
> that tree for the first time and instructions on how to force a
> re-build.  Catch the case when a build job, which successfully built
> and tested a particular tree for the first time, is restarted and omit
> the URL of the previous build job's trace log, as in this case it's
> the same build job and the trace log has just been overwritten.
> 
> Using an ever-growing flat text file might seem like asking for
> trouble on the long run, but it's perfectly adequate for this purpose.
> Contributors' topic branches are short-lived in general, so this file
> won't grow large enough to cause any issues.  Grepping through several
> tens of thousands such lines is sufficiently fast, so not even
> git/git's forever living integration branches will cause scalability
> issues with the current rate of ~1 push/day for a couple of decades.
> And even if we reach the point that this file grows too big, the
> caches can be deleted on Travis CI's web interface.
> 
> Note: this won't kick in if two identical trees are on two different
> branches, because Travis CI caches are not shared between build jobs
> of different branches.

This is a nice idea. If there are no other objections to this approach 
then I will forward it to the TravisCI folks. Maybe they are willing to 
implement this natively to safe some more cycles beyond */git ;-)


> 
> Signed-off-by: SZEDER Gábor 
> ---
> ci/lib-travisci.sh| 43 +++
> ci/run-linux32-docker.sh  |  2 ++
> ci/run-static-analysis.sh |  2 ++
> ci/run-tests.sh   |  2 ++
> ci/run-windows-build.sh   |  2 ++
> ci/test-documentation.sh  |  2 ++
> 6 files changed, 53 insertions(+)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 348fe3c3c..05e73123f 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -21,6 +21,48 @@ skip_branch_tip_with_tag () {
>   fi
> }
> 
> +good_trees_file="$HOME/travis-cache/good-trees"
> +
> +# Save some info about the current commit's tree, so we can skip the build
> +# job if we encounter the same tree again and can provide a useful info
> +# message.
> +save_good_tree () {
> + echo "$(git rev-parse $TRAVIS_COMMIT^{tree}) $TRAVIS_COMMIT 
> $TRAVIS_JOB_NUMBER $TRAVIS_JOB_ID" >>"$good_trees_file"
> +}
> +
> +# Skip the build job if the same tree has already been built and tested
> +# successfully before (e.g. because the branch got rebased, changing only
> +# the commit messages).
> +skip_good_tree () {
> + if ! good_tree_info="$(grep "^$(git rev-parse $TRAVIS_COMMIT^{tree}) " 
> "$good_trees_file")"
> + then
> + # haven't seen this tree yet; continue the build job
> + return
> + fi
> +
> + echo "$good_tree_info" | {
> + read tree prev_good_commit prev_good_job_number prev_good_job_id
> +
> + if test "$TRAVIS_JOB_ID" =  "$prev_good_job_id"

Under what circumstances would that be true?

Nit: One unintended space after = ?!

> + then
> + cat <<-EOF
> + Skipping build job for commit $TRAVIS_COMMIT.
> + This commit has already been built and tested 
> successfully by this build job.
> + To force a re-build delete the branch's cache and then 
> hit 'Restart job'.
> + EOF
> + else
> + cat <<-EOF
> + Skipping build job for commit 

Re: [PATCH v2 8/9] rebase -i: learn to abbreviate command names

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Dec 4, 2017 at 5:17 AM, Liam Beguin  wrote:
>> +static const char command_to_char(const enum todo_command command)
>> +{
>> +   if (command < TODO_COMMENT && todo_command_info[command].c)
>> +   return todo_command_info[command].c;
>> +   return comment_line_char;
>> +}
>
> CC sequencer.o
> sequencer.c:798:19: error: type qualifiers ignored on function return
> type [-Werror=ignored-qualifiers]
>  static const char command_to_char(const enum todo_command command)
>^
>
> Maybe drop the first const.

Thanks.  This topic has been in 'next' for quite some time and I
wanted to merge it down to 'master' soonish, so I've added the
following before doing so.

-- >8 --
From: Junio C Hamano 
Date: Wed, 27 Dec 2017 11:12:45 -0800
Subject: [PATCH] sequencer.c: drop 'const' from function return type

With -Werror=ignored-qualifiers, a function that claims to return
"const char" gets this error:

CC sequencer.o
sequencer.c:798:19: error: type qualifiers ignored on function return
type [-Werror=ignored-qualifiers]
 static const char command_to_char(const enum todo_command command)
   ^

Reported-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 115085d39c..2a407cbe54 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -795,7 +795,7 @@ static const char *command_to_string(const enum 
todo_command command)
die("Unknown command: %d", command);
 }
 
-static const char command_to_char(const enum todo_command command)
+static char command_to_char(const enum todo_command command)
 {
if (command < TODO_COMMENT && todo_command_info[command].c)
return todo_command_info[command].c;
-- 
2.15.1-597-g62d91a8972

Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> + # Turn foo/bar/baz into foo/bar to create foo/bar as a
> + # directory structure.
> + dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')

Let's not limit this script to GNUism systems.

You want to see if $file has a slash followed by one or more non-slash
letters to the end and strip the whole thing.  We further know that
at this point $file does not end with a slash.  So perhaps

dirs=${file%/*}

is sufficient?

Thanks.

Re: [PATCH] diffcore: add a filter to find a specific blob

[Jonathan Nieder]

Stefan Beller wrote:
> On Thu, Dec 14, 2017 at 6:18 PM, Junio C Hamano  wrote:

>> I think it would make it a better companion to --pickaxe but we need
>> to align its behaviour a little bit so that it plays better with the
>> "--pickaxe-all" option, and also needs to hide mode and name only
>> changes just like pickaxe.
>
> I looked into this, and the small changes needed led me to thinking
> it could be integrated into the diffcore-pickaxe code completely,
> roughly like (spaces mangled):

Nice, this looks promising.

[...]
> But then, it seems as if any pickaxe option is incompatible with
> any other, i.e. from reading the code, you cannot combine -S
> and -G, or even give one of them twice.
>
> I guess that would be not a big deal for the --pickaxe-object,
> but just want to point it out.

Agreed that that's not a big deal for --pickaxe-object.

>> After all, the diffcore-blobfind code was written while looking at
>> the diffcore-pickaxe's code in another window shown in the pager,
>> and I tend to agree with your earlier message that this is an
>> extreme case of -S where the contents happens to be the
>> whole file.
>
> I disagree, as the user doesn't have the content, but the hash
> over the content only and wants to know more about it. The new
> option cannot be used to find a file whose partial content hashes to
> the given sha1, either.
>
> So with these considerations, I would keep the patch as currently\
> queued at sb/diff-blobfind.

Interesting --- I come to the opposite conclusion.

The pickaxe-style behavior seems more consistent and simpler to
explain and better matches the use cases I can think of.

Thanks,
Jonathan

Re: [PATCH] setup.c: move statement under condition

[Junio C Hamano]

Johannes Schindelin  writes:

>> I suppose that if the condition is fulfilled then the previously
>> obtained value will not be necessary.
>
> I have to be honest: this commit message (including the subject) left me
> quite puzzled as to the intent of this patch.
>
> Maybe something like this would have spared me that puzzlement:
>
>   Avoid unnecessary offset_1st_component() when prefixing filenames
>
>   In the abspath_part_inside_repo() function that is called
>   somewhere deep in the call-chain when prefixing paths, we
>   calculate the offset of the first component, but under certain
>   circumstances, the result is not even used.
>
>   This patch changes the code to avoid that.

Sensible.

>> diff --git a/setup.c b/setup.c
>> index 8cc34186c..1ce0189fa 100644
>> --- a/setup.c
>> +++ b/setup.c
>> @@ -35,7 +35,6 @@ static int abspath_part_inside_repo(char *path)
>>  return -1;
>>  wtlen = strlen(work_tree);
>>  len = strlen(path);
>> -off = offset_1st_component(path);
>>  
>>  /* check if work tree is already the prefix */
>>  if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
>> @@ -49,6 +48,8 @@ static int abspath_part_inside_repo(char *path)
>>  }
>>  /* work tree might match beginning of a symlink to work tree */
>>  off = wtlen;
>> +} else {
>> +off = offset_1st_component(path);
>>  }
>
> Up until recently, we encouraged dropping the curly brackets from
> single-line statements, but apparently that changed. 

That is not quite correct.  We do encourage 

if (...)
single statement;

with or without

else
another single statement;

IOW, when both sides do not need curlies, we save vertical space.
On the other hand, when one side needs curlies, we tend to add to
both to make it easier to spot the correspondence, i.e.

if (...) {
compond statement;
compond statement;
} else {
single statement;
}

or the other way around.


if (...) {
single statement;
} else {
compond statement;
compond statement;
}

> However, we still encourage to put shorter alternative code paths
> (i.e. the blocks after `if` and `else`) first, in your case:
>
> @@ -35,18 +35,19 @@ static int abspath_part_inside_repo(char *path)
>   return -1;
>   wtlen = strlen(work_tree);
>   len = strlen(path);
> - off = offset_1st_component(path);
>  
>   /* check if work tree is already the prefix */
> - if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
> + if (wtlen > len || strncmp(path, work_tree, wtlen))
> + off = offset_1st_component(path);
> + else {
>   if (path[wtlen] == '/') {
>   memmove(path, path + wtlen + 1, len - wtlen);
>   return 0;
>   } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
>   /* work tree is the root, or the whole path */
>   memmove(path, path + wtlen, len - wtlen + 1);
>   return 0;
>   }
>   /* work tree might match beginning of a symlink to work * tree 
> */
>   off = wtlen;
>   }

This also may allow you to further dedent the if/else chain and make
the result easier to follow.  I dunno.

Thanks for a review.

Re: [PATCH v3 4/4] travis-ci: only print test failures if there are test results available

[Lars Schneider]

> On 27 Dec 2017, at 17:36, SZEDER Gábor  wrote:
> 
> When a build job running the test suite fails, our
> 'ci/print-test-failures.sh' script scans all 't/test-results/*.exit'
> files to find failed tests and prints their verbose output.  However,
> if a build job were to fail before it ever gets to run the test suite,
> then there will be no files to match the above pattern and the shell
> will take the pattern literally, resulting in errors like this in the
> trace log:
> 
>  cat: t/test-results/*.exit: No such file or directory
>  
>  t/test-results/*.out...
>  
>  cat: t/test-results/*.out: No such file or directory
> 
> Check upfront and proceed only if there are any such files present.
> 
> Signed-off-by: SZEDER Gábor 
> ---
> ci/print-test-failures.sh | 6 ++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
> index 97cc05901..4f261ddc0 100755
> --- a/ci/print-test-failures.sh
> +++ b/ci/print-test-failures.sh
> @@ -8,6 +8,12 @@
> # Tracing executed commands would produce too much noise in the loop below.
> set +x
> 
> +if ! ls t/test-results/*.exit >/dev/null 2>/dev/null
> +then
> + echo "Build job failed before the tests could have been run"
> + exit
> +fi
> +

Look good to me!

Minor nit: I am not 100% sure about the grammar but I am no native speaker
and can't really tell.


Thanks,
Lars



> for TEST_EXIT in t/test-results/*.exit
> do
>   if [ "$(cat "$TEST_EXIT")" != "0" ]
> -- 
> 2.15.1.500.g54ea76cc4
> 

Re: [RFC/PATCH] perl: bump the required Perl version to 5.10.0 from 5.8.0

[Junio C Hamano]

Eric Wong  writes:

> Fair enough, I haven't run 5.8 in a while, either.  One concern
> I have is it makes reviewing more difficult as the language gets
> bigger and (even more) unfamiliar constructs pop up.  This is
> probably more important for git as most of us are not dedicated
> Perl hackers.

Yes, but that cuts both ways.  When we do get a less amateur Perl
folks who contribute and review, they are more and more likely to be
fluent in modern versions of Perl than the ancient ones some of us
who learned the tongue long time ago are familiar with.  Telling
them to limit themselves to what is also available in ancient
versions is a burden that is backwards.

So

> What mostly bugs me about this is going from:
>
>   "we'll accept patches to keep your old system working"
>
>  to:
>
>   "your software is too old, upgrade or go away"

is something we need to be able to say at some point.

Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 27 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> +status_is_clean() {
>> +>../status.expect &&
>> +git status --porcelain >../status.actual &&
>> +test_cmp ../status.expect ../status.actual
>> +}
>> +
>>  test_lazy_prereq UNTRACKED_CACHE '
>>  { git update-index --test-untracked-cache; ret=$?; } &&
>>  test $ret -ne 1
>> @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' 
>> '
>>  test_cmp ../before ../after
>>  '
>>
>> +test_expect_success 'teardown worktree' '
>> +cd ..
>> +'
>
> Funny indentation.

Thanks. I'll submit a new series with all 3 patches with the issues you
brought up mentioned...

>> +test_expect_success 'setup worktree for symlink test' '
>> +git init worktree-symlink &&
>> +cd worktree-symlink &&
>> +git config core.untrackedCache true &&
>> +mkdir one two &&
>> +touch one/file two/file &&
>> +git add one/file two/file &&
>> +git commit -m"first commit" &&
>> +git rm -rf one &&
>> +ln -s two one &&
>> +git add one &&
>> +git commit -m"second commit"
>> +'
>
> This needs SYMLINKS prereq, which in turn means it cannot be tested
> on certain platforms.  I thought Duy's answer was that this does not
> need to involve a symbolic link at all?  If so, perhaps we can have
> another test that does not need symlink?

... as soon as I figure out how to add such a non-symlink test as well
(seems sensible to test both), but I can't bring it to fail without
symlinks, I just adjusted my test script to do this instead:

(
rm -rf /tmp/testrepo &&
git init /tmp/testrepo &&
cd /tmp/testrepo &&
mkdir x y &&
touch x/a y/b &&
git add x/a y/b &&
git commit -msnap &&
git rm -rf y &&
mkdir y &&
touch y/c &&
git add y &&
git commit -msnap2 &&
git checkout HEAD~ &&
git status &&
git checkout master &&
sleep 1 &&
git status &&
git status
)

Duy, what am I missing here?

Re: [PATCH] diffcore: add a filter to find a specific blob

[Stefan Beller]

On Thu, Dec 14, 2017 at 6:18 PM, Junio C Hamano  wrote:
> Jonathan Nieder  writes:
>
>>> Regarding finding a better name, I would want to hear from others,
>>> I am happy with --find-object, though I can see --pickaxe-object
>>> or --object--filter to be a good narrative as well.
>>
>> Drat, I was hoping for an opinion.
>
> I think it would make it a better companion to --pickaxe but we need
> to align its behaviour a little bit so that it plays better with the
> "--pickaxe-all" option, and also needs to hide mode and name only
> changes just like pickaxe.

I looked into this, and the small changes needed led me to thinking
it could be integrated into the diffcore-pickaxe code completely,
roughly like (spaces mangled):

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 9476bd2108..46f875a7b4 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -124,13 +124,21 @@ static int pickaxe_match(struct diff_filepair
*p, struct diff_options *o,
mmfile_t mf1, mf2;
int ret;

-   if (!o->pickaxe[0])
-   return 0;
-
/* ignore unmerged */
if (!DIFF_FILE_VALID(p->one) && !DIFF_FILE_VALID(p->two))
return 0;

+   if (options->objfind) {
+   if ((DIFF_FILE_VALID(p->one) &&
+oidset_contains(options->objfind, >one->oid)) ||
+   (DIFF_FILE_VALID(p->two) &&
+oidset_contains(options->objfind, >two->oid)))
+   return 1;
+   }
+
+   if (!o->pickaxe[0])
+   return 0;
+
if (o->flags.allow_textconv) {
textconv_one = get_textconv(p->one);
textconv_two = get_textconv(p->two);
---8<---

But then, it seems as if any pickaxe option is incompatible with
any other, i.e. from reading the code, you cannot combine -S
and -G, or even give one of them twice.

I guess that would be not a big deal for the --pickaxe-object,
but just want to point it out.

> After all, the diffcore-blobfind code was written while looking at
> the diffcore-pickaxe's code in another window shown in the pager,
> and I tend to agree with your earlier message that this is an
> extreme case of -S where the contents happens to be the
> whole file.

I disagree, as the user doesn't have the content, but the hash
over the content only and wants to know more about it. The new
option cannot be used to find a file whose partial content hashes to
the given sha1, either.

So with these considerations, I would keep the patch as currently\
queued at sb/diff-blobfind.

Thanks,
Stefan

Re: [PATCH v3 3/4] travis-ci: save prove state for the 32 bit Linux build

[Lars Schneider]

> On 27 Dec 2017, at 17:36, SZEDER Gábor  wrote:
> 
> This change follows suit of 6272ed319 (travis-ci: run previously
> failed tests first, then slowest to fastest, 2016-01-26), which did
> this for the Linux and OSX build jobs.  Travis CI build jobs run the
> tests parallel, which is sligtly faster when tests are run in slowest
> to fastest order, shortening the overall runtime of this build job by
> about a minute / 10%.
> 
> Note, that the 32 bit Linux build job runs the tests suite in a Docker
> container and we have to share the Travis CI cache directory with the
> container as a second volume.  Otherwise we couldn't use a symlink
> pointing to the prove state file in the cache directory, because
> that's outside of the directory hierarchy accessible from within the
> container.
> 
> Signed-off-by: SZEDER Gábor 
> ---
> ci/run-linux32-build.sh  | 1 +
> ci/run-linux32-docker.sh | 1 +
> 2 files changed, 2 insertions(+)
> 
> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> index a8518eddf..c19c50c1c 100755
> --- a/ci/run-linux32-build.sh
> +++ b/ci/run-linux32-build.sh
> @@ -27,6 +27,7 @@ test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID 
> $CI_USER) &&
> # Build and test
> linux32 --32bit i386 su -m -l $CI_USER -c '
> cd /usr/src/git &&
> +ln -s /tmp/travis-cache/.prove t/.prove &&
> make --jobs=2 &&
> make --quiet test
> '
> diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
> index 0edf63acf..3a8b2ba42 100755
> --- a/ci/run-linux32-docker.sh
> +++ b/ci/run-linux32-docker.sh
> @@ -19,5 +19,6 @@ docker run \
>   --env GIT_TEST_OPTS \
>   --env GIT_TEST_CLONE_2GB \
>   --volume "${PWD}:/usr/src/git" \
> + --volume "${HOME}/travis-cache:/tmp/travis-cache" \

I assume "${HOME}/travis-cache:/usr/src/git/t/.prove" would not
work because that would be a mapping in another mapping?

Thanks,
Lars

>   daald/ubuntu32:xenial \
>   /usr/src/git/ci/run-linux32-build.sh $(id -u $USER)
> -- 
> 2.15.1.500.g54ea76cc4
> 

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Sat, Dec 23 2017, Wei Shuyu jotted:
>
>> Git has been taught to support an https:// used for http.proxy when
>> using recent versions of libcurl.
>>
>> To use https proxy with self-signed certs, we need a way to
>> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST
>> just like direct SSL connection. This is required if we want
>> use t/lib-httpd to test proxy.
>>
>> In this patch I reuse http.sslverify to do this, do we need an
>> independent option like http.sslproxyverify?
>>
>> To fully support https proxy, we also need ways to set more options
>> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to
>> support them.
>
> It would be good to add a link to
> https://daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl/ to the
> commit message, since it explains in great detail what this is for and
> how it compares to what we were doing before.

Yeah, I found it a quite readable explanation.

It appears to me that the configuration for the proxy would have to
be different from the configuration for the hosts in the outside
world.  You may want to be a lot more strongly suspicious to the
latter while trusting the former a bit more.  I also suspect that
we'd need options like PROXY_SSLCERT that are parallel to the main
SSL support in the final form, but it may be OK to leave it to a
follow-up series.

And in order to start small and grow later, I think it is not a good
idea to repurpose http.sslverify to imply http.sslproxyverify; you
are declaring "when you disable peer verification, you also disable
proxy verification", and need to worry about backward compatibility
when you later introduce http.sslproxyverify that can control these
independently.  It probably is less bad than usual b/c issues in
that it probably is much more common to disable verification for
proxy than target hosts, though.

In any case, thanks for working on this.

>
>> Signed-off-by: Wei Shuyu 
>> ---
>>  http.c | 9 +
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/http.c b/http.c
>> index 215bebef1..d8a5e48f0 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -708,6 +708,10 @@ static CURL *get_curl_handle(void)
>>  if (!curl_ssl_verify) {
>>  curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 0);
>>  curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 0);
>> +#if LIBCURL_VERSION_NUM >= 0x073400
>> +curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYPEER, 0);
>> +curl_easy_setopt(result, CURLOPT_PROXY_SSL_VERIFYHOST, 0);
>> +#endif
>>  } else {
>>  /* Verify authenticity of the peer's certificate */
>>  curl_easy_setopt(result, CURLOPT_SSL_VERIFYPEER, 1);
>> @@ -865,6 +869,11 @@ static CURL *get_curl_handle(void)
>>  else if (starts_with(curl_http_proxy, "socks"))
>>  curl_easy_setopt(result,
>>  CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
>> +#endif
>> +#if LIBCURL_VERSION_NUM >= 0x073400
>> +else if (starts_with(curl_http_proxy, "https"))
>> +curl_easy_setopt(result,
>> +CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
>>  #endif
>>  if (strstr(curl_http_proxy, "://"))
>>  credential_from_url(_auth, curl_http_proxy);

Re: [PATCH v3 2/4] travis-ci: don't install default addon packages for the 32 bit Linux build

[Lars Schneider]

> On 27 Dec 2017, at 17:36, SZEDER Gábor  wrote:
> 
> The 32 bit Linux build job compiles Git and runs the test suite in a
> Docker container, while the additional packages (apache2, git-svn,
> language-pack-is) are installed on the host, therefore don't have
> any effect and are unnecessary.

Nice optimization - ACK!

Thanks,
Lars



> 
> Signed-off-by: SZEDER Gábor 
> ---
> .travis.yml | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 7c9aa0557..4684b3f4f 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -42,6 +42,7 @@ matrix:
> - env: jobname=Linux32
>   os: linux
>   compiler:
> +  addons:
>   services:
> - docker
>   before_install:
> -- 
> 2.15.1.500.g54ea76cc4
> 

Re: [PATCH v3 1/4] travis-ci: fine tune the use of 'set -x' in 'ci/*' scripts

[Lars Schneider]

> On 27 Dec 2017, at 17:36, SZEDER Gábor  wrote:
> 
> The change in commit 4f2636667 (travis-ci: use 'set -x' in 'ci/*'
> scripts for extra tracing output, 2017-12-12) left a couple of rough
> edges:
> 
>  - 'ci/run-linux32-build.sh' is executed in a Docker container and
>therefore doesn't source 'ci/lib-travisci.sh', which would enable
>tracing executed commands.  Enable 'set -x' in this script, too.
> 
>  - 'ci/print-test-failures.sh' iterates over all the files containing
>the exit codes of all the execued test scripts.  Since there are
s/execued/executed/

>over 800 such files, the loop produces way too much noise with
>tracing executed commands enabled, so disable 'set -x' for this
>script.
> 
>  - 'ci/run-windows-build.sh' busily waits in a loop for the result of
>the Windows build, producing too much noise with tracing executed
>commands enabled as well.  Disable 'set -x' for the duration of
>that loop.
> 
> igned-off-by: SZEDER Gábor 
> ---
> ci/lib-travisci.sh| 4 +++-
> ci/print-test-failures.sh | 3 +++
> ci/run-linux32-build.sh   | 2 ++
> ci/run-windows-build.sh   | 5 +
> 4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 331d3eb3a..348fe3c3c 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -22,7 +22,9 @@ skip_branch_tip_with_tag () {
> }
> 
> # Set 'exit on error' for all CI scripts to let the caller know that
> -# something went wrong
> +# something went wrong.
> +# Set tracing executed commands, primarily setting environment variables
> +# and installing dependencies.

Maybe:

  # something went wrong. Enable tracing to ease debugging on TravisCI.

I would move the "primarily setting environment ..." either to the
top of the file or to the respective section below.


But this is a minor nit. The rest of the patch looks very good.

Thanks,
Lars
 

> set -ex
> 
> skip_branch_tip_with_tag
> diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
> index 8c8973cbf..97cc05901 100755
> --- a/ci/print-test-failures.sh
> +++ b/ci/print-test-failures.sh
> @@ -5,6 +5,9 @@
> 
> . ${0%/*}/lib-travisci.sh
> 
> +# Tracing executed commands would produce too much noise in the loop below.
> +set +x
> +
> for TEST_EXIT in t/test-results/*.exit
> do
>   if [ "$(cat "$TEST_EXIT")" != "0" ]
> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> index e30fb2cdd..a8518eddf 100755
> --- a/ci/run-linux32-build.sh
> +++ b/ci/run-linux32-build.sh
> @@ -6,6 +6,8 @@
> #   run-linux32-build.sh [host-user-id]
> #
> 
> +set -x
> +
> # Update packages to the latest available versions
> linux32 --32bit i386 sh -c '
> apt update >/dev/null &&
> diff --git a/ci/run-windows-build.sh b/ci/run-windows-build.sh
> index 8757b3a97..86999268a 100755
> --- a/ci/run-windows-build.sh
> +++ b/ci/run-windows-build.sh
> @@ -69,6 +69,10 @@ esac
> 
> echo "Visual Studio Team Services Build #${BUILD_ID}"
> 
> +# Tracing execued commands would produce too much noise in the waiting
> +# loop below.
> +set +x
> +
> # Wait until build job finished
> STATUS=
> RESULT=
> @@ -90,6 +94,7 @@ done
> # Print log
> echo ""
> echo ""
> +set -x
> gfwci "action=log=$BUILD_ID" | cut -c 30-
> 
> # Set exit code for TravisCI
> -- 
> 2.15.1.500.g54ea76cc4
> 

Re: [RFC PATCH v2] http: support CURLPROXY_HTTPS

[Jonathan Nieder]

Wei Shuyu wrote:

> Git has been taught to support an https:// used for http.proxy when
> using recent versions of libcurl.

nit: commit messages use the imperative mood, as though commanding the
code base to do something:

Support https:// for http.proxy when using recent versions of
curl.

[...]
> To use https proxy with self-signed certs, we need a way to
> unset CURLOPT_PROXY_SSL_VERIFYPEER and CURLOPT_PROXY_SSL_VERIFYHOST
> just like direct SSL connection. This is required if we want
> use t/lib-httpd to test proxy.

Interesting.  Sounds promising.

> In this patch I reuse http.sslverify to do this, do we need an
> independent option like http.sslproxyverify?

I think a separate option is a good idea.  This way, I can use

[http "https://example.com;]
sslVerify = false

to fetch from a misconfigured server or

[http "https://example.com;]
proxy = https://127.0.0.1
sslVerifyProxy = false

to proxy through a misconfigured proxy.  Alternatively, is there a
straightforward way to make

[http "https://example.com;]
proxy = https://127.0.0.1
[http "https://127.0.0.1;]
sslVerify = false

do that?  Something like

struct urlmatch_config proxy_config = { STRING_LIST_INIT_DUP };
config.section = "http";
config.key = NULL;
config.collect_fn = proxy_options;
config.cascade_fn = git_default_config;
config.cb = NULL;
config.url = normalized_proxy_url;
git_config(urlmatch_config_entry, );

How does this interact with the GIT_SSL_NO_VERIFY environment
variable?

> To fully support https proxy, we also need ways to set more options
> such as CURLOPT_PROXY_SSLCERT. However, I'm not sure if we need to
> support them.

That's for client certs, right?  Would it work to make that
configurable as

[http "https://127.0.0.1;]
sslCert = ...

?

That said, I agree with you that it's not a prerequisite for this
initial patch.

Thanks,
Jonathan

Re: [RFC] Prepend "tags/" when describing tags with embedded name

[Junio C Hamano]

Daniel Knittl-Frank  writes:

> On Fri, Dec 15, 2017 at 8:25 PM, Junio C Hamano  wrote:
>> I think the code makes sense, but it won't be understandable by
>> those who do not know what you discussed in the original thread.
>>
>> A proper commit log message, with a new test or two in t6120, would
>> be an appropriate way to fix that.
>>
>> Care to follow through, along the lines in
>> Documentation/SubmittingPatches?
>
> The updated branch including tests can be found at:
> http://repo.or.cz/git/dkf.git/shortlog/refs/heads/bugfix/describe-all
>
> One existing test in t6210 needed updating to match the new behavior,
> three new tests have been added: annotated tags, lightweight tags, and
> branches.
>
> Daniel

Thanks.  Looks excellent.  

The second paragraph, while it did not say anything incorrect, felt
a bit too indirect to point out that the commit it refers introduced
the regression this is fixing, so I updated the description a bit,
like below.

-- >8 --
From: Daniel Knittl-Frank 
Date: Mon, 11 Dec 2017 18:24:54 +0100
Subject: [PATCH] describe: prepend "tags/" when describing tags with embedded 
name

The man page of the "git describe" command explains the expected
output when using the --all option, i.e. the full reference path is
shown, including heads/ or tags/ prefix.

When 212945d4a85dfa172ea55ec73b1d830ef2d8582f ("Teach git-describe
to verify annotated tag names before output") made Git favor the
embedded name of annotated tags, it accidentally changed the output
format when the --all flag is given, only printing the tag's name
without the prefix.

Check if --all was specified and re-add the "tags/" prefix for this
special case to fix the regresssion.

Signed-off-by: Daniel Knittl-Frank 
Signed-off-by: Junio C Hamano 
---
 builtin/describe.c  | 7 +--
 t/t6120-describe.sh | 6 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 29075dbd0f..2004a1a86e 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -271,10 +271,13 @@ static void display_name(struct commit_name *n)
n->name_checked = 1;
}
 
-   if (n->tag)
+   if (n->tag) {
+   if (all)
+   printf("tags/");
printf("%s", n->tag->tag);
-   else
+   } else {
printf("%s", n->path);
+   }
 }
 
 static void show_suffix(int depth, const struct object_id *oid)
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 1c0e8659d9..15612b3bbe 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -122,7 +122,7 @@ test_expect_success 'describe --contains defaults to HEAD 
without commit-ish' '
 '
 
 : >err.expect
-check_describe A --all A^0
+check_describe tags/A --all A^0
 test_expect_success 'no warning was displayed for A' '
test_cmp err.expect err.actual
 '
@@ -340,4 +340,8 @@ test_expect_success ULIMIT_STACK_SIZE 'describe works in a 
deep repo' '
test_cmp expect actual
 '
 
+check_describe tags/A --all A
+check_describe tags/c --all c
+check_describe heads/branch_A --all --match='branch_*' branch_A
+
 test_done
-- 
2.15.1-597-g62d91a8972

Re: [PATCH] status: handle worktree renames

[Junio C Hamano]

Duy Nguyen  writes:

>> The problem is, you cannot know if it's a rename from HEAD or from
>> worktree with this updated v2 (or perhaps you could because HEAD name
>> should be all zero?).
>
> I'm wrong about this. the "" code for HEAD rename would be "R."
> while worktree rename is ".R" so I think we're good.

Ah, OK, then.  Thanks.

Re: [PATCH] status: handle worktree renames

[Junio C Hamano]

Duy Nguyen  writes:

> Or we disable rename-from-worktree when porcelain v2 is requested (and
> optionally introduce v3 to support it). Jeff, any preference?

I actually think disabling rename-from-worktree consistently may be
the best way to go.

Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> +status_is_clean() {
> + >../status.expect &&
> + git status --porcelain >../status.actual &&
> + test_cmp ../status.expect ../status.actual
> +}
> +
>  test_lazy_prereq UNTRACKED_CACHE '
>   { git update-index --test-untracked-cache; ret=$?; } &&
>   test $ret -ne 1
> @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' '
>   test_cmp ../before ../after
>  '
>  
> +test_expect_success 'teardown worktree' '
> +cd ..
> +'

Funny indentation.

> +test_expect_success 'setup worktree for symlink test' '
> + git init worktree-symlink &&
> + cd worktree-symlink &&
> + git config core.untrackedCache true &&
> + mkdir one two &&
> + touch one/file two/file &&
> + git add one/file two/file &&
> + git commit -m"first commit" &&
> + git rm -rf one &&
> + ln -s two one &&
> + git add one &&
> + git commit -m"second commit"
> +'

This needs SYMLINKS prereq, which in turn means it cannot be tested
on certain platforms.  I thought Duy's answer was that this does not
need to involve a symbolic link at all?  If so, perhaps we can have
another test that does not need symlink?

Thanks.

Re: [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Document the bug tested for in my "status: add a failing test showing
> a core.untrackedCache bug" and fixed in Duy's "dir.c: fix missing dir
> invalidation in untracked code".
>
> Since this is very likely something others will encounter in the
> future on older versions, and it's not obvious how to fix it let's
> document both that it exists, and how to "fix" it with a one-off
> command.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>
>> On Tue, Dec 26, 2017 at 05:47:19PM +0700, Duy Nguyen wrote:
>>> Strangely, root dir is not cached (no valid flag). I don't know why
>>> but that's ok we'll roll with it.
>>
>> I figured this out. Which is good because now I know how/why the bug
>> happens.
>
> Thanks a lot. I think it's probably good to apply something like this
> on top of this now 3-patch series.

Thanks both for working well together.  Now that the problem turns
out not about symbolic links, can we update the test in 1/1 not to
depend on symbolic links?

>
>  Documentation/git-update-index.txt | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/git-update-index.txt 
> b/Documentation/git-update-index.txt
> index bdb0342593..bc6c32002f 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -464,6 +464,17 @@ command reads the index; while when 
> `--[no-|force-]untracked-cache`
>  are used, the untracked cache is immediately added to or removed from
>  the index.
>  
> +Before 2.16, the untracked cache had a bug where replacing a directory
> +with a symlink to another directory could cause it to incorrectly show
> +files tracked by git as untracked. See the "status: add a failing test
> +showing a core.untrackedCache bug" commit to git.git. A workaround for
> +that was (and this might work for other undiscoverd bugs in the
> +future):
> +
> +
> +$ git -c core.untrackedCache=false status
> +
> +
>  File System Monitor
>  ---

Re: [PATCH] oidmap: ensure map is initialized

[Junio C Hamano]

Brandon Williams  writes:

> Ensure that an oidmap is initialized before attempting to add, remove,
> or retrieve an entry by simply performing the initialization step
> before accessing the underlying hashmap.
>
> Signed-off-by: Brandon Williams 
> ---

Looks sane.  Thanks for illustrating the idea with actual code.

Essentially, you are using map->map.cmpfn as a boolean to see "have
we initialized this thing?  if not, we need to initialize it on
demand".  

By the way, I am somewhat more sympathetic than usual to Dscho's
"make oidmap_get() very aware of the internal implementation detail
of hashmap_get_from_hash() to micro-optimize by removing the check
from _get()".  Such a layering violation is disgusting, and making
it deliberately shows an even worse design taste, but in this
particular case, because the oidmap API is too thin a layer on top
of hashmap, it is understandably a very tempting approach.

>  oidmap.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/oidmap.c b/oidmap.c
> index 6db4fffcd..d9fb19ba6 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)
>  
>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)
>  {
> + if (!map->map.cmpfn)
> + return NULL;
> +
>   return hashmap_get_from_hash(>map, hash(key), key);
>  }
>  
>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  {
>   struct hashmap_entry entry;
> +
> + if (!map->map.cmpfn)
> + oidmap_init(map, 0);
> +
>   hashmap_entry_init(, hash(key));
>   return hashmap_remove(>map, , key);
>  }
> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct 
> object_id *key)
>  void *oidmap_put(struct oidmap *map, void *entry)
>  {
>   struct oidmap_entry *to_put = entry;
> +
> + if (!map->map.cmpfn)
> + oidmap_init(map, 0);
> +
>   hashmap_entry_init(_put->internal_entry, hash(_put->oid));
>   return hashmap_put(>map, to_put);
>  }

Re: [PATCH] status: add a failing test showing a core.untrackedCache bug

[Eric Sunshine]

On Wed, Dec 27, 2017 at 5:25 AM, Duy Nguyen  wrote:
> Subject: [PATCH] dir.c: fix missing dir invalidation in untracked code
> [...]
> After step 3, we mark the cache valid. Any stale subdir with incorrect
> recurse flagbecomes a real subdir next time we traverse the directory

s/flagbecomes/flag becomes/

> using dirs[] array.
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy 

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Junio C Hamano]

Jeff King  writes:

> I, too, had a funny feeling about calling this "core". But I didn't have
> a better name, as I'm not sure what other place we have for config
> options that cross many command boundaries. "diff" and "status" don't
> seem quite right to me. While you can argue they are subsystems, it
> seems too easy for users to confuse them with the commands of the same
> names.
>
> Maybe there should be a "ui.*" config hierarchy for these kinds of
> cross-command interface options?

I had an impression that ui.* was primarily pretty-printing,
colouring and things of such nature.  I do not think it is such a
bad idea to honor a status.frotz variable that affects how (e.g. to
what degree of detailedness) status on frotz are reported in Git
subcommands other than 'git status' if they report the same sort of
information on 'frotz' that 'git status' makes.

[PATCH 0/2] Travis CI: skip commits with successfully built and tested trees

[SZEDER Gábor]

Travis CI dutifully builds and tests each new branch tip, even if its
tree has previously been successfully built and tested.  This happens
often enough in contributors' workflows, when a work-in-progress
branch is rebased changing e.g. only commit messages or the order or
number of commits while leaving the resulting code intact, and is then
pushed to a Travis CI-enabled GitHub fork.

This is wasting Travis CI's resources and is sometimes scary-annoying
when the new tip commit with a tree identical to the previous,
successfully tested one is suddenly reported in red, because one of
the OSX build jobs happened to exceed the time limit yet again.

These two patches extend our Travis CI build scripts to skip building
commits whose trees have previously been successfully built and tested.


These patches should go on top of the "Rest of the Travis CI fixes" patch
series, just posted at:

  https://public-inbox.org/git/20171227163603.13313-1-szeder@gmail.com/


SZEDER Gábor (2):
  travis-ci: don't try to create the cache directory unnecessarily
  travis-ci: record and skip successfully built trees

 ci/lib-travisci.sh| 43 +++
 ci/run-linux32-docker.sh  |  2 ++
 ci/run-static-analysis.sh |  2 ++
 ci/run-tests.sh   |  3 ++-
 ci/run-windows-build.sh   |  2 ++
 ci/test-documentation.sh  |  2 ++
 6 files changed, 53 insertions(+), 1 deletion(-)

-- 
2.15.1.500.g54ea76cc4

[PATCH 1/2] travis-ci: don't try to create the cache directory unnecessarily

[SZEDER Gábor]

Travis CI creates that directory for us anyway, even when a previous
cache doesn't exist for the current build job.

In itself it doesn't hurt to try, of course, but the following patch
will access the Travis CI cache much earlier in the build process, and
then creating the cache directory this late might cause confusion.

Signed-off-by: SZEDER Gábor 
---
 ci/run-tests.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ci/run-tests.sh b/ci/run-tests.sh
index f0c743de9..ccdfc2b9d 100755
--- a/ci/run-tests.sh
+++ b/ci/run-tests.sh
@@ -5,6 +5,5 @@
 
 . ${0%/*}/lib-travisci.sh
 
-mkdir -p $HOME/travis-cache
 ln -s $HOME/travis-cache/.prove t/.prove
 make --quiet test
-- 
2.15.1.500.g54ea76cc4

[PATCH 2/2] travis-ci: record and skip successfully built trees

[SZEDER Gábor]

Travis CI dutifully builds and tests each new branch tip, even if its
tree has previously been successfully built and tested.  This happens
often enough in contributors' workflows, when a work-in-progress
branch is rebased changing e.g. only commit messages or the order or
number of commits while leaving the resulting code intact, and is then
pushed to a Travis CI-enabled GitHub fork.

This is wasting Travis CI's resources and is sometimes scary-annoying
when the new tip commit with a tree identical to the previous,
successfully tested one is suddenly reported in red, because one of
the OSX build jobs happened to exceed the time limit yet again.

So extend our Travis CI build scripts to skip building commits whose
trees have previously been successfully built and tested.  Use the
Travis CI cache feature to keep a record of the object names of trees
that tested successfully, in a plain and simple flat text file, one
line per tree object name.  Append the current tree's object name at
the end of every successful build job to this file, along with a bit
of additional info about the build job (commit object name, Travis CI
job number and id).  Check, using a simple grep invocation, in each
build job whether the current commit's tree is already in there, and
skip the build if it is.  Include a message in the skipped build job's
trace log, containing the URL to the build job successfully testing
that tree for the first time and instructions on how to force a
re-build.  Catch the case when a build job, which successfully built
and tested a particular tree for the first time, is restarted and omit
the URL of the previous build job's trace log, as in this case it's
the same build job and the trace log has just been overwritten.

Using an ever-growing flat text file might seem like asking for
trouble on the long run, but it's perfectly adequate for this purpose.
Contributors' topic branches are short-lived in general, so this file
won't grow large enough to cause any issues.  Grepping through several
tens of thousands such lines is sufficiently fast, so not even
git/git's forever living integration branches will cause scalability
issues with the current rate of ~1 push/day for a couple of decades.
And even if we reach the point that this file grows too big, the
caches can be deleted on Travis CI's web interface.

Note: this won't kick in if two identical trees are on two different
branches, because Travis CI caches are not shared between build jobs
of different branches.

Signed-off-by: SZEDER Gábor 
---
 ci/lib-travisci.sh| 43 +++
 ci/run-linux32-docker.sh  |  2 ++
 ci/run-static-analysis.sh |  2 ++
 ci/run-tests.sh   |  2 ++
 ci/run-windows-build.sh   |  2 ++
 ci/test-documentation.sh  |  2 ++
 6 files changed, 53 insertions(+)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 348fe3c3c..05e73123f 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -21,6 +21,48 @@ skip_branch_tip_with_tag () {
fi
 }
 
+good_trees_file="$HOME/travis-cache/good-trees"
+
+# Save some info about the current commit's tree, so we can skip the build
+# job if we encounter the same tree again and can provide a useful info
+# message.
+save_good_tree () {
+   echo "$(git rev-parse $TRAVIS_COMMIT^{tree}) $TRAVIS_COMMIT 
$TRAVIS_JOB_NUMBER $TRAVIS_JOB_ID" >>"$good_trees_file"
+}
+
+# Skip the build job if the same tree has already been built and tested
+# successfully before (e.g. because the branch got rebased, changing only
+# the commit messages).
+skip_good_tree () {
+   if ! good_tree_info="$(grep "^$(git rev-parse $TRAVIS_COMMIT^{tree}) " 
"$good_trees_file")"
+   then
+   # haven't seen this tree yet; continue the build job
+   return
+   fi
+
+   echo "$good_tree_info" | {
+   read tree prev_good_commit prev_good_job_number prev_good_job_id
+
+   if test "$TRAVIS_JOB_ID" =  "$prev_good_job_id"
+   then
+   cat <<-EOF
+   Skipping build job for commit $TRAVIS_COMMIT.
+   This commit has already been built and tested 
successfully by this build job.
+   To force a re-build delete the branch's cache and then 
hit 'Restart job'.
+   EOF
+   else
+   cat <<-EOF
+   Skipping build job for commit $TRAVIS_COMMIT.
+   This commit's tree has already been built and tested 
successfully in build job $prev_good_job_number for commit $prev_good_commit.
+   The log of that build job is available at 
https://travis-ci.org/$TRAVIS_REPO_SLUG/jobs/$prev_good_job_id
+   To force a re-build delete the branch's cache and then 
hit 'Restart job'.
+   EOF
+   fi
+   }
+
+   exit 0
+}
+
 # Set 'exit on error' for all CI scripts 

[PATCH v3 3/4] travis-ci: save prove state for the 32 bit Linux build

[SZEDER Gábor]

This change follows suit of 6272ed319 (travis-ci: run previously
failed tests first, then slowest to fastest, 2016-01-26), which did
this for the Linux and OSX build jobs.  Travis CI build jobs run the
tests parallel, which is sligtly faster when tests are run in slowest
to fastest order, shortening the overall runtime of this build job by
about a minute / 10%.

Note, that the 32 bit Linux build job runs the tests suite in a Docker
container and we have to share the Travis CI cache directory with the
container as a second volume.  Otherwise we couldn't use a symlink
pointing to the prove state file in the cache directory, because
that's outside of the directory hierarchy accessible from within the
container.

Signed-off-by: SZEDER Gábor 
---
 ci/run-linux32-build.sh  | 1 +
 ci/run-linux32-docker.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index a8518eddf..c19c50c1c 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -27,6 +27,7 @@ test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID 
$CI_USER) &&
 # Build and test
 linux32 --32bit i386 su -m -l $CI_USER -c '
 cd /usr/src/git &&
+ln -s /tmp/travis-cache/.prove t/.prove &&
 make --jobs=2 &&
 make --quiet test
 '
diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
index 0edf63acf..3a8b2ba42 100755
--- a/ci/run-linux32-docker.sh
+++ b/ci/run-linux32-docker.sh
@@ -19,5 +19,6 @@ docker run \
--env GIT_TEST_OPTS \
--env GIT_TEST_CLONE_2GB \
--volume "${PWD}:/usr/src/git" \
+   --volume "${HOME}/travis-cache:/tmp/travis-cache" \
daald/ubuntu32:xenial \
/usr/src/git/ci/run-linux32-build.sh $(id -u $USER)
-- 
2.15.1.500.g54ea76cc4

[PATCH v3 0/4] Rest of the Travis CI fixes

[SZEDER Gábor]

These are the rest of the fixes that were in the second half of the v2
patch series but not already in 'sg/travis-fixes', which was already
merged to next, perhaps a bit prematurely.

Patch 1/4 fixes some rough edges related to tracing executed commands
according to the discussion.
Patches 2/4, 3/4 are the same as patches 5/8 and 7/8 in v2, while patch
4/4 includes the small update I sent out as v2.1 of patch 8/8.

I dropped v2's patch 6/8 (travis-ci: don't install 'language-pack-is'
package) for now: these locale-related things require more
investigation.

v2: https://public-inbox.org/git/20171216125418.10743-1-szeder@gmail.com/
v1: https://public-inbox.org/git/20171211233446.10596-1-szeder@gmail.com/

SZEDER Gábor (4):
  travis-ci: fine tune the use of 'set -x' in 'ci/*' scripts
  travis-ci: don't install default addon packages for the 32 bit Linux
build
  travis-ci: save prove state for the 32 bit Linux build
  travis-ci: only print test failures if there are test results
available

 .travis.yml   | 1 +
 ci/lib-travisci.sh| 4 +++-
 ci/print-test-failures.sh | 9 +
 ci/run-linux32-build.sh   | 3 +++
 ci/run-linux32-docker.sh  | 1 +
 ci/run-windows-build.sh   | 5 +
 6 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.15.1.500.g54ea76cc4

[PATCH v3 4/4] travis-ci: only print test failures if there are test results available

[SZEDER Gábor]

When a build job running the test suite fails, our
'ci/print-test-failures.sh' script scans all 't/test-results/*.exit'
files to find failed tests and prints their verbose output.  However,
if a build job were to fail before it ever gets to run the test suite,
then there will be no files to match the above pattern and the shell
will take the pattern literally, resulting in errors like this in the
trace log:

  cat: t/test-results/*.exit: No such file or directory
  
  t/test-results/*.out...
  
  cat: t/test-results/*.out: No such file or directory

Check upfront and proceed only if there are any such files present.

Signed-off-by: SZEDER Gábor 
---
 ci/print-test-failures.sh | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 97cc05901..4f261ddc0 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -8,6 +8,12 @@
 # Tracing executed commands would produce too much noise in the loop below.
 set +x
 
+if ! ls t/test-results/*.exit >/dev/null 2>/dev/null
+then
+   echo "Build job failed before the tests could have been run"
+   exit
+fi
+
 for TEST_EXIT in t/test-results/*.exit
 do
if [ "$(cat "$TEST_EXIT")" != "0" ]
-- 
2.15.1.500.g54ea76cc4

[PATCH v3 1/4] travis-ci: fine tune the use of 'set -x' in 'ci/*' scripts

[SZEDER Gábor]

The change in commit 4f2636667 (travis-ci: use 'set -x' in 'ci/*'
scripts for extra tracing output, 2017-12-12) left a couple of rough
edges:

  - 'ci/run-linux32-build.sh' is executed in a Docker container and
therefore doesn't source 'ci/lib-travisci.sh', which would enable
tracing executed commands.  Enable 'set -x' in this script, too.

  - 'ci/print-test-failures.sh' iterates over all the files containing
the exit codes of all the execued test scripts.  Since there are
over 800 such files, the loop produces way too much noise with
tracing executed commands enabled, so disable 'set -x' for this
script.

  - 'ci/run-windows-build.sh' busily waits in a loop for the result of
the Windows build, producing too much noise with tracing executed
commands enabled as well.  Disable 'set -x' for the duration of
that loop.

igned-off-by: SZEDER Gábor 
---
 ci/lib-travisci.sh| 4 +++-
 ci/print-test-failures.sh | 3 +++
 ci/run-linux32-build.sh   | 2 ++
 ci/run-windows-build.sh   | 5 +
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 331d3eb3a..348fe3c3c 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -22,7 +22,9 @@ skip_branch_tip_with_tag () {
 }
 
 # Set 'exit on error' for all CI scripts to let the caller know that
-# something went wrong
+# something went wrong.
+# Set tracing executed commands, primarily setting environment variables
+# and installing dependencies.
 set -ex
 
 skip_branch_tip_with_tag
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 8c8973cbf..97cc05901 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -5,6 +5,9 @@
 
 . ${0%/*}/lib-travisci.sh
 
+# Tracing executed commands would produce too much noise in the loop below.
+set +x
+
 for TEST_EXIT in t/test-results/*.exit
 do
if [ "$(cat "$TEST_EXIT")" != "0" ]
diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index e30fb2cdd..a8518eddf 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -6,6 +6,8 @@
 #   run-linux32-build.sh [host-user-id]
 #
 
+set -x
+
 # Update packages to the latest available versions
 linux32 --32bit i386 sh -c '
 apt update >/dev/null &&
diff --git a/ci/run-windows-build.sh b/ci/run-windows-build.sh
index 8757b3a97..86999268a 100755
--- a/ci/run-windows-build.sh
+++ b/ci/run-windows-build.sh
@@ -69,6 +69,10 @@ esac
 
 echo "Visual Studio Team Services Build #${BUILD_ID}"
 
+# Tracing execued commands would produce too much noise in the waiting
+# loop below.
+set +x
+
 # Wait until build job finished
 STATUS=
 RESULT=
@@ -90,6 +94,7 @@ done
 # Print log
 echo ""
 echo ""
+set -x
 gfwci "action=log=$BUILD_ID" | cut -c 30-
 
 # Set exit code for TravisCI
-- 
2.15.1.500.g54ea76cc4