Gruß

[Clark Harrison]

Gruß

In einer kurzen Einführung bin ich Barrister Clark Harrison, aus
Portugal, aber jetzt lebe ich in London, ich habe Ihnen eine E-Mail
über Ihre verstorbene Verwandte Familie geschickt, aber ich habe keine
Antwort von Ihnen erhalten, verstorben ist ein Bürger Ihres Landes mit
derselbe Nachname mit dir, er ist ein Exporteur von Gold hier in
London,

Er ist vor ein paar Jahren mit seiner Familie gestorben, hat sein
Unternehmen verlassen und riesige Geldbeträge bei der UBS Investment
Bank in London deponiert.

Ich bin ein persönlicher Barrister für die verstorbene Familie, ich
brauche Ihre Mitarbeit Damit wir das Geld von der Bank bekommen
können, bevor die Regierung es schließlich beschlagnahmt, ist der
Gesamtbetrag in der Bank £ 7,9 Millionen, GBP aber wird erklären, mehr
Details wenn ich von dir höre.

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 02:37:25PM -0800, Junio C Hamano wrote:

> Well, MaintNotes on the 'todo' branch needs a bit of updating, as it
> says something somewhat misleading.
> 
> -- >8 --
> Subject: MaintNotes: clarify the purpose of maint->master upmerge

Yup, this makes sense. Thanks for clarifying.

-Peff

Hey

[Financial Services]

Loan Offer at 3% Lowest Rate Get Now.

Re: [PATCH 1/5] convert_to_git(): checksafe becomes an integer

[Lars Schneider]

> On 06 Jan 2018, at 00:22, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> On 31 Dec 2017, at 09:05, tbo...@web.de wrote:
>>> 
>>> From: Torsten Bögershausen 
>>> 
>>> When calling convert_to_git(), the checksafe parameter has been used to
>>> check if commit would give a non-roundtrip conversion of EOL.
>>> 
>>> When checksafe was introduced, 3 values had been in use:
>>> SAFE_CRLF_FALSE: no warning
>>> SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
>>> SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip
>> 
>> In general, I really like the direction as this simplifies
>> my patch later on in 5/5. However, I see a few problems:
>> ...
> 
> Yes, this looks like a sensible way to go.  I saw Torsten's v3 for
> 1/5 but will end up queuing v2b, as I suspect 5/5 would need to be
> adjusted for the change between the two versions.

I just send out my v3 which integrates the latest version of Torsten's
patch into my series:

https://public-inbox.org/git/20180106004808.77513-1-lars.schnei...@autodesk.com/

Please note: I did not rebase my series. Therefore, there is a small
conflict with 86ff70a0f0 (convert: tighten the safe autocrlf handling, 
2017-11-26)
because has_cr_in_index() was renamed to has_crlf_in_index().

@Junio: What do you prefer in these cases? A rebased series or the conflict?

Thanks,
Lars

[PATCH v3 1/7] strbuf: remove unnecessary NUL assignment in xstrdup_tolower()

[lars . schneider]

From: Lars Schneider 

Since 3733e69464 (use xmallocz to avoid size arithmetic, 2016-02-22) we
allocate the buffer for the lower case string with xmallocz(). This
already ensures a NUL at the end of the allocated buffer.

Remove the unnecessary assignment.

Signed-off-by: Lars Schneider 
---
 strbuf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/strbuf.c b/strbuf.c
index 323c49ceb3..b5d03a5029 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -756,7 +756,6 @@ char *xstrdup_tolower(const char *string)
result = xmallocz(len);
for (i = 0; i < len; i++)
result[i] = tolower(string[i]);
-   result[i] = '\0';
return result;
 }
 
-- 
2.15.1

[PATCH v3 6/7] convert: add support for 'checkout-encoding' attribute

[lars . schneider]

From: Lars Schneider 

Git recognizes files encoded with ASCII or one of its supersets (e.g.
UTF-8 or ISO-8859-1) as text files. All other encodings are usually
interpreted as binary and consequently built-in Git text processing
tools (e.g. 'git diff') as well as most Git web front ends do not
visualize the content.

Add an attribute to teach Git what encoding the user has defined for a
given file. If the content is added to the index, then Git converts the
content to a canonical UTF-8 representation. On checkout Git will
reverse the conversion.

Signed-off-by: Lars Schneider 
---
 Documentation/gitattributes.txt |  60 
 convert.c   | 190 +-
 convert.h   |   1 +
 t/t0028-checkout-encoding.sh| 196 
 4 files changed, 446 insertions(+), 1 deletion(-)
 create mode 100755 t/t0028-checkout-encoding.sh

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 30687de81a..1bc03e69cb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -272,6 +272,66 @@ few exceptions.  Even though...
   catch potential problems early, safety triggers.
 
 
+`checkout-encoding`
+^^^
+
+Git recognizes files encoded with ASCII or one of its supersets (e.g.
+UTF-8 or ISO-8859-1) as text files.  All other encodings are usually
+interpreted as binary and consequently built-in Git text processing
+tools (e.g. 'git diff') as well as most Git web front ends do not
+visualize the content.
+
+In these cases you can teach Git the encoding of a file in the working
+directory with the `checkout-encoding` attribute. If a file with this
+attributes is added to Git, then Git reencodes the content from the
+specified encoding to UTF-8 and stores the result in its internal data
+structure (called "the index"). On checkout the content is encoded
+back to the specified encoding.
+
+Please note that using the `checkout-encoding` attribute may have a
+number of pitfalls:
+
+- Git clients that do not support the `checkout-encoding` attribute
+  will checkout the respective files UTF-8 encoded and not in the
+  expected encoding. Consequently, these files will appear different
+  which typically causes trouble. This is in particular the case for
+  older Git versions and alternative Git implementations such as JGit
+  or libgit2 (as of January 2018).
+
+- Reencoding content to non-UTF encodings (e.g. SHIFT-JIS) can cause
+  errors as the conversion might not be round trip safe.
+
+- Reencoding content requires resources that might slow down certain
+  Git operations (e.g 'git checkout' or 'git add').
+
+Use the `checkout-encoding` attribute only if you cannot store a file in
+UTF-8 encoding and if you want Git to be able to process the content as
+text.
+
+Use the following attributes if your '*.txt' files are UTF-16 encoded
+with byte order mark (BOM) and you want Git to perform automatic line
+ending conversion based on your platform.
+
+
+*.txt  text checkout-encoding=UTF-16
+
+
+Use the following attributes if your '*.txt' files are UTF-16 little
+endian encoded without BOM and you want Git to use Windows line endings
+in the working directory.
+
+
+*.txt  checkout-encoding=UTF-16LE text eol=CRLF
+
+
+You can get a list of all available encodings on your platform with the
+following command:
+
+
+iconv --list
+
+
+
 `ident`
 ^^^
 
diff --git a/convert.c b/convert.c
index f39150cde9..13f766d2a2 100644
--- a/convert.c
+++ b/convert.c
@@ -7,6 +7,7 @@
 #include "sigchain.h"
 #include "pkt-line.h"
 #include "sub-process.h"
+#include "utf8.h"
 
 /*
  * convert.c - convert a file when checking it out and checking it in.
@@ -256,6 +257,147 @@ static int will_convert_lf_to_crlf(size_t len, struct 
text_stat *stats,
 
 }
 
+static struct encoding {
+   const char *name;
+   struct encoding *next;
+} *encoding, **encoding_tail;
+static const char *default_encoding = "UTF-8";
+
+static int encode_to_git(const char *path, const char *src, size_t src_len,
+struct strbuf *buf, struct encoding *enc, int 
conv_flags)
+{
+   char *dst;
+   int dst_len;
+
+   /*
+* No encoding is specified or there is nothing to encode.
+* Tell the caller that the content was not modified.
+*/
+   if (!enc || (src && !src_len))
+   return 0;
+
+   /*
+* Looks like we got called from "would_convert_to_git()".
+* This means Git wants to know if it would encode (= modify!)
+* the content. Let's answer with "yes", since an encoding was
+* specified.
+*/
+   if (!buf && !src)
+   return 1;
+
+   if 

[PATCH v3 2/7] strbuf: add xstrdup_toupper()

[lars . schneider]

From: Lars Schneider 

Create a copy of an existing string and make all characters upper case.
Similar xstrdup_tolower().

This function is used in a subsequent commit.

Signed-off-by: Lars Schneider 
---
 strbuf.c | 12 
 strbuf.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index b5d03a5029..703a1556cb 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -759,6 +759,18 @@ char *xstrdup_tolower(const char *string)
return result;
 }
 
+char *xstrdup_toupper(const char *string)
+{
+   char *result;
+   size_t len, i;
+
+   len = strlen(string);
+   result = xmallocz(len);
+   for (i = 0; i < len; i++)
+   result[i] = toupper(string[i]);
+   return result;
+}
+
 char *xstrvfmt(const char *fmt, va_list ap)
 {
struct strbuf buf = STRBUF_INIT;
diff --git a/strbuf.h b/strbuf.h
index 0a74acb236..2bc148526f 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -616,6 +616,7 @@ __attribute__((format (printf,2,3)))
 extern int fprintf_ln(FILE *fp, const char *fmt, ...);
 
 char *xstrdup_tolower(const char *);
+char *xstrdup_toupper(const char *);
 
 /**
  * Create a newly allocated string using printf format. You can do this easily
-- 
2.15.1

[PATCH v3 7/7] convert: add tracing for checkout-encoding

[lars . schneider]

From: Lars Schneider 

Add the GIT_TRACE_CHECKOUT_ENCODING environment variable to enable
tracing for content that is reencoded with the checkout-encoding
attribute.

Signed-off-by: Lars Schneider 
---
 convert.c| 28 
 t/t0028-checkout-encoding.sh |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/convert.c b/convert.c
index 13f766d2a2..525958bb56 100644
--- a/convert.c
+++ b/convert.c
@@ -257,6 +257,29 @@ static int will_convert_lf_to_crlf(size_t len, struct 
text_stat *stats,
 
 }
 
+static void trace_encoding(const char *context, const char *path,
+  const char *encoding, const char *buf, size_t len)
+{
+   static struct trace_key coe = TRACE_KEY_INIT(CHECKOUT_ENCODING);
+   struct strbuf trace = STRBUF_INIT;
+   int i;
+
+   strbuf_addf(, "%s (%s, considered %s):\n", context, path, 
encoding);
+   for (i = 0; i < len && buf; ++i) {
+   strbuf_addf(
+   ,"| \e[2m%2i:\e[0m %2x \e[2m%c\e[0m%c",
+   i,
+   (unsigned char) buf[i],
+   (buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),
+   ((i+1) % 8 && (i+1) < len ? ' ' : '\n')
+   );
+   }
+   strbuf_addchars(, '\n', 1);
+
+   trace_strbuf(, );
+   strbuf_release();
+}
+
 static struct encoding {
const char *name;
struct encoding *next;
@@ -316,6 +339,7 @@ static int encode_to_git(const char *path, const char *src, 
size_t src_len,
error(error_msg, path, enc->name);
}
 
+   trace_encoding("source", path, enc->name, src, src_len);
dst = reencode_string_len(src, src_len, default_encoding, enc->name,
  _len);
if (!dst) {
@@ -331,6 +355,7 @@ static int encode_to_git(const char *path, const char *src, 
size_t src_len,
else
error(msg, path, enc->name, default_encoding);
}
+   trace_encoding("destination", path, default_encoding, dst, dst_len);
 
/*
 * UTF supports lossless round tripping [1]. UTF to other encoding are
@@ -356,6 +381,9 @@ static int encode_to_git(const char *path, const char *src, 
size_t src_len,
 enc->name, default_encoding,
 _src_len);
 
+   trace_encoding("reencoded source", path, enc->name,
+  re_src, re_src_len);
+
if (!re_src || src_len != re_src_len ||
memcmp(src, re_src, src_len)) {
const char* msg = _("encoding '%s' from %s to %s and "
diff --git a/t/t0028-checkout-encoding.sh b/t/t0028-checkout-encoding.sh
index 3a9951fdf3..5f1c911c07 100755
--- a/t/t0028-checkout-encoding.sh
+++ b/t/t0028-checkout-encoding.sh
@@ -4,6 +4,8 @@ test_description='checkout-encoding conversion via 
gitattributes'
 
 . ./test-lib.sh
 
+GIT_TRACE_CHECKOUT_ENCODING=1 && export GIT_TRACE_CHECKOUT_ENCODING
+
 test_expect_success 'setup test repo' '
git config core.eol lf &&
 
-- 
2.15.1

[PATCH v3 4/7] utf8: add function to detect a missing UTF-16/32 BOM

[lars . schneider]

From: Lars Schneider 

If the endianness is not defined in the encoding name, then let's
be strict and require a BOM to avoid any encoding confusion. The
has_missing_utf_bom() function returns true if a required BOM is
missing.

The Unicode standard instructs to assume big-endian if there in no BOM
for UTF-16/32 [1][2]. However, the W3C/WHATWG encoding standard used
in HTML5 recommends to assume little-endian to "deal with deployed
content" [3]. Strictly requiring a BOM seems to be the safest option
for content in Git.

This function is used in a subsequent commit.

[1] http://unicode.org/faq/utf_bom.html#gen6
[2] http://www.unicode.org/versions/Unicode10.0.0/ch03.pdf
 Section 3.10, D98, page 132
[3] https://encoding.spec.whatwg.org/#utf-16le

Signed-off-by: Lars Schneider 
---
 utf8.c | 13 +
 utf8.h | 16 
 2 files changed, 29 insertions(+)

diff --git a/utf8.c b/utf8.c
index 914881cd1f..f033fec1c2 100644
--- a/utf8.c
+++ b/utf8.c
@@ -562,6 +562,19 @@ int has_prohibited_utf_bom(const char *enc, const char 
*data, size_t len)
);
 }
 
+int has_missing_utf_bom(const char *enc, const char *data, size_t len)
+{
+   return (
+  !strcmp(enc, "UTF-16") &&
+  !(has_bom_prefix(data, len, utf16_be_bom, sizeof(utf16_be_bom)) ||
+has_bom_prefix(data, len, utf16_le_bom, sizeof(utf16_le_bom)))
+   ) || (
+  !strcmp(enc, "UTF-32") &&
+  !(has_bom_prefix(data, len, utf32_be_bom, sizeof(utf32_be_bom)) ||
+has_bom_prefix(data, len, utf32_le_bom, sizeof(utf32_le_bom)))
+   );
+}
+
 /*
  * Returns first character length in bytes for multi-byte `text` according to
  * `encoding`.
diff --git a/utf8.h b/utf8.h
index 4711429af9..26b5e91852 100644
--- a/utf8.h
+++ b/utf8.h
@@ -79,4 +79,20 @@ void strbuf_utf8_align(struct strbuf *buf, align_type 
position, unsigned int wid
  */
 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len);
 
+/*
+ * If the endianness is not defined in the encoding name, then we
+ * require a BOM. The function returns true if a required BOM is missing.
+ *
+ * The Unicode standard instructs to assume big-endian if there
+ * in no BOM for UTF-16/32 [1][2]. However, the W3C/WHATWG
+ * encoding standard used in HTML5 recommends to assume
+ * little-endian to "deal with deployed content" [3].
+ *
+ * [1] http://unicode.org/faq/utf_bom.html#gen6
+ * [2] http://www.unicode.org/versions/Unicode10.0.0/ch03.pdf
+ * Section 3.10, D98, page 132
+ * [3] https://encoding.spec.whatwg.org/#utf-16le
+ */
+int has_missing_utf_bom(const char *enc, const char *data, size_t len);
+
 #endif
-- 
2.15.1

[PATCH v3 3/7] utf8: add function to detect prohibited UTF-16/32 BOM

[lars . schneider]

From: Lars Schneider 

Whenever a data stream is declared to be UTF-16BE, UTF-16LE, UTF-32BE
or UTF-32LE a BOM must not be used [1]. The function returns true if
this is the case.

This function is used in a subsequent commit.

[1] http://unicode.org/faq/utf_bom.html#bom10

Signed-off-by: Lars Schneider 
---
 utf8.c | 24 
 utf8.h |  9 +
 2 files changed, 33 insertions(+)

diff --git a/utf8.c b/utf8.c
index 2c27ce0137..914881cd1f 100644
--- a/utf8.c
+++ b/utf8.c
@@ -538,6 +538,30 @@ char *reencode_string_len(const char *in, int insz,
 }
 #endif
 
+static int has_bom_prefix(const char *data, size_t len,
+ const char *bom, size_t bom_len)
+{
+   return (len >= bom_len) && !memcmp(data, bom, bom_len);
+}
+
+static const char utf16_be_bom[] = {0xFE, 0xFF};
+static const char utf16_le_bom[] = {0xFF, 0xFE};
+static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
+static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+
+int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
+{
+   return (
+ (!strcmp(enc, "UTF-16BE") || !strcmp(enc, "UTF-16LE")) &&
+ (has_bom_prefix(data, len, utf16_be_bom, sizeof(utf16_be_bom)) ||
+  has_bom_prefix(data, len, utf16_le_bom, sizeof(utf16_le_bom)))
+   ) || (
+ (!strcmp(enc, "UTF-32BE") || !strcmp(enc, "UTF-32LE")) &&
+ (has_bom_prefix(data, len, utf32_be_bom, sizeof(utf32_be_bom)) ||
+  has_bom_prefix(data, len, utf32_le_bom, sizeof(utf32_le_bom)))
+   );
+}
+
 /*
  * Returns first character length in bytes for multi-byte `text` according to
  * `encoding`.
diff --git a/utf8.h b/utf8.h
index 6bbcf31a83..4711429af9 100644
--- a/utf8.h
+++ b/utf8.h
@@ -70,4 +70,13 @@ typedef enum {
 void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int 
width,
   const char *s);
 
+/*
+ * Whenever a data stream is declared to be UTF-16BE, UTF-16LE, UTF-32BE
+ * or UTF-32LE a BOM must not be used [1]. The function returns true if
+ * this is the case.
+ *
+ * [1] http://unicode.org/faq/utf_bom.html#bom10
+ */
+int has_prohibited_utf_bom(const char *enc, const char *data, size_t len);
+
 #endif
-- 
2.15.1

[PATCH v3 5/7] convert_to_git(): safe_crlf/checksafe becomes int conv_flags

[lars . schneider]

From: Torsten Bögershausen 

When calling convert_to_git(), the checksafe parameter defined what
should happen if the EOL conversion (CRLF --> LF --> CRLF) does not
roundtrip cleanly. In addition, it also defined if line endings should
be renormalized (CRLF --> LF) or kept as they are.

checksafe was an safe_crlf enum with these values:
SAFE_CRLF_FALSE:   do nothing in case of EOL roundtrip errors
SAFE_CRLF_FAIL:die in case of EOL roundtrip errors
SAFE_CRLF_WARN:print a warning in case of EOL roundtrip errors
SAFE_CRLF_RENORMALIZE: change CRLF to LF
SAFE_CRLF_KEEP_CRLF:   keep all line endings as they are

In some cases the integer value 0 was passed as checksafe parameter
instead of the correct enum value SAFE_CRLF_FALSE. That was no problem
because SAFE_CRLF_FALSE is defined as 0.

FALSE/FAIL/WARN are different from RENORMALIZE and KEEP_CRLF. Therefore,
an enum is not ideal. Let's use a integer bit pattern instead and rename
the parameter to conv_flags to make it more generically usable. This
allows us to extend the bit pattern in a subsequent commit.

Helped-By: Lars Schneider 
Signed-off-by: Torsten Bögershausen 
Signed-off-by: Lars Schneider 
---
 apply.c|  6 +++---
 combine-diff.c |  2 +-
 config.c   |  7 +--
 convert.c  | 38 +++---
 convert.h  | 17 +++--
 diff.c |  8 
 environment.c  |  2 +-
 sha1_file.c| 12 ++--
 8 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/apply.c b/apply.c
index 321a9fa68d..f8b67bfee2 100644
--- a/apply.c
+++ b/apply.c
@@ -2263,8 +2263,8 @@ static void show_stats(struct apply_state *state, struct 
patch *patch)
 static int read_old_data(struct stat *st, struct patch *patch,
 const char *path, struct strbuf *buf)
 {
-   enum safe_crlf safe_crlf = patch->crlf_in_old ?
-   SAFE_CRLF_KEEP_CRLF : SAFE_CRLF_RENORMALIZE;
+   int conv_flags = patch->crlf_in_old ?
+   CONV_EOL_KEEP_CRLF : CONV_EOL_RENORMALIZE;
switch (st->st_mode & S_IFMT) {
case S_IFLNK:
if (strbuf_readlink(buf, path, st->st_size) < 0)
@@ -2281,7 +2281,7 @@ static int read_old_data(struct stat *st, struct patch 
*patch,
 * should never look at the index when explicit crlf option
 * is given.
 */
-   convert_to_git(NULL, path, buf->buf, buf->len, buf, safe_crlf);
+   convert_to_git(NULL, path, buf->buf, buf->len, buf, conv_flags);
return 0;
default:
return -1;
diff --git a/combine-diff.c b/combine-diff.c
index 2505de119a..19f30c3353 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1053,7 +1053,7 @@ static void show_patch_diff(struct combine_diff_path 
*elem, int num_parent,
if (is_file) {
struct strbuf buf = STRBUF_INIT;
 
-   if (convert_to_git(_index, elem->path, 
result, len, , safe_crlf)) {
+   if (convert_to_git(_index, elem->path, 
result, len, , global_conv_flags_eol)) {
free(result);
result = strbuf_detach(, );
result_size = len;
diff --git a/config.c b/config.c
index e617c2018d..1f003fbb90 100644
--- a/config.c
+++ b/config.c
@@ -1149,11 +1149,14 @@ static int git_default_core_config(const char *var, 
const char *value)
}
 
if (!strcmp(var, "core.safecrlf")) {
+   int eol_rndtrp_die;
if (value && !strcasecmp(value, "warn")) {
-   safe_crlf = SAFE_CRLF_WARN;
+   global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
return 0;
}
-   safe_crlf = git_config_bool(var, value);
+   eol_rndtrp_die = git_config_bool(var, value);
+   global_conv_flags_eol = eol_rndtrp_die ?
+   CONV_EOL_RNDTRP_DIE : CONV_EOL_RNDTRP_WARN;
return 0;
}
 
diff --git a/convert.c b/convert.c
index 20d7ab67bd..f39150cde9 100644
--- a/convert.c
+++ b/convert.c
@@ -193,30 +193,30 @@ static enum eol output_eol(enum crlf_action crlf_action)
return core_eol;
 }
 
-static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
+static void check_global_conv_flags_eol(const char *path, enum crlf_action 
crlf_action,
struct text_stat *old_stats, struct text_stat 
*new_stats,
-   enum safe_crlf checksafe)
+   int conv_flags)
 {
if (old_stats->crlf && !new_stats->crlf ) {
/*
 * CRLFs would not be restored by checkout
 */
-   if (checksafe == SAFE_CRLF_WARN)
+   

[PATCH v3 0/7] convert: add support for different encodings

[lars . schneider]

From: Lars Schneider 

Hi,

Patches 1-5 and 6 are helper functions and preparation.
Patch 6 is the actual change.

I am still torn between "checkout-encoding" and "working-tree-encoding"
as attribute name. I am happy to hear arguments for/against one or the
other.

Changes since v2:

* Added Torsten's crlfsave refactoring patch (patch 5)
  @Torsten: I tried to make the commit message more clean, added
some comments to and renamed conv_flags_eol to
global_conv_flags_eol.

* Improved documentation and commit message (Torsten)

* Removed unnecessary NUL assignment in xstrdup_tolower() (Torsten)

* Set "git config core.eol lf" to made the test run on Windows (Dscho)

* Made BOM arrays static (Ramsay)


Thanks,
Lars


RFC: 
https://public-inbox.org/git/bdb9b884-6d17-4be3-a83c-f67e2afa2...@gmail.com/
 v1: 
https://public-inbox.org/git/20171211155023.1405-1-lars.schnei...@autodesk.com/
 v2: 
https://public-inbox.org/git/2017122915.39680-1-lars.schnei...@autodesk.com/



Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/f21a1841a4
Checkout: git fetch https://github.com/larsxschneider/git encoding-v3 && git 
checkout f21a1841a4


### Interdiff (v2..v3):

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 0039bd38c3..1bc03e69cb 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -285,11 +285,18 @@ In these cases you can teach Git the encoding of a file 
in the working
 directory with the `checkout-encoding` attribute. If a file with this
 attributes is added to Git, then Git reencodes the content from the
 specified encoding to UTF-8 and stores the result in its internal data
-structure. On checkout the content is encoded back to the specified
-encoding.
+structure (called "the index"). On checkout the content is encoded
+back to the specified encoding.

-Please note that using the `checkout-encoding` attribute has a number
-of drawbacks:
+Please note that using the `checkout-encoding` attribute may have a
+number of pitfalls:
+
+- Git clients that do not support the `checkout-encoding` attribute
+  will checkout the respective files UTF-8 encoded and not in the
+  expected encoding. Consequently, these files will appear different
+  which typically causes trouble. This is in particular the case for
+  older Git versions and alternative Git implementations such as JGit
+  or libgit2 (as of January 2018).

 - Reencoding content to non-UTF encodings (e.g. SHIFT-JIS) can cause
   errors as the conversion might not be round trip safe.
@@ -297,12 +304,6 @@ of drawbacks:
 - Reencoding content requires resources that might slow down certain
   Git operations (e.g 'git checkout' or 'git add').

-- Git clients that do not support the `checkout-encoding` attribute or
-  the used encoding will checkout the respective files as UTF-8 encoded.
-  That means the content appears to be different which could cause
-  trouble. Affected clients are older Git versions and alternative Git
-  implementations such as JGit or libgit2 (as of January 2018).
-
 Use the `checkout-encoding` attribute only if you cannot store a file in
 UTF-8 encoding and if you want Git to be able to process the content as
 text.
diff --git a/apply.c b/apply.c
index c4bd5cf1f2..f8b67bfee2 100644
--- a/apply.c
+++ b/apply.c
@@ -2263,8 +2263,8 @@ static void show_stats(struct apply_state *state, struct 
patch *patch)
 static int read_old_data(struct stat *st, struct patch *patch,
 const char *path, struct strbuf *buf)
 {
-   enum safe_crlf safe_crlf = patch->crlf_in_old ?
-   SAFE_CRLF_KEEP_CRLF : SAFE_CRLF_RENORMALIZE;
+   int conv_flags = patch->crlf_in_old ?
+   CONV_EOL_KEEP_CRLF : CONV_EOL_RENORMALIZE;
switch (st->st_mode & S_IFMT) {
case S_IFLNK:
if (strbuf_readlink(buf, path, st->st_size) < 0)
@@ -2281,7 +2281,7 @@ static int read_old_data(struct stat *st, struct patch 
*patch,
 * should never look at the index when explicit crlf option
 * is given.
 */
-   convert_to_git(NULL, path, buf->buf, buf->len, buf, safe_crlf, 
0);
+   convert_to_git(NULL, path, buf->buf, buf->len, buf, conv_flags);
return 0;
default:
return -1;
diff --git a/blame.c b/blame.c
index 388b66897b..2893f3c103 100644
--- a/blame.c
+++ b/blame.c
@@ -229,7 +229,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
if (strbuf_read(, 0, 0) < 0)
die_errno("failed to read from stdin");
}
-   convert_to_git(_index, path, buf.buf, buf.len, , 0, 0);
+   convert_to_git(_index, path, buf.buf, buf.len, , 0);
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_oid.hash);
diff --git 

[PATCH v2] stash: don't delete untracked files that match pathspec

[Thomas Gummerer]

Currently when 'git stash push -- ' is used, untracked files
that match the pathspec will be deleted, even though they do not end up
in a stash anywhere.

This is because the original commit introducing the pathspec feature in
git stash push (df6bba0937 ("stash: teach 'push' (and 'create_stash') to
honor pathspec", 2017-02-28)) used the sequence of 'git reset 
&& git ls-files --modified  | git checkout-index && git clean
'.

The intention was to emulate what 'git reset --hard -- ' would
do.  The call to 'git clean' was supposed to clean up the files that
were unstaged by 'git reset'.  This would work fine if the pathspec
doesn't match any files that were untracked before 'git stash push --
'.  However if  matches a file that was untracked
before invoking the 'stash' command, all untracked files matching the
pathspec would inadvertently be deleted as well, even though they
wouldn't end up in the stash, and are therefore lost.

This behaviour was never what was intended, only blobs that also end up
in the stash should be reset to their state in HEAD, previously
untracked files should be left alone.

To achieve this, first match what's in the index and what's in the
working tree by adding all changes to the index, ask diff-index what
changed between HEAD and the current index, and then apply that patch in
reverse to get rid of the changes, which includes removal of added
files and resurrection of removed files.

Reported-by: Reid Price 
Helped-by: Junio C Hamano 
Signed-off-by: Thomas Gummerer 
---
> Thanks, I'll fill in the gaps, and send a new patch, hopefully over
> the weekend.

Here it is :)

Changes since the previous version:
 - handle binary files correctly and add a test for that
 - updated commit message
 - took back authorship, and added Helped-by: Junio as he mentioned
   would be the best way earlier in the thread.

 git-stash.sh |  5 ++---
 t/t3903-stash.sh | 32 
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 1114005ce2..fc8f8ae640 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -322,10 +322,9 @@ push_stash () {
 
if test $# != 0
then
-   git reset -q -- "$@"
-   git ls-files -z --modified -- "$@" |
+   git add -u -- "$@" |
git checkout-index -z --force --stdin
-   git clean --force -q -d -- "$@"
+   git diff-index -p --cached --binary HEAD -- "$@" | git 
apply --index -R
else
git reset --hard -q
fi
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 39c7f2ebd7..aefde7b172 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1064,4 +1064,36 @@ test_expect_success 'stash -k --  leaves 
unstaged files intact' '
test foo,bar = $(cat foo),$(cat bar)
 '
 
+test_expect_success 'stash --  leaves untracked files in subdir 
intact' '
+   git reset &&
+   >subdir/untracked &&
+   >subdir/tracked1 &&
+   >subdir/tracked2 &&
+   git add subdir/tracked* &&
+   git stash -- subdir/ &&
+   test_path_is_missing subdir/tracked1 &&
+   test_path_is_missing subdir/tracked2 &&
+   test_path_is_file subdir/untracked &&
+   git stash pop &&
+   test_path_is_file subdir/tracked1 &&
+   test_path_is_file subdir/tracked2 &&
+   test_path_is_file subdir/untracked
+'
+
+test_expect_success 'stash --  works with binary files' '
+   git reset &&
+   >subdir/untracked &&
+   >subdir/tracked &&
+   cp "$TEST_DIRECTORY"/test-binary-1.png subdir/tracked-binary &&
+   git add subdir/tracked* &&
+   git stash -- subdir/ &&
+   test_path_is_missing subdir/tracked &&
+   test_path_is_missing subdir/tracked-binary &&
+   test_path_is_file subdir/untracked &&
+   git stash pop &&
+   test_path_is_file subdir/tracked &&
+   test_path_is_file subdir/tracked-binary &&
+   test_path_is_file subdir/untracked
+'
+
 test_done
-- 
2.15.1.620.gb9897f4670

Re: Can't squash merge with merge.ff set to false

[Jonathan Nieder]

Hi,

Robert Dailey wrote:

> Not sure if this is intended or a bug, but with the following configuration:
>
> $ git config --global merge.ff false
>
> I am not able to merge my topic branch into master with squash option:
>
> $ git checkout master
> $ git merge --squash topic
> fatal: You cannot combine --squash with --no-ff.

I see two issues here:

 1. The check and error message really only make sense when you passed
--no-ff directly, not implicitly using config.  The problem you
are running into was presumably introduced when merge.ff was added
in v1.7.6-rc0~67^2~1 (2011-05-06).

 2. Whether it comes from an alias or config, --no-ff and --squash are
not fundamentally incompatible.  --no-ff says not to do something
and --squash says to do a different thing, so --squash should win.

So I suspect that making --squash override --no-ff would be a
reasonable behavior.

Care to write a patch?

Thanks,
Jonathan

Re: [PATCH 20/26] fetch-pack: perform a fetch using v2

[Brandon Williams]

On 01/03, Stefan Beller wrote:
> On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> > +
> > +#define FETCH_CHECK_LOCAL 0
> > +#define FETCH_SEND_REQUEST 1
> > +#define FETCH_PROCESS_ACKS 2
> > +#define FETCH_SEND_HAVES 3
> > +#define FETCH_GET_PACK 4
> > +#define FETCH_DONE 5
> > +
> > +static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> > +   int fd[2],
> > +   const struct ref *orig_ref,
> > +   struct ref **sought, int nr_sought,
> > +   char **pack_lockfile)
> > +{
> > +   struct ref *ref = copy_ref_list(orig_ref);
> > +   int state = FETCH_CHECK_LOCAL;
> 
> Is there any reason to use #defines over an enum here?
> 

No, it would probably be better to use an enum, that would also get rid
of the default case of the switch statement.

-- 
Brandon Williams

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

[Brandon Williams]

On 01/03, Stefan Beller wrote:
> On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> > Introduce the ls-refs server command.  In protocol v2, the ls-refs
> > command is used to request the ref advertisement from the server.  Since
> > it is a command which can be requested (as opposed to mandatory in v1),
> > a client can sent a number of parameters in its request to limit the ref
> > advertisement based on provided ref-patterns.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  Documentation/technical/protocol-v2.txt | 26 +
> >  Makefile|  1 +
> >  ls-refs.c   | 97 
> > +
> >  ls-refs.h   |  9 +++
> 
> Maybe consider putting any served command into a sub directory?
> 
> For example the code in builtin/ has laxer rules w.r.t. die()ing
> as it is a user facing command, whereas some devs want to see
> code at the root of the repo to not die() at all as the eventual goal
> is to have a library there.
> All this code is on the remote side, which also has different traits than
> the code at the root of the git.git repo; non-localisation comes to mind,
> but there might be other aspects as well (security?).

Well if we were to do this then we should move upload-pack and
receive-pack into this same "server code" directory.

> 
> 
> >  serve.c |  2 +
> >  5 files changed, 135 insertions(+)
> >  create mode 100644 ls-refs.c
> >  create mode 100644 ls-refs.h
> >
> > diff --git a/Documentation/technical/protocol-v2.txt 
> > b/Documentation/technical/protocol-v2.txt
> > index b87ba3816..5f4d0e719 100644
> > --- a/Documentation/technical/protocol-v2.txt
> > +++ b/Documentation/technical/protocol-v2.txt
> > @@ -89,3 +89,29 @@ terminate the connection.
> >  Commands are the core actions that a client wants to perform (fetch, push,
> >  etc).  Each command will be provided with a list capabilities and
> >  arguments as requested by a client.
> > +
> > + Ls-refs
> 
> So is it ls-refs or Ls-refs or is any capitalization valid?

"ls-refs"  I'll make sure to change this.
> 
> > +-
> > +
> > +Ls-refs is the command used to request a reference advertisement in v2.
> > +Unlike the current reference advertisement, ls-refs takes in parameters
> > +which can be used to limit the refs sent from the server.
> > +
> > +Ls-ref takes in the following parameters wraped in packet-lines:
> > +
> > +  symrefs: In addition to the object pointed by it, show the underlying
> > +  ref pointed by it when showing a symbolic ref.
> > +  peel: Show peeled tags.
> > +  ref-pattern : When specified, only references matching the
> > +given patterns are displayed.
> 
> What kind of pattern matching is allowed here?
> strictly prefix only, or globbing, regexes?
> Is there a given grammar to follow? Maybe a link to the git
> glossary is or somewhere else might be fine.
> 
> Seeing that we do wildmatch() down there (as opposed to regexes),
> I wonder if it provides an entry for a denial of service attack, by crafting
> a pattern that is very expensive for the server to compute but cheap to
> ask for from a client. (c.f. 94da9193a6 (grep: add support for PCRE v2,
> 2017-06-01, but that is regexes!)
> 
> > +The output of ls-refs is as follows:
> > +
> > +output = *ref
> > +flush-pkt
> > +ref = PKT-LINE((tip | peeled) LF)
> > +tip = obj-id SP refname (SP symref-target)
> > +peeled = obj-id SP refname "^{}"
> > +
> > +symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
> > +shallow = PKT-LINE("shallow" SP obj-id LF)
> > diff --git a/Makefile b/Makefile
> > index 5f3b5fe8b..152a73bec 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -820,6 +820,7 @@ LIB_OBJS += list-objects-filter-options.o
> >  LIB_OBJS += ll-merge.o
> >  LIB_OBJS += lockfile.o
> >  LIB_OBJS += log-tree.o
> > +LIB_OBJS += ls-refs.o
> >  LIB_OBJS += mailinfo.o
> >  LIB_OBJS += mailmap.o
> >  LIB_OBJS += match-trees.o
> > diff --git a/ls-refs.c b/ls-refs.c
> > new file mode 100644
> > index 0..ac4904a40
> > --- /dev/null
> > +++ b/ls-refs.c
> > @@ -0,0 +1,97 @@
> > +#include "cache.h"
> > +#include "repository.h"
> > +#include "refs.h"
> > +#include "remote.h"
> > +#include "argv-array.h"
> > +#include "ls-refs.h"
> > +#include "pkt-line.h"
> > +
> > +struct ls_refs_data {
> > +   unsigned peel;
> > +   unsigned symrefs;
> > +   struct argv_array patterns;
> > +};
> > +
> > +/*
> > + * Check if one of the patterns matches the tail part of the ref.
> > + * If no patterns were provided, all refs match.
> > + */
> > +static int ref_match(const struct argv_array *patterns, const char 
> > *refname)
> > +{
> > +   char *pathbuf;
> > +   int i;
> > +
> > +   if (!patterns->argc)
> > +   return 1; /* no restriction */
> > +
> > +   pathbuf = xstrfmt("/%s", 

Re: [PATCH 01/26] pkt-line: introduce packet_read_with_status

[Brandon Williams]

On 01/03, Stefan Beller wrote:
> On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> > The current pkt-line API encodes the status of a pkt-line read in the
> > length of the read content.  An error is indicated with '-1', a flush
> > with '0' (which can be confusing since a return value of '0' can also
> > indicate an empty pkt-line), and a positive integer for the length of
> > the read content otherwise.  This doesn't leave much room for allowing
> > the addition of additional special packets in the future.
> >
> > To solve this introduce 'packet_read_with_status()' which reads a packet
> > and returns the status of the read encoded as an 'enum packet_status'
> > type.  This allows for easily identifying between special and normal
> > packets as well as errors.  It also enables easily adding a new special
> > packet in the future.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  pkt-line.c | 55 ++-
> >  pkt-line.h | 15 +++
> >  2 files changed, 57 insertions(+), 13 deletions(-)
> >
> > diff --git a/pkt-line.c b/pkt-line.c
> > index 2827ca772..8d7cd389f 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
> > return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
> >  }
> >
> > -int packet_read(int fd, char **src_buf, size_t *src_len,
> > -   char *buffer, unsigned size, int options)
> > +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
> > size_t *src_len,
> > +   char *buffer, unsigned 
> > size, int *pktlen,
> > +   int options)
> >  {
> > -   int len, ret;
> > +   int len;
> > char linelen[4];
> >
> > -   ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> > -   if (ret < 0)
> > -   return ret;
> > +   if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 
> > 0)
> > +   return PACKET_READ_EOF;
> > +
> > len = packet_length(linelen);
> > if (len < 0)
> > die("protocol error: bad line length character: %.4s", 
> > linelen);
> > -   if (!len) {
> > +
> > +   if (len == 0) {
> > packet_trace("", 4, 0);
> > -   return 0;
> > +   return PACKET_READ_FLUSH;
> > +   } else if (len >= 1 && len <= 3) {
> > +   die("protocol error: bad line length character: %.4s", 
> > linelen);
> 
> I wonder how much libified code we want here already, maybe we could
> have PACKET_READ_ERROR as a return value here instead of die()ing.
> There could also be an option that tells this code to die on error, this 
> reminds
> me of the repository discovery as well as the refs code, both of which have
> this pattern.
> 
> Currently this series is only upgrading commands that use the network
> anyway, so I guess die()ing in an ls-remote or fetch is no big deal,
> but it could
> be interesting to keep going once we have more of the partial clone
> stuff working
> (e.g. remote assisted log/blame would want to gracefully fall back instead of
> die()ing without any useful output, I would think.)

These are all things we could do, but the current code just dies and it
may be more hassle right now to change all the uses of packet_read to
handle errors gracefully.  But its definitely something we can do in the
future.

> 
> > }
> > +
> > len -= 4;
> > -   if (len >= size)
> > +   if ((len < 0) || ((unsigned)len >= size))
> > die("protocol error: bad line length %d", len);
> > -   ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
> > -   if (ret < 0)
> > -   return ret;
> > +
> > +   if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) 
> > < 0)
> > +   return PACKET_READ_EOF;
> >
> > if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> > len && buffer[len-1] == '\n')
> > @@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t 
> > *src_len,
> >
> > buffer[len] = 0;
> > packet_trace(buffer, len, 0);
> > -   return len;
> > +   *pktlen = len;
> > +   return PACKET_READ_NORMAL;
> > +}
> > +
> > +int packet_read(int fd, char **src_buffer, size_t *src_len,
> > +   char *buffer, unsigned size, int options)
> > +{
> > +   enum packet_read_status status;
> > +   int pktlen;
> > +
> > +   status = packet_read_with_status(fd, src_buffer, src_len,
> > +buffer, size, ,
> > +options);
> > +   switch (status) {
> > +   case PACKET_READ_EOF:
> > +   pktlen = -1;
> > +   break;
> > +   case PACKET_READ_NORMAL:
> > +   break;
> > +   case 

[ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

A release candidate Git v2.16.0-rc1 is now available for testing
at the usual places.  It is comprised of 455 non-merge commits
since v2.15.0, contributed by 79 people, 23 of which are new faces.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the
'v2.16.0-rc1' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.15.0 are as follows.
Welcome to the Git development community!

  Albert Astals Cid, Antoine Beaupré, Damien Marié, Daniel
  Bensoussan, Florian Klink, Gennady Kupava, Guillaume Castagnino,
  Haaris Mehmood, Hans Jerry Illikainen, Ingo Ruhnke, Jakub
  Bereżański, Jean Carlo Machado, Julien Dusser, J Wyman,
  Kevin, Łukasz Stelmach, Marius Paliga, Olga Telezhnaya,
  Rafael Ascensão, Robert Abel, Robert P. J. Day, Shuyu Wei,
  and Wei Shuyu.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Adam Dinwoodie, Ævar Arnfjörð Bjarmason, Alex Vandiver,
  Anders Kaseorg, Andrey Okoshkin, Ann T Ropea, Beat Bolli,
  Ben Peart, Brandon Williams, brian m. carlson, Carlos Martín
  Nieto, Charles Bailey, Christian Couder, Dave Borowitz, Dennis
  Kaarsemaker, Derrick Stolee, Elijah Newren, Emily Xie, Eric
  Sunshine, Eric Wong, Heiko Voigt, Jacob Keller, Jameson Miller,
  Jean-Noel Avila, Jeff Hostetler, Jeff King, Johannes Schindelin,
  Jonathan Nieder, Jonathan Tan, Junio C Hamano, Kaartic Sivaraam,
  Kevin Daudt, Lars Schneider, Liam Beguin, Luke Diamand, Martin
  Ågren, Michael Haggerty, Nicolas Morey-Chaisemartin, Phil
  Hord, Phillip Wood, Pranit Bauva, Prathamesh Chavan, Ramsay
  Jones, Randall S. Becker, Rasmus Villemoes, René Scharfe,
  Simon Ruderich, Stefan Beller, Steffen Prohaska, Stephan Beyer,
  SZEDER Gábor, Thomas Braun, Thomas Gummerer, Todd Zullinger,
  Torsten Bögershausen, and W. Trevor King.



Git 2.16 Release Notes (draft)
==

Backward compatibility notes and other notable changes.

 * Use of an empty string as a pathspec element that is used for
   'everything matches' is now an error.


Updates since v2.15
---

UI, Workflows & Features

 * An empty string as a pathspec element that means "everything"
   i.e. 'git add ""', is now illegal.  We started this by first
   deprecating and warning a pathspec that has such an element in
   2.11 (Nov 2016).

 * A hook script that is set unexecutable is simply ignored.  Git
   notifies when such a file is ignored, unless the message is
   squelched via advice.ignoredHook configuration.

 * "git pull" has been taught to accept "--[no-]signoff" option and
   pass it down to "git merge".

 * The "--push-option=" option to "git push" now defaults to a
   list of strings configured via push.pushOption variable.

 * "gitweb" checks if a directory is searchable with Perl's "-x"
   operator, which can be enhanced by using "filetest 'access'"
   pragma, which now we do.

 * "git stash save" has been deprecated in favour of "git stash push".

 * The set of paths output from "git status --ignored" was tied
   closely with its "--untracked=" option, but now it can be
   controlled more flexibly.  Most notably, a directory that is
   ignored because it is listed to be ignored in the ignore/exclude
   mechanism can be handled differently from a directory that ends up
   to be ignored only because all files in it are ignored.

 * The remote-helper for talking to MediaWiki has been updated to
   truncate an overlong pagename so that ".mw" suffix can still be
   added.

 * The remote-helper for talking to MediaWiki has been updated to
   work with mediawiki namespaces.

 * The "--format=..." option "git for-each-ref" takes learned to show
   the name of the 'remote' repository and the ref at the remote side
   that is affected for 'upstream' and 'push' via "%(push:remotename)"
   and friends.

 * Doc and message updates to teach users "bisect view" is a synonym
   for "bisect visualize".

 * "git bisect run" that did not specify any command to run used to go
   ahead and treated all commits to be tested as 'good'.  This has
   been corrected by making the command error out.

 * The SubmittingPatches document has been converted to produce an
   HTML version via AsciiDoc/Asciidoctor.

 * We learned to talk to watchman to speed up "git status" and other
   operations that need to see which paths have been modified.

 * The "diff" family of commands learned to ignore differences in
   carriage return at the end of line.

 * Places that know about "sendemail.to", like documentation and shell
   completion (in contrib/) have been taught about "sendemail.tocmd",
   too.

 * "git add --renormalize ." is a new and safer way to record the fact
   

What's cooking in git.git (Jan 2018, #01; Fri, 5)

[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.

The tip of 'master' has been tagged as 2.16-rc1; I am aware that a
handful of larger topics are on the list, yet to be picked up, but
now we are pretty much in feature-freeze mode before the release;
please shift your attention to fixes from shiny new toys.

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"]

* bp/fsmonitor (2017-12-18) 1 commit
  (merged to 'next' on 2017-12-27 at ce216e2978)
 + p7519: improve check for prerequisite WATCHMAN

 Test fix.


* bw/path-doc (2017-12-13) 1 commit
  (merged to 'next' on 2017-12-19 at 2cddee77ca)
 + path: document path functions

 Doc updates.


* cc/skip-to-optional-val (2017-12-11) 7 commits
  (merged to 'next' on 2017-12-27 at 1b189d8556)
 + t4045: reindent to make helpers readable
 + diff: add tests for --relative without optional prefix value
 + diff: use skip_to_optional_arg_default() in parsing --relative
 + diff: use skip_to_optional_arg_default()
 + diff: use skip_to_optional_arg()
 + index-pack: use skip_to_optional_arg()
 + git-compat-util: introduce skip_to_optional_arg()

 Introduce a helper to simplify code to parse a common pattern that
 expects either "--key" or "--key=".


* db/doc-config-section-names-with-bs (2017-12-22) 1 commit
  (merged to 'next' on 2017-12-28 at e744b99449)
 + config.txt: document behavior of backslashes in subsections

 Doc update.


* ew/empty-merge-with-dirty-index (2017-12-22) 1 commit
  (merged to 'next' on 2017-12-28 at 17aa5ec010)
 + 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.)

 "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.


* 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.


* ew/svn-crlf (2017-12-14) 2 commits
  (merged to 'next' on 2017-12-27 at 1b81bd634d)
 + Merge branch 'svn-crlf' of git://bogomips.org/git-svn into ew/svn-crlf
 + git-svn: convert CRLF to LF in commit message to SVN

 "git svn" has been updated to strip CRs in the commit messages, as
 recent versions of Subversion rejects them.


* hi/merge-verify-sig-config (2017-12-19) 3 commits
  (merged to 'next' on 2017-12-27 at 34360fb1c1)
 + t5573, t7612: clean up after unexpected success of 'pull' and 'merge'
  (merged to 'next' on 2017-12-14 at cdc511dc36)
 + t: add tests for pull --verify-signatures
 + merge: add config option for verifySignatures

 "git merge" learned to pay attention to merge.verifySignatures
 configuration variable and pretend as if '--verify-signatures'
 option was given from the command line.


* jd/fix-strbuf-add-urlencode-bytes (2017-12-22) 1 commit
  (merged to 'next' on 2017-12-28 at 7f4f291966)
 + strbuf: fix urlencode format string on signed char

 Bytes with high-bit set were encoded incorrectly and made
 credential helper fail.


* jh/memihash-opt (2017-12-22) 1 commit
  (merged to 'next' on 2017-12-28 at bf96e0d849)
 + t/helper/test-lazy-name-hash: fix compilation

 Squelch compiler warning.


* jh/partial-clone-doc (2017-12-14) 1 commit
  (merged to 'next' on 2017-12-27 at 3695847773)
 + partial-clone: design doc


* jk/test-suite-tracing (2017-12-08) 4 commits
  (merged to 'next' on 2017-12-27 at 7034a51474)
 + t/Makefile: introduce TEST_SHELL_PATH
 + test-lib: make "-x" work with "--verbose-log"
 + t5615: avoid re-using descriptor 4
 + test-lib: silence "-x" cleanup under bash

 Assorted fixes around running tests with "-x" tracing option.


* js/enhanced-version-info (2017-12-14) 2 commits
  (merged to 'next' on 2017-12-27 at a95dd96a78)
 + version --build-options: report commit, too, if possible
 + version --build-options: also report host CPU

 "git version --build-options" learned to report the host CPU and
 the exact commit object name the binary was built from.


* js/sequencer-cleanups (2017-12-27) 5 commits
  (merged to 'next' on 2017-12-28 at 23c10afb09)
 + sequencer: do not invent whitespace when transforming OIDs
 + sequencer: report when noop has an argument
 + sequencer: remove superfluous conditional
 + 

Re: [PATCH 1/5] convert_to_git(): checksafe becomes an integer

[Junio C Hamano]

Lars Schneider  writes:

>> On 31 Dec 2017, at 09:05, tbo...@web.de wrote:
>> 
>> From: Torsten Bögershausen 
>> 
>> When calling convert_to_git(), the checksafe parameter has been used to
>> check if commit would give a non-roundtrip conversion of EOL.
>> 
>> When checksafe was introduced, 3 values had been in use:
>> SAFE_CRLF_FALSE: no warning
>> SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
>> SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip
>
> In general, I really like the direction as this simplifies
> my patch later on in 5/5. However, I see a few problems:
> ...

Yes, this looks like a sensible way to go.  I saw Torsten's v3 for
1/5 but will end up queuing v2b, as I suspect 5/5 would need to be
adjusted for the change between the two versions.

Thanks.

Hello Dear

[Ruth Mawere]

--
Hello Dear,

Its me Ruth, did you receive my previous email? please write me back 
immediately to let me know, i wait to hear back from you, kisses.

Re: [PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Skip the newly added file creation tests on Windows proper, these
> already work under Cygwin, but as that involves a significant
> emulation layer the results are different under Windows proper with
> MinGW.
> ...
> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> index f606f91acb..50a53e7a62 100755
> --- a/t/t3070-wildmatch.sh
> +++ b/t/t3070-wildmatch.sh
> @@ -7,6 +7,14 @@ test_description='wildmatch tests'
>  create_test_file() {
>   file=$1
>  
> + # These tests limp along under Cygwin, but the failures on
> + # native Windows are still too many. Skip file tests there
> + # until they're sorted out.
> + if test_have_prereq MINGW
> + then
> + return 1
> + fi
> +

That looks to be a nuclear option.  For now it may be suffice, but
somehow it feels that it goes directly against Dscho's wish to treat
(or pretend to treat) Windows as first-class citizens and/or just
like other platforms.

Re: Bug report: git clone with dest

[Junio C Hamano]

Jeff King  writes:

> On Fri, Jan 05, 2018 at 12:45:03PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > Out of curiosity, did this change at some point? I thought the process
>> > used to be to merge to maint, and then pick up topics in master by
>> > merging maint to master.
>> 
>> If you look at "Sync with maint" merges made to 'master', you'd
>> notice that most of them are only updating Documentation/RelNotes/*
>> and otherwise no-effect merges, simply because when such an up-merge
>> is made, everything in 'maint' is already in 'master' because topics
>> are merged to the latter first.  Security fixes that go through
>> embargoes are excempt for obvious reasons ;-)
>
> OK, that makes sense. Pretty sure I did it wrong when I was interim
> maintainer back in the day, then. :)

Well, MaintNotes on the 'todo' branch needs a bit of updating, as it
says something somewhat misleading.

-- >8 --
Subject: MaintNotes: clarify the purpose of maint->master upmerge

Even though the paragraph before this one is pretty clear that
topics are first merged to 'master' and then to 'maint', it was
misleading to say 'maint' is merged to 'master' to propagate fixes
forward, as most of the time, such an upmerge is a noop because
topics merged to 'maint' are usually merged to 'master' already.

These up-merges are done primarily to make sure that the tip of
'master' has updated release notes from all the maintenance tracks,
so be explicit about that to avoid confusion.

Signed-off-by: Junio C Hamano 
---
 MaintNotes | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MaintNotes b/MaintNotes
index 3a70b88..393d81f 100644
--- a/MaintNotes
+++ b/MaintNotes
@@ -173,8 +173,8 @@ feature release).  These days, maintenance releases are 
named by
 incrementing the last digit of three-dotted decimal name (e.g. "2.12.1"
 was the first maintenance release for the "2.12" series).
 
-New features never go to the 'maint' branch.  This branch is also
-merged into "master" to propagate the fixes forward as needed.
+New features never go to the 'maint' branch.  It is merged into "master"
+primarily to propagate the description in the release notes forward.
 
 A new development does not usually happen on "master". When you send a
 series of patches, after review on the mailing list, a separate topic

Re: [PATCH] perf: amend the grep tests to test grep.threads

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> Judging by the way the other side uses "test_perf $prereq ..."
>> without quotes around it, I suspect you do expect it to be empty in
>> some cases.  It means you expect test_have_prereq is prepared to
>> skip an empty prerequisite in a prereq list, but I do not recall
>> writing that helper in such a way, so...
>>
>>  PTHREADS${prereq:+,}$prereq
>>
>> or something along that line, perhaps?
>
> It's not, but a trailing comma at the end of the prereq list works since
> test_have_prereq() relies on setting the IFS to ",", so as long as this
> is portable:
> ...
> We could use the ${...} pattern above if you prefer, but since it
> doesn't appear to be needed...

I view that reasoning as depending more on how the helper happens to
be implemented right now than working by design, though.

Re: [PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`

[Junio C Hamano]

Johannes Schindelin  writes:

>> diff --git a/diff-lib.c b/diff-lib.c
>> index 8104603a3..13ff00d81 100644
>> --- a/diff-lib.c
>> +++ b/diff-lib.c
>> @@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int 
>> option)
>>  
>>  diff_set_mnemonic_prefix(>diffopt, "i/", "w/");
>>  
>> +if (!(option & DIFF_SKIP_FSMONITOR))
>> +refresh_fsmonitor(_index);
>> +
>>  if (diff_unmerged_stage < 0)
>>  diff_unmerged_stage = 2;
>
> I read over this hunk five times, and only now am I able to wrap my head
> around this: if we do *not* want to skip the fsmonitor data, we refresh
> the fsmonitor data in the index.
>
> That feels a bit like an unneeded double negation. Speaking for myself, I
> would prefore `DIFF_IGNORE_FSMONITOR` instead, it would feel less like a
> double negation then. But I am not a native speaker, so I might be wrong.

I do find the logic a bit convoluted with double negative.

Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip

[Junio C Hamano]

Yasushi SHOJI  writes:

> The patch (actually, I've tested the one in pu, 2e9fdc795cb27) avoids
> the seg fault for sure, but the question is:
>
> When does the list allowed to contain NULLs?

A very legitimate question.  With the proposed log message alone, it
is even tempting to declare that the change may merely be sweeping
the issue under the rug.  A bit better explanation is needed, at
least.

[PATCH v4 8/7] wildmatch test: skip file creation tests on Windows proper

[Ævar Arnfjörð Bjarmason]

Skip the newly added file creation tests on Windows proper, these
already work under Cygwin, but as that involves a significant
emulation layer the results are different under Windows proper with
MinGW.

Ideally we'd get exhaustive coverage for this area on all platforms,
but having any increase in test coverage anywhere is a net
improvement.

Particularly in this case where there's no reason to suspect (aside
from perhaps odd edge case like \foo meaning C:\foo) that the actual
pattern matching engine will behave differently on Windows. The tests
can't be run due to limitations elsewhere.

The thread starting at
https://public-inbox.org/git/?q=nycvar.QRO.7.76.6.1801051622010.1337%40wbunaarf-fpuvaqryva.tvgsbejvaqbjf.bet
has more details about specific issues under Windows.

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

On Fri, Jan 05 2018, Johannes Schindelin jotted:

> Hi Ævar,
>
> On Fri, 5 Jan 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Fri, Jan 05 2018, Johannes Schindelin jotted:
>> 
>> > [...]
>> >
>> > In short: the Unix shell script t3070 manages to write what it thinks is a
>> > file called 'foo*', but Git only sees 'foo'.
>> >
>> > I tried to address this problem with this patch:
>> 
>> ...I don't see any particular value in trying to do these full roundtrip
>> tests on platforms like Windows. Perhaps we should just do these on a
>> whitelist of POSIX systems for now, and leave expanding that list to
>> some future step.
>
> I don't think so. Windows is already handled as a second-class citizen, as
> if nobody developed on it. As a consequence, only very few of the
> gazillions of Windows developers... develop Git. We could worsify the
> situation, of course, but why? Shouldn't we at least pretend to try the
> opposite?

I don't think we should never test for this on MinGW, but given the
increase in test coverage, and not making perfect the enemy of the
good I think (as explained in the commit message above) that we're
better off *starting* with just disabling these tests under MinGW, and
then fixing that platform later.

> [...]
> That's all good and dandy, but what about regressions? I know how much I
> will curse in your vague direction when I encounter the next
> wildmatch-related bug in, say, half a year and have to wade through the
> jungle of unintuitive tests in t3070.

If we have a new wildmatch-related bug we'll be a lot better off with
exhaustive test coverage, even if we can only run those tests on
*nix-like platforms.

> Can't we do a lot better than this? Shouldn't it be a lot more obvious
> what the heck went wrong when running t3070 with -i -v -x?

I had something closer to that in v1 in
20171223213012.1962-7-ava...@gmail.com, but trying again I didn't find
a good way to compromise between -x readability and the entire patch
basically not being the copy/pasted code all over again, and I think
e.g. doing string interpolation into the test code would be even
nastier.

 t/t3070-wildmatch.sh | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index f606f91acb..50a53e7a62 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -7,6 +7,14 @@ test_description='wildmatch tests'
 create_test_file() {
file=$1
 
+   # These tests limp along under Cygwin, but the failures on
+   # native Windows are still too many. Skip file tests there
+   # until they're sorted out.
+   if test_have_prereq MINGW
+   then
+   return 1
+   fi
+
case $file in
# `touch .` will succeed but obviously not do what we intend
# here.
@@ -28,7 +36,7 @@ create_test_file() {
*/)
return 1
;;
-   # On Windows, \ in paths is silently converted to /, which
+   # On Cygwin, \ in paths is silently converted to /, which
# would result in the "touch" below working, but the test
# itself failing. See 6fd1106aa4 ("t3700: Skip a test with
# backslashes in pathspec", 2009-03-13) for prior art and
-- 
2.15.1.424.g9478a66081

Re: Git 2.15.0 on OSX 10.12.6: gui multi-select stage

[Matthew Orres]

Beautiful!

Pulled down that commit, was able to build and can confirm the issue
is fixed in git gui!

This has been a thorn in my side, so I appreciate your help! I look
forward to it being included in the next release for git in Homebrew!
:)

On Fri, Jan 5, 2018 at 4:41 PM, Johannes Schindelin  wrote:
> Hi Matthew,
>
> On Fri, 5 Jan 2018, Matthew Orres wrote:
>
>> builtin/checkout.c:24:10: fatal error: 'fscache.h' file not found
>> #include "fscache.h"
>>  ^~~
>> 1 error generated.
>
> I already had pushed a fix for this, have you re-pulled?
>
> Ciao,
> Johannes

Re: Can't squash merge with merge.ff set to false

[Robert Dailey]

On Fri, Jan 5, 2018 at 2:54 PM, Bryan Turner  wrote:
> The two _aren't_ distinctly separate, though. "git merge --squash
> --ff-only" has very different semantics to "git merge --squash --ff",
> in that it will only create a new squashed commit (or fast-forward a
> single commit) if the incoming commit(s) are fast-forward from the
> target. So there _is_ a setting for the fast-forward mode (given
> "--ff", "--ff-only", and "--no-ff" are a tri-state switch, and
> therefore comprise a single setting) that does impact squashing.

That feels really contrived to me though. For example, when I'm asking
to squash I don't really care about fast forward in that case.
Squashing means I'm expecting a possibly completely new commit with my
collective changes. If I only had one commit on my branch, likely I'd
be aware of that, and would do a fast forward merge or something. I
think the difference here is mind set. And maybe this is just me, but
the mentality when I choose --squash means I want nothing to do with
fast-foward. I don't care about it affecting the operation. If a
fast-foward happens to be the end result, I still don't care. Git made
that decision for me. And all I want is the end result: A single
commit.

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi Peff,

On Fri, 5 Jan 2018, Jeff King wrote:

> On Fri, Jan 05, 2018 at 09:22:07PM +0100, Johannes Schindelin wrote:
> 
> > On Fri, 5 Jan 2018, Isaac Shabtay wrote:
> > 
> > > Done: https://github.com/git-for-windows/git/pull/1421
> > > 
> > > I added credit to Jeff in the PR's description.
> > 
> > Sadly, the PR's description won't make it into the commit history, and the
> > authorship really should have been retained.
> > 
> > I found Peff's topic branch in his fork and force-pushed, to demonstrate
> > what I wanted to have. Currently the test suite is running (I test 64-bit
> > builds of the three major platforms Windows, macOS and Linux), and once
> > that is done and passed, I will merge the Pull Request.
> 
> I think the discussion has ended at "don't do anything else", but note
> that Junio and I were musing on whether to update the series around the
> dir_exists() function.

I briefly looked over this discussion and got the same impression.

> Which would then create headaches for you later when you try to merge a
> subtly-different series that makes it upstream.

Subtly-different is not a big problem. It is typically solved by `git
rebase --skip` ;-)

> Like I said, I think we've resolved not to do anything, but I wanted to
> point out a potential pitfall with this kind of "pick up a topic early"
> strategy (I'm intimately familiar with this pitfall because I do it all
> the time for the fork we run on our servers at GitHub).

Thanks for your concern.

And not to worry, I have plenty of expertise, won over the years, in
dealing with subtly different variants of patches having been accepted
upstream and conflicting with patches that were carried in Git for
Windows.

Ciao,
Dscho

Re: [PATCH] perf: amend the grep tests to test grep.threads

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 04 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Ever since 5b594f457a ("Threaded grep", 2010-01-25) the number of
>> threads git-grep uses under PTHREADS has been hardcoded to 8, but
>> there's no performance test to check whether this is an optimal
>> setting.
>>
>> Amend the existing tests for the grep engines to support a mode where
>> this can be tested, e.g.:
>>
>> GIT_PERF_GREP_THREADS='1 8 16' GIT_PERF_LARGE_REPO=~/g/linux ./run p782*
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> ---
>>
>> This looks less scary under diff -w.
>>
>>  t/perf/p7820-grep-engines.sh   | 52 ---
>>  t/perf/p7821-grep-engines-fixed.sh | 55 
>> ++
>>  2 files changed, 86 insertions(+), 21 deletions(-)
>>
>> diff --git a/t/perf/p7820-grep-engines.sh b/t/perf/p7820-grep-engines.sh
>> index 62aba19e76..8b09c5bf32 100755
>> --- a/t/perf/p7820-grep-engines.sh
>> +++ b/t/perf/p7820-grep-engines.sh
>> @@ -12,6 +12,9 @@ e.g. GIT_PERF_7820_GREP_OPTS=' -i'. Some options to try:
>> ...
>> +if ! test_have_prereq PERF_GREP_ENGINES_THREADS
>>  then
>> -test_cmp out.basic out.perl
>> +test_perf $prereq "$engine grep$GIT_PERF_7820_GREP_OPTS 
>> '$pattern'" "
>> +git -c grep.patternType=$engine 
>> grep$GIT_PERF_7820_GREP_OPTS -- '$pattern' >'out.$engine' || :
>> +"
>> +else
>> +for threads in $GIT_PERF_GREP_THREADS
>> +do
>> +test_perf PTHREADS,$prereq "$engine 
>> grep$GIT_PERF_7820_GREP_OPTS '$pattern' with $threads threads" "
>
> Is it guaranteed that $prereq is not empty at this point?
>
> Judging by the way the other side uses "test_perf $prereq ..."
> without quotes around it, I suspect you do expect it to be empty in
> some cases.  It means you expect test_have_prereq is prepared to
> skip an empty prerequisite in a prereq list, but I do not recall
> writing that helper in such a way, so...
>
>   PTHREADS${prereq:+,}$prereq
>
> or something along that line, perhaps?

It's not, but a trailing comma at the end of the prereq list works since
test_have_prereq() relies on setting the IFS to ",", so as long as this
is portable:

$ str="foo,bar,baz,"; export IFS=,; for word in $str; do echo "w:<$word>"; 
done
w:
w:
w:

And I'm not 100% sure that it is, this should be fine. Works on both
bash & dash for me.

We could use the ${...} pattern above if you prefer, but since it
doesn't appear to be needed...

>> diff --git a/t/perf/p7821-grep-engines-fixed.sh 
>> b/t/perf/p7821-grep-engines-fixed.sh
>> index c7ef1e198f..61e41b82cf 100755
>> --- a/t/perf/p7821-grep-engines-fixed.sh
>> +++ b/t/perf/p7821-grep-engines-fixed.sh
>> @@ -6,6 +6,9 @@ Set GIT_PERF_7821_GREP_OPTS in the environment to pass 
>> options to
>> ...
>>  for pattern in 'int' 'uncommon' 'æ'
>>  do
>>  for engine in fixed basic extended perl
>> @@ -23,19 +31,44 @@ do
>> ...
>> +if ! test_have_prereq PERF_GREP_ENGINES_THREADS
>>  then
>> -test_cmp out.fixed out.perl
>> +test_perf $prereq "$engine grep$GIT_PERF_7821_GREP_OPTS 
>> $pattern" "
>> +git -c grep.patternType=$engine 
>> grep$GIT_PERF_7821_GREP_OPTS $pattern >'out.$engine' || :
>> +"
>> +else
>> +for threads in $GIT_PERF_GREP_THREADS
>> +do
>> +test_perf PTHREADS,$prereq "$engine 
>> grep$GIT_PERF_7821_GREP_OPTS $pattern with $threads threads" "
>> +git -c grep.patternType=$engine -c 
>> grep.threads=$threads grep$GIT_PERF_7821_GREP_OPTS $pattern 
>> >'out.$engine.$threads' || :
>> +"
>> +done
>
> Same here, which means these two scripts share somewhat large body
> of text and makes me wonder if it is worth refactoring it to ease
> future updates to them.

Yeah, but I wanted to leave refactoring these for some other time.

Re: [PATCH 3/3] merge-recursive: add explanation for src_entry and dst_entry

[Johannes Schindelin]

Hi Elijah,

On Fri, 5 Jan 2018, Elijah Newren wrote:

> 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.

Makes sense.

Thanks,
Dscho

Re: [PATCH 1/3] Tighten and correct a few testcases for merging and cherry-picking

[Johannes Schindelin]

Hi Elijah,

On Fri, 5 Jan 2018, Elijah Newren wrote:

> 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.

Whoops. Sorry about that. Thanks for the patch, looks obviously good (I
haven't read 3/3 yet, but figure that the error message stems from the
changes introduced in that patch).

Ciao,
Dscho

Re: [PATCH 2/3] merge-recursive: fix logic ordering issue

[Johannes Schindelin]

Hi Elijah,

On Fri, 5 Jan 2018, Elijah Newren wrote:

> 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 cannot actually see how the *current* rename detection would result in
D/F conflicts, as it is really only about detecting the renames, the
conflicts should have happened earlier.

But I could imagine that your "detect directory renames" patch series
changes this behavior.

In any case, the reordering does not hurt.

Ciao,
Dscho

Re: Can't squash merge with merge.ff set to false

[Paul Smith]

On Fri, 2018-01-05 at 12:12 -0800, Bryan Turner wrote:
> On Fri, Jan 5, 2018 at 11:59 AM, Robert Dailey  
> wrote:
> > Not sure if this is intended or a bug, but with the following
> > configuration:
> > 
> > $ git config --global merge.ff false
> > 
> > I am not able to merge my topic branch into master with squash
> > option:
> > 
> > $ git checkout master
> > $ git merge --squash topic
> > fatal: You cannot combine --squash with --no-ff.
> > 
> > I'm not sure why a non-fast-forward merge would prevent a squash
> > merge, since by its very nature a squashed merge is not a fast
> > forward merge (or maybe it is if you only have one commit).

Hah!  I was just thinking of checking the latest Git RC I built
yesterday to see if this pet peeve of mine has been fixed yet.  I guess
not!

> The easiest way to move forward is probably to pass "--ff" on the
> command line to override the config, when you're using "--squash".

That's what we always have to do.  Very annoying; we use squash-merge
extensively but also want to require ff merge by default.

> As for why the two aren't allowed together, my assumption would be
> because if you're only squashing a single commit "--squash" and that
> commit is fast-forward from the target, a new commit is not created
> and instead the target branch is fast-forwarded. With "--no-ff", it's
> questionable what "--squash" should do in that case. Fast-forward
> anyway? Rewrite the commit simply to get new committer details and
> SHA-1?

If it only failed when you were squash-merging a single commit that was
also fast-forwardable, I guess that would be one thing.  But even if I
have multiple commits and I want to squash-merge them, which clearly is
a separate operation giving different results, I get this error.

I don't think Git should try to be clever here (if that's what it's
doing--I always assumed it was just a missing configuration case in the
error check).  If I asked for a squash-merge then Git should give me a
squash merge.

So in answer to your question, --squash should give me a squash merge
and the setting of --ff / --no-ff should be completely ignored, as it's
irrelevant.

My $0.02.

Re: [PATCH] rebase -p: fix quoting when calling `git merge`

[Johannes Schindelin]

Hi Junio,

On Fri, 5 Jan 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >  git-rebase--interactive.sh |  9 ++---
> >  t/t3418-rebase-continue.sh | 14 ++
> >  2 files changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index e3f5a0abf3c..085aa068cbb 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -392,9 +392,12 @@ pick_one_preserving_merges () {
> > new_parents=${new_parents# $first_parent}
> > merge_args="--no-log --no-ff"
> > if ! do_with_author output eval \
> > -   'git merge ${gpg_sign_opt:+"$gpg_sign_opt"} \
> > -   $allow_rerere_autoupdate $merge_args \
> > -   $strategy_args -m "$msg_content" $new_parents'
> > +   git merge ${gpg_sign_opt:+$(git rev-parse \
> > +   --sq-quote "$gpg_sign_opt")} \
> > +   $allow_rerere_autoupdate "$merge_args" \
> > +   "$strategy_args" \
> > +   -m $(git rev-parse --sq-quote "$msg_content") \
> > +   "$new_parents"
> 
> Makes sense.  I should have been more careful when reviewing
> db2b3b820e2.

Don't be so harsh on yourself. This bug was apparently not a big deal for
four years (which can feel like a *very* long time, can't it?).

Besides, the way code review happens in this project is not really
conducive to intense testing of the code: mere patch review will never be
as effective as patch review combined with hands-on testing of code paths
that may look like they could hide some unexpected bugs.

Ciao,
Dscho

Re: Git 2.15.0 on OSX 10.12.6: gui multi-select stage

[Johannes Schindelin]

Hi Matthew,

On Fri, 5 Jan 2018, Matthew Orres wrote:

> I'd be glad to give it a try - but am unfamiliar with how portable a
> manual build of Git can be used along side the version I have
> installed via Homebrew - do I just use full paths to reference the
> compiled executable from within my repository folder?

No. If you want to avoid installing Git, and instead want to run it
in-place after building it, you have to use --exec-path, like so:

$HOME/my-git-checkout/git --exec-path=$HOME/my-git-checkout gui

Ciao,
Johannes

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi,


On Fri, 5 Jan 2018, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Wed, Jan 03, 2018 at 02:42:51PM -0800, Isaac Shabtay wrote:
> >
> >> Indeed interesting... this one's for the books...
> >> Thanks for the patches. Any idea when these are going to make it to the
> >> official Git client builds? (specifically the Windows one)
> >
> > They haven't even been reviewed yet.

FWIW I went through them in that PR that Isaac opened (and which I updated
with Peff's patches, rebased), and I found nothing obviously wrong with
them. They seem to be straight-forward enough.

Just s/m(y temporarily removing its objects)/b\1/ and s/(call stat
directly)( the same scratch)/\1 using\2/

Ciao,
Dscho

Re: Can't squash merge with merge.ff set to false

[Bryan Turner]

On Fri, Jan 5, 2018 at 12:35 PM, Robert Dailey  wrote:
> On Fri, Jan 5, 2018 at 2:26 PM, Paul Smith  wrote:
>> On Fri, 2018-01-05 at 12:12 -0800, Bryan Turner wrote:
>>> On Fri, Jan 5, 2018 at 11:59 AM, Robert Dailey  
>>> wrote:
>>>
>>> As for why the two aren't allowed together, my assumption would be
>>> because if you're only squashing a single commit "--squash" and that
>>> commit is fast-forward from the target, a new commit is not created
>>> and instead the target branch is fast-forwarded. With "--no-ff", it's
>>> questionable what "--squash" should do in that case. Fast-forward
>>> anyway? Rewrite the commit simply to get new committer details and
>>> SHA-1?
>>
>> If it only failed when you were squash-merging a single commit that was
>> also fast-forwardable, I guess that would be one thing.  But even if I
>> have multiple commits and I want to squash-merge them, which clearly is
>> a separate operation giving different results, I get this error.

I think there's a reasonable argument that having the failure be
consistent is easier to reason about, and therefore provides a
"better" user experience (to some definition of "better" which all
people may not share in common).

If the failure was delayed until "git merge --squash" decided it
wanted to fast-forward, the failure might seem more arbitrary.

>>
>> I don't think Git should try to be clever here (if that's what it's
>> doing--I always assumed it was just a missing configuration case in the
>> error check).  If I asked for a squash-merge then Git should give me a
>> squash merge.
>>
>> So in answer to your question, --squash should give me a squash merge
>> and the setting of --ff / --no-ff should be completely ignored, as it's
>> irrelevant.
>>
>> My $0.02.
>
> Seems like --ff works, but is also misleading since in my case (more
> than one commit) I'm not doing a ff merge and there's no possibility
> of it.

"--ff" doesn't say "git merge" _must_ fast-forward ("--ff-only"); it
says that it _can_. At a general level with "--squash", that seems to
be exactly correct. A "--squash" merge can create a new commit, or it
can fast-forward an existing commit if the situation allows. Based on
that, passing "--ff" doesn't seem misleading to me.

> I think your idea of the 2 being distinctly separate makes
> sense. Basically, --squash takes precedence and if the mechanism to
> implement squash in certain scenarios (such as single commit) is
> fast-forward merge, then that decision is made for the user and is no
> longer something they can control.

The two _aren't_ distinctly separate, though. "git merge --squash
--ff-only" has very different semantics to "git merge --squash --ff",
in that it will only create a new squashed commit (or fast-forward a
single commit) if the incoming commit(s) are fast-forward from the
target. So there _is_ a setting for the fast-forward mode (given
"--ff", "--ff-only", and "--no-ff" are a tri-state switch, and
therefore comprise a single setting) that does impact squashing.

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 12:45:03PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Out of curiosity, did this change at some point? I thought the process
> > used to be to merge to maint, and then pick up topics in master by
> > merging maint to master.
> 
> If you look at "Sync with maint" merges made to 'master', you'd
> notice that most of them are only updating Documentation/RelNotes/*
> and otherwise no-effect merges, simply because when such an up-merge
> is made, everything in 'maint' is already in 'master' because topics
> are merged to the latter first.  Security fixes that go through
> embargoes are excempt for obvious reasons ;-)

OK, that makes sense. Pretty sure I did it wrong when I was interim
maintainer back in the day, then. :)

-Peff

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

[Johannes Schindelin]

Hi Ævar,

On Fri, 5 Jan 2018, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Jan 05 2018, Johannes Schindelin jotted:
> 
> > [...]
> >
> > In short: the Unix shell script t3070 manages to write what it thinks is a
> > file called 'foo*', but Git only sees 'foo'.
> >
> > I tried to address this problem with this patch:
> 
> ...I don't see any particular value in trying to do these full roundtrip
> tests on platforms like Windows. Perhaps we should just do these on a
> whitelist of POSIX systems for now, and leave expanding that list to
> some future step.

I don't think so. Windows is already handled as a second-class citizen, as
if nobody developed on it. As a consequence, only very few of the
gazillions of Windows developers... develop Git. We could worsify the
situation, of course, but why? Shouldn't we at least pretend to try the
opposite?

> > -- snip --
> > diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> > index f606f91acbb..51dcb675e7b 100755
> > --- a/t/t3070-wildmatch.sh
> > +++ b/t/t3070-wildmatch.sh
> > @@ -4,6 +4,14 @@ test_description='wildmatch tests'
> >
> >  . ./test-lib.sh
> >
> > +if test_have_prereq !MINGW && touch -- 'a*b' 2>/dev/null
> > +then
> > +   test_set_prereq FILENAMESWITHSTARS
> > +else
> > +   say 'Your filesystem does not allow stars in filenames.'
> > +fi
> > +rm -f 'a*b'
> > +
> >  create_test_file() {
> > file=$1
> >
> > @@ -28,6 +36,17 @@ create_test_file() {
> > */)
> > return 1
> > ;;
> > +   # On Windows, stars are not allowed in filenames. Git for Windows'
> > +   # Bash, however, is based on Cygwin which plays funny names with a
> > +   # private Unicode page to emulate stars in filenames. Meaning that
> > +   # the shell script will "succeed" to write the file, but Git will
> > +   # not see it. Nor any other, regular Windows process.
> > +   *\**|*\[*)
> > +   if ! test_have_prereq FILENAMESWITHSTARS
> > +   then
> > +   return 1
> > +   fi
> > +   ;;
> > # On Windows, \ in paths is silently converted to /, which
> > # would result in the "touch" below working, but the test
> > # itself failing. See 6fd1106aa4 ("t3700: Skip a test with
> > -- snap --
> >
> > This gets us further. But not by much:
> 
> Okey, that's very weird. So you can:
> 
> touch "./*"; echo $?
> 
> And it'll return 0 but then the file won't exist?

Almost. The file exists, but it won't have the name '*'. It will have as
name a Unicode character that is in a private page, not standardized by
the Unicode specification.

> How about this:
> 
> touch "./*" && test -e "./*"; echo $?

Would return 0. Why? Because *you are still in a Unix shell script, so the
Cygwin cuteness still applies*.

> The reason this latest version stopped creating files with "\" in them
> unless under BSLASHPSPEC is because Cygwin would silently translate it,
> so it would create the file but it would actually mean something the
> tests didn't expect.

I understand that. And I would wish that the test would be designed in a
more cross-platform-aware mindset.

> For anything else, such as stars not being allowed in filenames I was
> expecting that "touch" command to return an error, but if that's not the
> case maybe we need the "test -e" as well, unless I'm missing something
> here.

This is one of the many bad consequences of Git relying so much on Unix
shell scripting. Despite what many, many, many Git developers think: shell
scripting is not portable.

Cygwin does a good job of pretending that it is, and MSYS2 exacerbates
that notion, but it comes back to haunt you right here and right now. The
`touch` invocation will *report success*, but it will have done something
different than you wanted. It's like the Thinking: Fast and Slow.

> > fatal: Invalid path '\[ab]': No such file or directory
> >
> > You see, any path starting with a backslash *is* an absolute path on
> > Windows. It is relative to the current drive.
> 
> Right, which I was trying to avoid by not actually creating \[ab], but
> "./\[ab]", is that the same filename on Windows?

Whoops. I managed to copy-paste the *wrong* command's error message. Sorry
about that. The correct one:

$ git --glob-pathspecs ls-files -z -- '\[ab]'
fatal: \[ab]: '\[ab]' is outside repository

Note how it is Git reporting that you asked for a path that is outside?
That's because it thinks you are referring to C:\[ab] (if your current
directory is on the C: drive).

And it would be correct to complain on Windows: the `\[ab]` parameter
refers to an absolute path.

> > This affects *quite* a few of the test cases you added.
> >
> > And even if I just comment all of those out, I run into the next problem
> > where you try to create a file whose name consists of a single space,
> > which is also illegal on Windows.
> 
> Okey, but ditto above about touch not catching it, does:
> 
> touch "./ "; echo $?
> 
> Not return an error? Then how about:
> 
> touch "./ " && test -e "./ 

Re: Bug report: git clone with dest

[Junio C Hamano]

Jeff King  writes:

> Out of curiosity, did this change at some point? I thought the process
> used to be to merge to maint, and then pick up topics in master by
> merging maint to master.

If you look at "Sync with maint" merges made to 'master', you'd
notice that most of them are only updating Documentation/RelNotes/*
and otherwise no-effect merges, simply because when such an up-merge
is made, everything in 'maint' is already in 'master' because topics
are merged to the latter first.  Security fixes that go through
embargoes are excempt for obvious reasons ;-)

I do not recall how it was before 2012.

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 09:22:07PM +0100, Johannes Schindelin wrote:

> On Fri, 5 Jan 2018, Isaac Shabtay wrote:
> 
> > Done: https://github.com/git-for-windows/git/pull/1421
> > 
> > I added credit to Jeff in the PR's description.
> 
> Sadly, the PR's description won't make it into the commit history, and the
> authorship really should have been retained.
> 
> I found Peff's topic branch in his fork and force-pushed, to demonstrate
> what I wanted to have. Currently the test suite is running (I test 64-bit
> builds of the three major platforms Windows, macOS and Linux), and once
> that is done and passed, I will merge the Pull Request.

I think the discussion has ended at "don't do anything else", but note
that Junio and I were musing on whether to update the series around the
dir_exists() function. Which would then create headaches for you later
when you try to merge a subtly-different series that makes it upstream.

Like I said, I think we've resolved not to do anything, but I wanted to
point out a potential pitfall with this kind of "pick up a topic early"
strategy (I'm intimately familiar with this pitfall because I do it all
the time for the fork we run on our servers at GitHub).

-Peff

Re: Bug report: git clone with dest

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Isaac,
>
> On Fri, 5 Jan 2018, Isaac Shabtay wrote:
>
>> Done: https://github.com/git-for-windows/git/pull/1421
>> 
>> I added credit to Jeff in the PR's description.
>
> Sadly, the PR's description won't make it into the commit history, and the
> authorship really should have been retained.
>
> I found Peff's topic branch in his fork and force-pushed, to demonstrate
> what I wanted to have. Currently the test suite is running (I test 64-bit
> builds of the three major platforms Windows, macOS and Linux), and once
> that is done and passed, I will merge the Pull Request.
>
>> Note that I tried compiling master, but failed due to a reason
>> unrelated to this patch:
>> 
>> builtin/checkout.c:24:10: fatal error: fscache.h: No such file or directory
>
> That was an oversight in a previously-merged Pull Request. I have fixed
> that locally and will soon push it out onto `master`.
>
> Ciao,
> Johannes

FWIW, I do not mind including this as part of -rc2 if not -rc1 for
the upcoming release.

Re: Can't squash merge with merge.ff set to false

[Robert Dailey]

On Fri, Jan 5, 2018 at 2:26 PM, Paul Smith  wrote:
> On Fri, 2018-01-05 at 12:12 -0800, Bryan Turner wrote:
>> On Fri, Jan 5, 2018 at 11:59 AM, Robert Dailey  
>> wrote:
>> > Not sure if this is intended or a bug, but with the following
>> > configuration:
>> >
>> > $ git config --global merge.ff false
>> >
>> > I am not able to merge my topic branch into master with squash
>> > option:
>> >
>> > $ git checkout master
>> > $ git merge --squash topic
>> > fatal: You cannot combine --squash with --no-ff.
>> >
>> > I'm not sure why a non-fast-forward merge would prevent a squash
>> > merge, since by its very nature a squashed merge is not a fast
>> > forward merge (or maybe it is if you only have one commit).
>
> Hah!  I was just thinking of checking the latest Git RC I built
> yesterday to see if this pet peeve of mine has been fixed yet.  I guess
> not!
>
>> The easiest way to move forward is probably to pass "--ff" on the
>> command line to override the config, when you're using "--squash".
>
> That's what we always have to do.  Very annoying; we use squash-merge
> extensively but also want to require ff merge by default.
>
>> As for why the two aren't allowed together, my assumption would be
>> because if you're only squashing a single commit "--squash" and that
>> commit is fast-forward from the target, a new commit is not created
>> and instead the target branch is fast-forwarded. With "--no-ff", it's
>> questionable what "--squash" should do in that case. Fast-forward
>> anyway? Rewrite the commit simply to get new committer details and
>> SHA-1?
>
> If it only failed when you were squash-merging a single commit that was
> also fast-forwardable, I guess that would be one thing.  But even if I
> have multiple commits and I want to squash-merge them, which clearly is
> a separate operation giving different results, I get this error.
>
> I don't think Git should try to be clever here (if that's what it's
> doing--I always assumed it was just a missing configuration case in the
> error check).  If I asked for a squash-merge then Git should give me a
> squash merge.
>
> So in answer to your question, --squash should give me a squash merge
> and the setting of --ff / --no-ff should be completely ignored, as it's
> irrelevant.
>
> My $0.02.

Seems like --ff works, but is also misleading since in my case (more
than one commit) I'm not doing a ff merge and there's no possibility
of it. I think your idea of the 2 being distinctly separate makes
sense. Basically, --squash takes precedence and if the mechanism to
implement squash in certain scenarios (such as single commit) is
fast-forward merge, then that decision is made for the user and is no
longer something they can control.

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

[Elijah Newren]

On Fri, Jan 5, 2018 at 12:31 PM, Thomas Gummerer  wrote:
> On 01/04, Elijah Newren wrote:
>> On Wed, Jan 3, 2018 at 5:52 PM, SZEDER Gábor  wrote:
>>
>> >> + test $(git rev-parse :0:y/b) = $(git rev-parse O:z/b) &&
>> >
>> > There is a test helper for that :)
>> >
>> >   test_cmp_rev :0:y/b O:z/b
>> >
>> > Note, that this is not only a matter of useful output on failure, but
>> > also that of correctness and robustness.
>>
>>
>> Cool, good to know.  Is there any reason test_cmp_rev is not
>> documented in t/README?
>
> It is documented since a few weeks, I added the docs in 5a0526264b
> ("t/README: document test_cmp_rev", 2017-11-26).  Though as people
> mention they just look in 't/test-lib-functions.sh' anyway, maybe the
> docs should live there instead of in the README in the first place?
>

Nice!  If we don't end up moving them, it might be nice to add a note
to point people towards t/test-lib-functions.sh from t/README,
mentioning that it might have other functions available as well.

[PATCHv6 03/31] directory rename detection: testcases to avoid taking detection too far

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index f36493289..239819f2d 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -582,4 +582,157 @@ test_expect_success '2b-check: Directory split into two 
on one side, with equal
 #   messages are handled correctly.
 ###
 
+
+###
+# SECTION 3: Path in question is the source path for some rename already
+#
+# Combining cases from Section 1 and trying to handle them could lead to
+# directory renaming detection being over-applied.  So, this section
+# provides some good testcases to check that the implementation doesn't go
+# too far.
+###
+
+# Testcase 3a, Avoid implicit rename if involved as source on other side
+#   (Related to testcases 1c and 1f)
+#   Commit O: z/{b,c,d}
+#   Commit A: z/{b,c,d} (no change)
+#   Commit B: y/{b,c}, x/d
+#   Expected: y/{b,c}, x/d
+test_expect_success '3a-setup: Avoid implicit rename if involved as source on 
other side' '
+   test_create_repo 3a &&
+   (
+   cd 3a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d >z/d &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   test_tick &&
+   git commit --allow-empty -m "A" &&
+
+   git checkout B &&
+   mkdir y &&
+   mkdir x &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   git mv z/d x/ &&
+   rmdir z &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '3a-check: Avoid implicit rename if involved as source on 
other side' '
+   (
+   cd 3a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:x/d &&
+   git rev-parse >expect \
+   O:z/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_i18ngrep CONFLICT.*rename/rename.*z/d.*x/d.*w/d out &&
+   test_i18ngrep ! CONFLICT.*rename/rename.*y/d &&
+
+   git ls-files -s >out &&
+   test_line_count = 5 out &&
+   git ls-files -u >out &&
+   test_line_count = 3 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   :0:y/b :0:y/c 

[PATCH] upload-pack: fix some sparse warnings

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Brandon,

If you need to re-roll your 'bw/protocol-v2' branch, could you please
squash this (or something like it) into the relevant patches. The first
hunk would go in commit 6ec1105192, "upload-pack: convert to a builtin",
2018-01-02), whereas the second hunk would go to commit b3f3749a24,
"upload-pack: factor out processing lines", 2018-01-02).

The sparse warnings were:

   SP upload-pack.c
   upload-pack.c:783:43: error: incompatible types for operation (<=)
   upload-pack.c:783:43:left side has type int *depth
   upload-pack.c:783:43:right side has type int
   upload-pack.c:783:43: error: incorrect type in conditional
   upload-pack.c:783:43:got bad type
   upload-pack.c:1389:5: warning: symbol 'cmd_upload_pack' was not declared. 
Should it be static?

[Note that the line numbers are off-by-one.]

I note, in passing, that strtol() returns a 'long int' but *depth is
an 'int' (yes, the original code was like that too ;-) ).

Should cmd_upload_pack(), along with the #include "builtin.h", be moved
to builtin/upload-pack.c?

Also, I note that packet_read_with_status(), introduced in commit 4570421c3,
is not called outside of pkt-line.c; does this symbol need to be extern?

Thanks!

ATB,
Ramsay Jones


 upload-pack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 8002f1f20..6271245e2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "builtin.h"
 #include "config.h"
 #include "refs.h"
 #include "pkt-line.h"
@@ -780,7 +781,7 @@ static int process_deepen(const char *line, int *depth)
if (skip_prefix(line, "deepen ", )) {
char *end = NULL;
*depth = strtol(arg, , 0);
-   if (!end || *end || depth <= 0)
+   if (!end || *end || *depth <= 0)
die("Invalid deepen: %s", line);
return 1;
}
-- 
2.15.0

[PATCHv6 02/31] directory rename detection: directory splitting testcases

[Elijah Newren]

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

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

[PATCHv6 13/31] 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.14.2

[PATCHv6 11/31] directory rename detection: tests for handling overwriting dirty files

[Elijah Newren]

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

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

[PATCHv6 04/31] directory rename detection: partially renamed directory testcase/discussion

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 239819f2d..c61ecb9b7 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -735,4 +735,111 @@ 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 &&
+
+   git ls-files -s >out &&
+   test_line_count = 5 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/d HEAD:z/e HEAD:z/f &&
+   git rev-parse >expect \
+   O:z/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.14.2

[PATCHv6 12/31] 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 :
+   

Re: [PATCH] rebase -p: fix quoting when calling `git merge`

[Junio C Hamano]

Johannes Schindelin  writes:

>  git-rebase--interactive.sh |  9 ++---
>  t/t3418-rebase-continue.sh | 14 ++
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index e3f5a0abf3c..085aa068cbb 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -392,9 +392,12 @@ pick_one_preserving_merges () {
>   new_parents=${new_parents# $first_parent}
>   merge_args="--no-log --no-ff"
>   if ! do_with_author output eval \
> - 'git merge ${gpg_sign_opt:+"$gpg_sign_opt"} \
> - $allow_rerere_autoupdate $merge_args \
> - $strategy_args -m "$msg_content" $new_parents'
> + git merge ${gpg_sign_opt:+$(git rev-parse \
> + --sq-quote "$gpg_sign_opt")} \
> + $allow_rerere_autoupdate "$merge_args" \
> + "$strategy_args" \
> + -m $(git rev-parse --sq-quote "$msg_content") \
> + "$new_parents"

Makes sense.  I should have been more careful when reviewing
db2b3b820e2.

Thanks.

[PATCHv6 16/31] 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,
+

[PATCHv6 09/31] 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 | 549 +++-
 1 file changed, 548 insertions(+), 1 deletion(-)

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

[PATCHv6 14/31] 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.14.2

[PATCHv6 06/31] directory rename detection: testcases checking which side did the rename

[Elijah Newren]

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

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

[PATCHv6 10/31] directory rename detection: tests for handling overwriting untracked files

[Elijah Newren]

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

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

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

[Thomas Gummerer]

On 01/04, Elijah Newren wrote:
> On Wed, Jan 3, 2018 at 5:52 PM, SZEDER Gábor  wrote:
> 
> >> + test $(git rev-parse :0:y/b) = $(git rev-parse O:z/b) &&
> >
> > There is a test helper for that :)
> >
> >   test_cmp_rev :0:y/b O:z/b
> >
> > Note, that this is not only a matter of useful output on failure, but
> > also that of correctness and robustness.
> 
> 
> Cool, good to know.  Is there any reason test_cmp_rev is not
> documented in t/README?

It is documented since a few weeks, I added the docs in 5a0526264b
("t/README: document test_cmp_rev", 2017-11-26).  Though as people
mention they just look in 't/test-lib-functions.sh' anyway, maybe the
docs should live there instead of in the README in the first place?

> [...]

[PATCHv6 21/31] 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.14.2

[PATCHv6 20/31] 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);
+   

[PATCHv6 27/31] 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, 

[PATCHv6 22/31] 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;
+ 

[PATCHv6 31/31] 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 99dca4ff0..b36551662 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3854,7 +3854,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.14.2

[PATCHv6 23/31] 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 

[PATCHv6 18/31] 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.14.2

[PATCHv6 25/31] 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 

[PATCHv6 17/31] 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.14.2

[PATCHv6 26/31] 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 aa8f79638..c1082d625 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2968,7 +2968,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 &&
 
@@ -3046,7 +3046,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 &&
 
@@ -3121,7 +3121,7 @@ test_expect_success '10d-setup: Delete untracked with dir 
rename/rename(2to1)' '
)
 '
 
-test_expect_failure '10d-check: Delete 

[PATCHv6 29/31] 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 | 288 
 1 file changed, 288 insertions(+)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 003a65b1b..c684f34b6 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -159,6 +159,7 @@ test_expect_success '1b-check: Merge a directory with 
another' '
 # Testcase 1c, Transitive renaming
 #   (Related to testcases 3a and 6d -- when should a transitive rename apply?)
 #   (Related to testcases 9c and 9d -- can transitivity repeat?)
+#   (Related to testcase 12b -- joint-transitivity?)
 #   Commit O: z/{b,c},   x/d
 #   Commit A: y/{b,c},   x/d
 #   Commit B: z/{b,c,d}
@@ -2847,6 +2848,68 @@ test_expect_failure '9g-check: Renamed directory that 
only contained immediate s
)
 '
 
+# Testcase 9h, Avoid implicit rename if involved as source on other side
+#   (Extremely closely related to testcase 3a)
+#   Commit O: z/{b,c,d_1}
+#   Commit A: z/{b,c,d_2}
+#   Commit B: y/{b,c}, x/d_1
+#   Expected: y/{b,c}, x/d_2
+#   NOTE: If we applied the z/ -> y/ rename to z/d, then we'd end up with
+# a rename/rename(1to2) conflict (z/d -> y/d vs. x/d)
+test_expect_success '9h-setup: Avoid dir rename on merely modified path' '
+   test_create_repo 9h &&
+   (
+   cd 9h &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   printf "1\n2\n3\n4\n5\n6\n7\n8\nd\n" >z/d &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   test_tick &&
+   echo more >>z/d &&
+   git add z/d &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   mkdir y &&
+   mkdir x &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   git mv z/d x/ &&
+   rmdir z &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '9h-check: Avoid dir rename on merely modified path' '
+   (
+   cd 9h &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 3 out &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:x/d &&
+   git rev-parse >expect \
+   O:z/b O:z/c A:z/d &&
+   test_cmp expect actual
+   )
+'
+
 ###
 # Rules suggested by section 9:
 #
@@ -3680,4 +3743,229 @@ 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 &&
+ 

[PATCHv6 24/31] 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.14.2

[PATCHv6 19/31] 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) {
+ 

[PATCHv6 15/31] 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.14.2

[PATCHv6 01/31] directory rename detection: basic testcases

[Elijah Newren]

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

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

[PATCHv6 28/31] 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 dd0a037ec..003a65b1b 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3346,7 +3346,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 &&
 
@@ -3488,7 +3488,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 &&
 
@@ -3567,7 +3567,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 &&
 
@@ -3643,7 +3643,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.14.2

[PATCHv6 08/31] directory rename detection: testcases exploring possibly suboptimal merges

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 38ca791e9..470c9a79f 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -1904,4 +1904,408 @@ test_expect_failure '7e-check: transitive rename in 
rename/delete AND dirs in th
)
 '
 
+###
+# SECTION 8: Suboptimal merges
+#
+# As alluded to in the last section, the ruleset we have built up for
+# detecting directory renames unfortunately has some special cases where it
+# results in slightly suboptimal or non-intuitive behavior.  This section
+# explores these cases.
+#
+# To be fair, we already had non-intuitive or suboptimal behavior for most
+# of these cases in git before introducing implicit directory rename
+# detection, but it'd be nice if there was a modified ruleset out there
+# that handled these cases a bit better.
+###
+
+# Testcase 8a, Dual-directory rename, one into the others' way
+#   Commit O. x/{a,b},   y/{c,d}
+#   Commit A. x/{a,b,e}, y/{c,d,f}
+#   Commit B. y/{a,b},   z/{c,d}
+#
+# Possible Resolutions:
+#   w/o dir-rename detection: y/{a,b,f},   z/{c,d},   x/e
+#   Currently expected:   y/{a,b,e,f}, z/{c,d}
+#   Optimal:  y/{a,b,e},   z/{c,d,f}
+#
+# Note: Both x and y got renamed and it'd be nice to detect both, and we do
+# better with directory rename detection than git did without, but the
+# simple rule from section 5 prevents me from handling this as optimally as
+# we potentially could.
+
+test_expect_success '8a-setup: Dual-directory rename, one into the others way' 
'
+   test_create_repo 8a &&
+   (
+   cd 8a &&
+
+   mkdir x &&
+   mkdir y &&
+   echo a >x/a &&
+   echo b >x/b &&
+   echo c >y/c &&
+   echo d >y/d &&
+   git add x y &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   echo e >x/e &&
+   echo f >y/f &&
+   git add x/e y/f &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   git mv y z &&
+   git mv x y &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_failure '8a-check: Dual-directory rename, one into the others way' 
'
+   (
+   cd 8a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   git ls-files -s >out &&
+   test_line_count = 6 out &&
+   git ls-files -u >out &&
+   test_line_count = 0 out &&
+   git ls-files -o >out &&
+   test_line_count = 1 out &&
+
+   git rev-parse >actual \
+   HEAD:y/a HEAD:y/b HEAD:y/e HEAD:y/f HEAD:z/c HEAD:z/d &&
+   git rev-parse >expect \
+   O:x/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 &&
+   

[PATCHv6 00/31] 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/
  https://public-inbox.org/git/20171228041352.27880-1-new...@gmail.com/
  
Changes since v5 (full tbdiff follows below):
  * Split the first three (independent) patches off into a separate series.[1]
Semantically, they are separate and could go in either order, but built
with this one depending on the other series to avoid context region
conflicts.

[1] https://public-inbox.org/git/20180105202001.24218-1-new...@gmail.com/

  * Testsuite:

* Replaced plain 'test' with functions that'd provide more
  feedback on failure; e.g. test_line_count, test_path_is_missing,
  test_path_is_file (Thanks to SZEDER Gábor for the suggestions)
  
* Backslash escaped '^' character when found in middle of string
  being passed to test_i18ngrep (Thanks to Johannes Sixt), and
  avoided some shell globbing that somehow tripped up some old
  windows build

Elijah Newren (31):
  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   | 1212 ++-
 merge-recursive.h   |   17 +
 strbuf.c|   16 +
 strbuf.h|   16 +
 t/t3501-revert-cherry-pick.sh   |2 +-
 t/t6043-merge-rename-directories.sh | 3966 +++
 t/t7607-merge-overwrite.sh  |2 +-
 unpack-trees.c  |4 +-
 unpack-trees.h  |4 +
 9 files changed, 5124 insertions(+), 115 deletions(-)
 create mode 100755 t/t6043-merge-rename-directories.sh

 1: d95d5fffe < --:  --- Tighten and correct a few testcases for merging 
and cherry-picking
 2: b177dccc2 < --:  --- merge-recursive: fix logic ordering issue
 3: fe3de710e < --:  --- merge-recursive: add explanation for src_entry and 
dst_entry
 4: 19b82b795 !  1: 2fd0812b7 directory rename detection: basic testcases
@@ -88,18 +88,23 @@
 +
 +  git merge -s recursive B^0 &&
 +
-+  test 4 -eq $(git ls-files -s | wc -l) &&
++  git ls-files -s >out &&
++  test_line_count = 4 out &&
 +
 +  git rev-parse >actual \
 +  HEAD:y/b HEAD:y/c HEAD:y/d HEAD:y/e/f &&
 +  git rev-parse >expect \
 +  O:z/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) &&
++
++

[PATCHv6 07/31] directory rename detection: more involved edge/corner testcases

[Elijah Newren]

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

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

[PATCHv6 05/31] directory rename detection: files/directories in the way of some renames

[Elijah Newren]

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

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

[PATCHv6 30/31] 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 c684f34b6..99dca4ff0 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2070,18 +2070,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 &&
@@ -2114,32 +2119,32 @@ test_expect_success '8c-setup: rename+modify/delete' '
)
 '
 
-test_expect_success '8c-check: rename+modify/delete' '
+test_expect_success '8c-check: modify/delete or rename+modify/delete' '
(
cd 8c &&
 
git checkout A^0 &&
 
test_must_fail git merge -s recursive B^0 >out &&
-   test_i18ngrep "CONFLICT (rename/delete).* z/d.*y/d" out &&
+   test_i18ngrep "CONFLICT (modify/delete).* z/d" out &&
 
git ls-files -s >out &&
-   test_line_count = 4 out &&
+   test_line_count = 5 out &&
git ls-files -u >out &&
-   test_line_count = 1 out &&
+   test_line_count = 2 out &&
git ls-files -o >out &&
test_line_count = 1 out &&
 
git rev-parse >actual \
-   :0:y/b :0:y/c :0:y/e :3:y/d &&
+   :0:y/b :0:y/c :0:y/e :1:z/d :3:z/d &&
git rev-parse >expect \
-   O:z/b O:z/c B:z/e B:z/d &&
+   O:z/b O:z/c B:z/e O:z/d B:z/d &&
test_cmp expect actual &&
 
-   test_must_fail git rev-parse :1:y/d &&
-   test_must_fail git rev-parse :2:y/d &&
-   git ls-files -s y/d | grep ^100755 &&
-   test_path_is_file y/d
+   test_must_fail git rev-parse :2:z/d &&
+   git ls-files -s z/d | grep ^100755 &&
+   test_path_is_file z/d &&
+   test_path_is_missing y/d
)
 '
 
@@ -2153,16 +2158,6 @@ test_expect_success '8c-check: rename+modify/delete' '
 #
 #   

Re: Git 2.15.0 on OSX 10.12.6: gui multi-select stage

[Matthew Orres]

I'd be glad to give it a try - but am unfamiliar with how portable a
manual build of Git can be used along side the version I have
installed via Homebrew - do I just use full paths to reference the
compiled executable from within my repository folder?

Thanks!

On Wed, Nov 1, 2017 at 5:58 PM, Johannes Schindelin
 wrote:
> Hi Matthew,
>
> On Wed, 1 Nov 2017, Matthew Orres wrote:
>
>> Using 2.15.0 on OSX 10.12.6, when I open git gui, and then attempt to
>> stage multiple files as such:
>>
>> * Left click first file
>> * CMD+Shift+Click last file to multi-select all files
>> * CMT+T (shortcut for Stage to Commit)
>>
>> Only the file I selected with the first Left Click is staged and my
>> selection disappears.
>>
>> I'd be happy to provide more system-level info if there's issues with
>> reproducing this on other machines.
>
> Maybe you'll be also happy to test things?
>
> I believe that we carry a fix for this in Git for Windows:
> https://github.com/git-for-windows/git/commit/3a5640fd3f0aa57edecc8dab455a97c5a15e6626
>
> The easiest way to test this would be to simply build Git from the
> `master` branch of https://github.com/git-for-windows/git (I try to keep
> it building and passing the test suite at all times not only on Windows,
> but also on Linux, it should also work on macOS).
>
> Ciao,
> Johannes
>
> P.S.: If you test this, and can confirm that it fixes your issue, I'll get
> this patch submitted properly to the Git mailing list (sadly, it seems
> that the https://github.com/patthoyts/git-gui project is sleeping beauty
> mode for a while now, otherwise I would add PRs there).

[PATCH 2/3] 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.

Reviewed-By: Stefan Beller 
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.14.2

[PATCH 0/3] Some small fixes to merge-recursive and tests

[Elijah Newren]

These three patches were the first three from
en/rename-directory-detection[1], but as Stefan suggested[2], I'm submitting
them separately as they are independent fixes.

Change from PATCHv5-rename-directory-detection:
  * Split from the rest of the series.
  * Added Stefan's Reviewed-By.

[1] https://public-inbox.org/git/20171228041352.27880-1-new...@gmail.com/
[2] 
https://public-inbox.org/git/CAGZ79kapuEKLO4RUUPVS6_-aeBERDhjpBAtmK=gyct8gak2...@mail.gmail.com/

Elijah Newren (3):
  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

 merge-recursive.c | 21 -
 t/t3501-revert-cherry-pick.sh |  7 +--
 t/t7607-merge-overwrite.sh|  5 -
 3 files changed, 29 insertions(+), 4 deletions(-)

-- 
2.14.2

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi Isaac,

On Fri, 5 Jan 2018, Isaac Shabtay wrote:

> Done: https://github.com/git-for-windows/git/pull/1421
> 
> I added credit to Jeff in the PR's description.

Sadly, the PR's description won't make it into the commit history, and the
authorship really should have been retained.

I found Peff's topic branch in his fork and force-pushed, to demonstrate
what I wanted to have. Currently the test suite is running (I test 64-bit
builds of the three major platforms Windows, macOS and Linux), and once
that is done and passed, I will merge the Pull Request.

> Note that I tried compiling master, but failed due to a reason
> unrelated to this patch:
> 
> builtin/checkout.c:24:10: fatal error: fscache.h: No such file or directory

That was an oversight in a previously-merged Pull Request. I have fixed
that locally and will soon push it out onto `master`.

Ciao,
Johannes

[PATCH 1/3] 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.

Reviewed-By: Stefan Beller 
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..9c422bcd7 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_path_is_file other.c~HEAD &&
+   test $(git hash-object other.c~HEAD) = $(git rev-parse c1a:c1.c) &&
test_cmp important other.c
 '
 
-- 
2.14.2

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

Reviewed-By: Stefan Beller 
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.14.2

Re: [PATCH] send-email: add test for Linux's get_maintainer.pl

[Junio C Hamano]

Matthieu Moy  writes:

> Alex Bennée  writes:
>
>> I think you need to apply Eric's suggestions from:
>>
>>   From: Eric Sunshine 
>>   Date: Sat, 18 Nov 2017 21:54:46 -0500
>>   Message-ID: 
>> 
>
> Indeed. I'm squashing this into the patch:
>
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index f126177..d13d8c3 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -173,8 +173,7 @@ test_expect_success $PREREQ 'cc trailer with various 
> syntax' '
>  '
>  
>  test_expect_success $PREREQ 'setup fake get_maintainer.pl script for cc 
> trailer' "
> - cat >expected-cc-script.sh <<-EOF &&
> - #!/bin/sh
> + write_script expected-cc-script.sh <<-EOF &&
>   echo 'One Person  (supporter:THIS (FOO/bar))'
>   echo 'Two Person  (maintainer:THIS THING)'
>   echo 'Third List  (moderated list:THIS THING 
> (FOO/bar))'

Please do not forget to lose "chmod +x", which becomes unneeded when
you use write_script.

> @@ -186,7 +185,6 @@ test_expect_success $PREREQ 'setup fake get_maintainer.pl 
> script for cc trailer'
>  "
>  
>  test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '
> - test_commit cc-trailer-getmaint &&
>   clean_fake_sendmail &&
>   git send-email -1 --to=recipi...@example.com \
>   --cc-cmd="./expected-cc-script.sh" \

Re: Can't squash merge with merge.ff set to false

[Bryan Turner]

On Fri, Jan 5, 2018 at 11:59 AM, Robert Dailey  wrote:
> Not sure if this is intended or a bug, but with the following configuration:
>
> $ git config --global merge.ff false
>
> I am not able to merge my topic branch into master with squash option:
>
> $ git checkout master
> $ git merge --squash topic
> fatal: You cannot combine --squash with --no-ff.
>
> I'm not sure why a non-fast-forward merge would prevent a squash
> merge, since by its very nature a squashed merge is not a fast forward
> merge (or maybe it is if you only have one commit).
>
> Is there an issue here? I like fast forward merges to be off by
> default, since I want to control when they happen. Most of my merges
> do not use --squash, so I'm catering to the common case.
>
> Need advice on how to get past this issue. Thanks in advance.

The easiest way to move forward is probably to pass "--ff" on the
command line to override the config, when you're using "--squash".

As for why the two aren't allowed together, my assumption would be
because if you're only squashing a single commit "--squash" and that
commit is fast-forward from the target, a new commit is not created
and instead the target branch is fast-forwarded. With "--no-ff", it's
questionable what "--squash" should do in that case. Fast-forward
anyway? Rewrite the commit simply to get new committer details and
SHA-1?

Hope this helps!
Bryan Turner

Re: Bring together merge and rebase

[Junio C Hamano]

Martin Fick  writes:

> These scenarios seem to come up most for me at Gerrit hack-
> a-thons where we collaborate a lot in short time spans on 
> changes.  We (the Gerrit maintainers) too have wanted and 
> sometimes discussed ways to track the relation of "amended" 
> commits (which is generally what Gerrit patchsets are).  We 
> also concluded that some sort of parent commit pointer was 
> needed, although parent is somewhat the wrong term since 
> that already means something in git.  Rather, maybe some 
> "predecessor" type of term would be better, maybe 
> "antecedent", but "amended-commit" pointer might be best?

In general, I agree that you would want richer set of "relationship"
than mere "predecessor" or "related", but I do not think "amended"
is sufficient.  I certainly do not think a "pointer" embedded in a
commit object is a good idea, either (a new commit object header is
out of question, but I doubt it is a good idea to make a pointer
back to an existing commit as a part of the log message).

You may used to have a set of n-patches A1, A2, ..., An, that turned
into m-patches X1, X2, ..., Xm, after refactoring.  During the work,
it may turned out that some things the original tried to do are not
sensible and dropped, while some other things are added in the final.
series.  

When n==m==1, "amended" pointer from X1 to A1 may allow you to
answer "Is this the first attempt?  If this is refined, what did the
earlier one look like?" when given X1, but you would also want to
answer a related question "This was a good start, but did the effort
result in a refined patch, and if so what is it?" when given A1, and
"amended" pointer won't help at all.  Needless to say, the "pointer"
approach breaks down when !(n==m==1).

[PATCHv4 4/4] 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 
Signed-off-by: Junio C Hamano 
---
 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 2967704317..47ddc9b273 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1657,7 +1657,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.16.0.rc0.223.g4a4ac83678-goog

[PATCHv4 3/4] 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 
Signed-off-by: Junio C Hamano 
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..96c3327f19 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 (o->update && S_ISGITLINK(old->ce_mode) &&
+   should_update_submodules() && !verify_uptodate(old, o))
+   update |= CE_UPDATE;
add_entry(o, old, update, 0);
return 0;
}
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCHv4 2/4] 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 
Signed-off-by: Junio C Hamano 
---
 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.16.0.rc0.223.g4a4ac83678-goog

[PATCHv4 1/4] 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 
Signed-off-by: Junio C Hamano 
---
 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.16.0.rc0.223.g4a4ac83678-goog

[PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes

[Stefan Beller]

v4:
* dropped the patch "t/helper/test-lazy-name-hash: fix compilation"
  (that snuck into v3 by accident, it is already present at
   remotes/origin/jh/memihash-opt, but I am carrying it locally in
   most branches currently.)
* Junio pointed out the micro-optimisation below, both in terms of readability
  as well as performance

diff --git a/unpack-trees.c b/unpack-trees.c
index 7657d6ecdd..96c3327f19 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,8 +2139,8 @@ 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() &&
-   o->update && !verify_uptodate(old, o))
+   if (o->update && S_ISGITLINK(old->ce_mode) &&
+   should_update_submodules() && !verify_uptodate(old, o))
update |= CE_UPDATE;
add_entry(o, old, update, 0);
return 0;

v3:
Thanks Junio for review of this series!
The only change in this version of the series is

--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
update |= CE_UPDATE;
}
if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
-   !verify_uptodate(old, o))
+   o->update && !verify_uptodate(old, o))
update |= CE_UPDATE;
add_entry(o, old, update, 0);


v2:
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 (4):
  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/lib-submodule-update.sh | 19 +--
 unpack-trees.c|  3 +++
 3 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: Apparent bug in 'git stash push ' loses untracked files

[Thomas Gummerer]

On 12/18, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > Ah interesting, what you have below looks good to me indeed, it
> > matches what I'd expect it to do and fixes the bug that was reported.
> > Thanks! 
> >
> > I've taken the liberty to take what you have below and turned into a
> > proper patch, giving you authorship of it, as you wrote the code for
> > the fix.  Hope that's the appropriate credit for you for this patch.
> 
> Not so fast.
> 
> I know why the updated code works like "--hard -- ", but I
> do not quite get what the original was trying to do and how it is
> different.  Even with your proposed log message, which describes a
> symptom (i.e. "untracked files ... be deleted"), it is unclear why
> this deletion was happening in the first place.  Specifically, it is
> unclear why that "clean --force -q -d" was in there, and are we
> breaking other cases by rewriting this codepath without it?

As you hint at below, the intention was to get rid of the files that
were unstaged by 'git reset', but didn't realize that it would also
delete files that were already untracked before the 'git reset' and
thus not in the stash.

It behaves fine if the pathspec matches a filename exactly, which is
why it worked in all my tests, but is obviously broken in the case
where the pathspec matches untracked files that are around.

I completely failed to realize this, and we wouldn't break any
intended usecases with the change.  I'll update the commit message to
include this information. 

> In any case, instead of the 
> 
>   ls-files -z -- "$@" | checkout-index -z --force --stdin
>   diff-index -p HEAD -- "$@" | apply --index -R
> 
> sequence, a shorter variant that should work is
> 
>   add -u -- "$@"
>   diff-index -p --cached HEAD -- "$@" | apply --index -R
> 
> Both of these share the same idea.  
> 
>  - The first step is to match what is in the index and what is in
>the working tree (i.e. make "diff-files" silent).  The version
>you have does so by checking out what is in the index to the
>working tree.  The shorter one goes the other way and updates the
>index with what is in the working tree.
> 
>  - Once that is done, we ask diff-index what got changed since the
>HEAD in the working tree (or in the index in the updated
>one---after the first step that makes the two match, comparing
>with the working tree and comparing with the index should result
>in the same diff; I have a suspicion that "--cached" is faster,
>but we need to benchmark to pick), and ask apply to get rid of
>all these changes, which includes removal of added files, and
>resurrection of removed files.

Thanks for the explanation.

> I think the original that did 'git reset -- "$@"' upfront lost new
> paths from the index (without removing it from the working tree), I
> am guessing that it is why "clean" was there to get rid of them, and
> if that is the reason, I can understand why the original code was
> behaving incorrectly---it would get rid of new files that did not
> exist in HEAD correctly, but it also would remove untracked ones,
> because after that first 'reset', the code couldn't tell between
> them.
> 
> And I think that is what we want, i.e. why the original was wrong
> and how the new one fixes it, to describe in the log message to
> justify this change.
> 
> One thing that I didn't think through and you need to verify is if
> we need to do anything extra to deal with binary files (in the old
> days, we needed --full-index and --binary options to produce and
> apply a binary patch; I do not offhand know if that is still the
> case) in the final "diff-index piped to apply -R --index" dance.
> 
> So I am asking you to fill in quite a lot of gaps that I didn't do
> with only the above two-liner ;-)  You should take the authorship
> and, if you like, credit me with helped-by: or something.

Thanks, I'll fill in the gaps, and send a new patch, hopefully over
the weekend.

> Thanks.
> 
> 
> >
> > [...]
> >
> > --- >8 ---
> > From: Junio C Hamano 
> > Subject: [PATCH] stash: don't delete untracked files that match pathspec
> >
> > Currently when 'git stash push -- ' is used, untracked files
> > that match the pathspec will be deleted, even though they do not end up
> > in a stash anywhere.
> >
> > Untracked files should be left along by 'git stash push -- '
> > unless the --untracked or --all flags are given.  Fix that.
> >
> > Reported-by: Reid Price 
> > Test-by: Thomas Gummerer 
> > Signed-off-by: Thomas Gummerer 
> > ---
> >  git-stash.sh |  5 ++---
> >  t/t3903-stash.sh | 16 
> >  2 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/git-stash.sh b/git-stash.sh
> > index 1114005ce2..a979bfb665 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -322,10 +322,9 @@ push_stash () {
> >  
> > if test $# 

Can't squash merge with merge.ff set to false

[Robert Dailey]

Not sure if this is intended or a bug, but with the following configuration:

$ git config --global merge.ff false

I am not able to merge my topic branch into master with squash option:

$ git checkout master
$ git merge --squash topic
fatal: You cannot combine --squash with --no-ff.

I'm not sure why a non-fast-forward merge would prevent a squash
merge, since by its very nature a squashed merge is not a fast forward
merge (or maybe it is if you only have one commit).

Is there an issue here? I like fast forward merges to be off by
default, since I want to control when they happen. Most of my merges
do not use --squash, so I'm catering to the common case.

Need advice on how to get past this issue. Thanks in advance.

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 11:53:50AM -0800, Junio C Hamano wrote:

> > They haven't even been reviewed yet. If they get good feedback, then the
> > maintainer will pick them up, then merge them to 'next', and then
> > eventually to 'master', after which they'd become part of the next
> > major release. For a pure bug-fix, it may instead go to 'maint' and
> > become part of the next minor release.
> 
> Even a pure bug-fix, unless it is something no longer needed on the
> 'master' front, goes thru 'pu'->'next'->'master' avenue first, and
> is recorded in the RelNotes with the notes like "(merge d45420c1c8
> jk/abort-clone-with-existing-dest later to maint)" when it happens.
> 
>   side note: in fact "grep -e 'later to maint' RelNotes" is
>   how I remind myself what to merge down to 'maint'; the
>   actual procedure is a bit more involved (those interested in
>   the details can find the 'ML' script on the 'todo' branch;
>   its name stands for 'merge later')
> 
> Later, after not hearing from people that the "fix" breaks things,
> the topic is also mreged to 'maint' and becomes part of the next
> minor release.

Out of curiosity, did this change at some point? I thought the process
used to be to merge to maint, and then pick up topics in master by
merging maint to master.

-Peff

Re: [PATCH v3 0/5] Add --no-ahead-behind to status

[Junio C Hamano]

Jeff Hostetler  writes:

> I kinda trying to serve 2 masters here.  As we discussed earlier, we
> don't want config options to change porcelain formats, hence the
> true/false thing only affecting non-porcelain formats.  On the other
> hand, VS and git.exe are on different release schedules.  Normally,
> I'd just have VS emit a "git status --no-ahead-behind --porcelain=v2"
> and be done, but that requires that git.exe gets updated before VS.
> We do control some of that, but if VS gets updated first, that causes
> an error, whereas "git -c status.aheadbehind= status --porcelain=v2"
> does not.  It is respected if/when git is updated and ignored until
> then.  Likewise, if they update git first, we can tell them to set a
> config setting on the repo and inherit it for porcelain v2 output
> without VS knowing about it.  Sorry, if that's too much detail.

That is not really too much detail.  

But if the above is your plan, then boolean=2 cannot merely be "an
experimental" as described in [5/5]; it needs to be carried for some
extended period of time, depending on the release schedule of the
other half of the coin.

> It is OK with me if we omit the last commit in the patch series (that
> does the experimental =2 extension) and I'll deal with this separately
> (maybe differently) in the gvfs fork.

I think that sounds more sensible.  Thanks.

Re: Bug report: git clone with dest

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jan 03, 2018 at 02:42:51PM -0800, Isaac Shabtay wrote:
>
>> Indeed interesting... this one's for the books...
>> Thanks for the patches. Any idea when these are going to make it to the
>> official Git client builds? (specifically the Windows one)
>
> They haven't even been reviewed yet. If they get good feedback, then the
> maintainer will pick them up, then merge them to 'next', and then
> eventually to 'master', after which they'd become part of the next
> major release. For a pure bug-fix, it may instead go to 'maint' and
> become part of the next minor release.

Even a pure bug-fix, unless it is something no longer needed on the
'master' front, goes thru 'pu'->'next'->'master' avenue first, and
is recorded in the RelNotes with the notes like "(merge d45420c1c8
jk/abort-clone-with-existing-dest later to maint)" when it happens.

side note: in fact "grep -e 'later to maint' RelNotes" is
how I remind myself what to merge down to 'maint'; the
actual procedure is a bit more involved (those interested in
the details can find the 'ML' script on the 'todo' branch;
its name stands for 'merge later')

Later, after not hearing from people that the "fix" breaks things,
the topic is also mreged to 'maint' and becomes part of the next
minor release.

> Right now we're entering release freeze for v2.16.0. We'd still take
> fixes for recent breakages there, but given the age of the problem I
> doubt it will make the cutoff. But as this is a bug-fix, it might make
> it into v2.16.1.

Yup.  From the usual timeline, I'd expect this to be part of
'master' working towards 2.17 and to become a part of 2.16.x
series.

Thanks.