Re: [PATCH v2 0/3] rebase: give precise error message

[Junio C Hamano]

Kaartic Sivaraam  writes:

>> I do not think the above is a good change in the first place for at
>> least two reasons.  By saying , the updated text says "not just
>> branches but you can also give tags and remote-tracking branches".
>
> I used  as you could actually use tags, remote-tracking branches
> and even "notes" ...
> ...
> It just works (of course, I couldn't understand what it did)! I didn't
> want to lie to the user. So, I used . So, should we just update
> the  part of the doc or should we update the code for 'rebase'?
> I'm unsure.

By saying , you are not covering these cases

git rebase master HEAD^0
git rebase master pu^2

where the command gets non refs.

Most of the time, people use a , and rare cases like these
what a user can give is not restricted to a , so there is *no*
value in replacing  with .  If we needed to replace it
with something, replacing  with [ | ] is
not wrong per-se, but I do not think it is an improvement.

As  is merely a kind of , it may be tempting to
instead replace  with , but I do not think it is
a good idea, either.  No matter what you write there in the synopsis
(and let's call it X), the description would have to say "when X is
the name of a branch, that branch is checked out, its history gets
rebased, and at the end, the tip of that branch points at the
result.  When X is not a branch but just a commit-ish, the HEAD is
detached at that commit, its history gets rebased and you'll be left
in that state".  Having  in that sentence is clear enough
and any intelligent reader would understand what we mean by that
notation: we are showing there can be various things that can come
on the command line depicted in the SYNOPSIS section at the point
where we have a placeholder called , and the argument does
not necessarily have to be the name of a branch.

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Max Kirillov]

On Tue, Nov 28, 2017 at 10:26:33PM -0500, Jeff King wrote:
> On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:
> That said, we already have some precedent in "git version
> --build-options" to report sizes there. Can we do something like the
> patch below instead of adding a new test helper?
> 
> diff --git a/help.c b/help.c
> index 88a3aeaeb9..9590eaba28 100644
> --- a/help.c
> +++ b/help.c
> @@ -413,6 +413,7 @@ int cmd_version(int argc, const char **argv, const char 
> *prefix)
>  
>   if (build_options) {
>   printf("sizeof-long: %d\n", (int)sizeof(long));
> + printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
>   /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
>   }
>   return 0;

Thank you! I knew there should have been something.

If nobody objects changing the user-visible behavior, I'll
consider using this.

PS: I'll respond to your other reply a bit later.

Re: [PATCH v6 0/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Max Kirillov]

On Mon, Nov 27, 2017 at 01:02:10PM +0900, Junio C Hamano wrote:
> To recap (other than the typofix in the proposed log message), here
> is what I would have as SQUASH??? on top of (or interspersed with)
> v6.

Thank you. I'll update it a bit later. May/should I add
"Signed-off-by:" from you?

Re: git-p4: cloning with a change number does not import all files

[Patrick Rouleau]

Hi,

On Mon, Nov 27, 2017 at 7:52 AM, Lars Schneider
 wrote:
>
> what is your goal here? Do you want to convert the repo to Git or do you
> want to use Git to interact with a P4 repo?

I want to use git to interact with a P4 repo. I am used to git tools
and I prefer them to
P4V. Visual Studio is also slower with P4 plugin and I hate P4
approach to check out
a file before modifying it. The .sln files always get check outed for no reason.

I am working remotely. The P4 server is behind a VPN. That one reason
I am trying to
clone from some point in the past. I also want to have some history to
see from where
come the code and from who.

Regards,
P. Rouleau

[PATCH v4 4/4] builtin/branch: strip refs/heads/ using skip_prefix

[Kaartic Sivaraam]

Instead of hard-coding the offset strlen("refs/heads/") to skip
the prefix "refs/heads/" use the skip_prefix() function which
is more communicative and verifies that the string actually
starts with that prefix.

Signed-off-by: Kaartic Sivaraam 
---
 v2 and v3 of this patch got detached to a different thread due
 to a small mistake I made, sorry. You can find it at

 https://public-inbox.org/git/20171121141852.551-1-kaartic.sivar...@gmail.com/

 I've brought v4 back to this thread to bring back the continuity. I guess
 this series gets stabilised with this patch.

 Changes in v4 from v1:

  - updated commit message

  - removed superfluous comment

  - updated variable names

  - checked for the errors pointed out by skip_prefix

 builtin/branch.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ca9d8abd0..b2f5e4a0c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -462,6 +462,8 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
+   const char *interpreted_oldname = NULL;
+   const char *interpreted_newname = NULL;
int recovery = 0;
int clobber_head_ok;
 
@@ -493,6 +495,11 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
reject_rebase_or_bisect_branch(oldref.buf);
 
+   if (!skip_prefix(oldref.buf, "refs/heads/", _oldname) ||
+   !skip_prefix(newref.buf, "refs/heads/", _newname)) {
+   die("BUG: expected prefix missing for refs")
+   }
+
if (copy)
strbuf_addf(, "Branch: copied %s to %s",
oldref.buf, newref.buf);
@@ -508,10 +515,10 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
if (recovery) {
if (copy)
warning(_("Created a copy of a misnamed branch '%s'"),
-   oldref.buf + 11);
+   interpreted_oldname);
else
warning(_("Renamed a misnamed branch '%s' away"),
-   oldref.buf + 11);
+   interpreted_oldname);
}
 
if (!copy &&
@@ -520,9 +527,9 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
strbuf_release();
 
-   strbuf_addf(, "branch.%s", oldref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_oldname);
strbuf_release();
-   strbuf_addf(, "branch.%s", newref.buf + 11);
+   strbuf_addf(, "branch.%s", interpreted_newname);
strbuf_release();
if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) 
< 0)
die(_("Branch is renamed, but update of config-file failed"));
-- 
2.15.0.531.g2ccb3012c

Re: [PATCH v6 2/2] t5560-http-backend-noserver.sh: add CONTENT_LENGTH cases

[Jeff King]

On Mon, Nov 27, 2017 at 12:40:51AM +0200, Max Kirillov wrote:

> > Rather than introducing a new 'test' program, would it be possible to
> > get by with just using 'printf' from the shell?
> > 
> > % printf "%zu\n" -20
> > 18446744073709551596
> 
> I thought about it, of course. But, I am not sure I can
> exclude cases when the shell's printf uses 64-bit size_t and
> git 32-bit one, or vise-versa. Same way, I cannot say it for
> sure for any other software which I might use here instead
> of the shell's printf. The only somewhat sure way would be
> to use the same compiler, with same settings, which is used
> for the production code.
> 
> I do not exclude possibility that my reasoning above is
> wrong, either in general of specifically for git case. If
> there are some examples where it is already used and the
> risk of type size mismatch is prevented I could do it
> similarly.

That's definitely something to worry about, and I have a vague
recollection that build differences between the shell environment and
git have bitten us in the past.

That said, we already have some precedent in "git version
--build-options" to report sizes there. Can we do something like the
patch below instead of adding a new test helper?

diff --git a/help.c b/help.c
index 88a3aeaeb9..9590eaba28 100644
--- a/help.c
+++ b/help.c
@@ -413,6 +413,7 @@ int cmd_version(int argc, const char **argv, const char 
*prefix)
 
if (build_options) {
printf("sizeof-long: %d\n", (int)sizeof(long));
+   printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
}
return 0;

That does still require you to compute size_t based on the byte-size in
the test script, that should be do-able.

-Peff

Re: [PATCH v6 1/2] http-backend: respect CONTENT_LENGTH as specified by rfc3875

[Jeff King]

On Sun, Nov 26, 2017 at 09:38:12PM +0200, Max Kirillov wrote:

> From: Florian Manschwetus 
> Date: Wed, 30 Mar 2016 10:54:21 +0200
> 
> http-backend reads whole input until EOF. However, the RFC 3875 specifies
> that a script must read only as many bytes as specified by CONTENT_LENGTH
> environment variable. Web server may exercise the specification by not closing
> the script's standard input after writing content. In that case http-backend
> would hang waiting for the input. The issue is known to happen with
> IIS/Windows, for example.
> 
> Make http-backend read only CONTENT_LENGTH bytes, if it's defined, rather than
> the whole input until EOF. If the variable is not defined, keep older behavior
> of reading until EOF because it is used to support chunked transfer-encoding.

I missed out on review of the earlier iterations from the past few days,
but I looked this over in light of the comments I made long ago in:

  https://public-inbox.org/git/20160329201349.gb9...@sigill.intra.peff.net/

The concerns I had there were:

  1. It did the wrong thing when CONTENT_LENGTH was not present in the
 environment. Your version here gets this right.

  2. I earlier was worried that this wouldn't kick in for the
 inflate_request() code path. It looks like we do use read_request()
 from inflate_request(), but only when buffer_input is true. Which
 means we wouldn't do so for receive-pack (i.e., for pushes).

 I'm not sure if the client would ever gzip a push request. Without
 double-checking, I suspect it would if the list of refs to update
 was sufficiently large (and at any rate, I think other clients
 potentially could do so).

 That _might_ be OK in practice. If the gzip stream is well-formed,
 we'd stop at its end. We'd possibly still try to read() more bytes
 than were promised, but if I understand the original problem,
 that's not that big a deal as long as we don't hang waiting for
 EOF.

 If the stream isn't well-formed, we'd hang waiting for more bytes
 (rather than seeing the EOF and complaining that the gzip stream is
 truncated). That's poor, but no worse than the current behavior.

  3. For large inputs (like incoming packfiles), we connect the
 descriptor directly to index-pack or unpack-objects, and they try
 to read to EOF.

 For a well-formed pack, I _think_ this would work OK. We'd see the
 end of the pack and quit (there's a check for garbage at the end of
 the pack, but it triggers only for the non-pipe case).

 For a truncated input, we'd hang forever rather than report an
 error.

So I suspect there are lurking problems that may trigger in corner
cases. That said, I don't think this pack makes any case _worse_, and it
may make some common ones better. So I'm not opposed, though we may be
giving people a false sense of security that it actually works robustly
on IIS.

> ---
>  config.c   |  2 +-
>  config.h   |  1 +
>  http-backend.c | 50 +-
>  3 files changed, 51 insertions(+), 2 deletions(-)

The patch itself looks OK, modulo the comments already made by others.
Two minor observations, and a possible bug:

> +static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char 
> **out)
> +{

I did wonder if this "stop at n bytes" could simply be rolled into the
existing read_request loop (by limiting the length we pass to
read_in_full there). But it may be cleaner to just have a separate
function. There's some repetition, but not much since we can rely on a
single malloc and read_in_full() for this case.

> + unsigned char *buf = NULL;
> + ssize_t cnt = 0;
> +
> + if (max_request_buffer < req_len) {
> + die("request was larger than our maximum size (%lu): %lu;"
> + " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> + max_request_buffer,
> + req_len);
> + }
> +
> + if (req_len <= 0) {
> + *out = NULL;
> + return 0;
> + }

I was slightly surprised by "<= 0" here. We should never get here with a
negative req_len, since we'd catch that in the read_request() wrapper
here. If we want to document that assumption, should this be
assert(req_len >= 0)?

I'm also puzzled about the behavior with a zero-byte CONTENT_LENGTH.
We'd return NULL here. But in the other read_request_eof() path, we'd
end up with a non-NULL pointer. I'm not sure if it matters to the caller
or not, but it seems like a potential trap.

Is it even worth special-casing here? Our xmalloc and read_in_full
wrappers should handle the zero-byte just fine. I think this whole
conditional could just go away.

-Peff

Re: [PATCH v2 0/3] rebase: give precise error message

[Kaartic Sivaraam]

On Wed, 2017-11-29 at 09:10 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > If  is the correct substitute , I could try to send a
> > patch that fixes this.
> 
> I do not think the above is a good change in the first place for at
> least two reasons.  By saying , the updated text says "not just
> branches but you can also give tags and remote-tracking branches".

I used  as you could actually use tags, remote-tracking branches
and even "notes" (i.e.) any kind of  for the  part. That's
how the code for rebasing works currently (specifically the elif part),

if git show-ref --verify --quiet -- "refs/heads/$1" &&
   orig_head=$(git rev-parse -q --verify "refs/heads/$1")
then
head_name="refs/heads/$1"
elif orig_head=$(git rev-parse -q --verify "$1")
then
head_name="detached HEAD"
else
die "$(eval_gettext "fatal: no such branch: \$branch_name")"
fi

Which means that you could not only do,

$ git rebase origin/next refs/remotes/origin/maint

You could even get creative and do,

$ git rebase origin/next refs/notes/$something

It just works (of course, I couldn't understand what it did)! I didn't
want to lie to the user. So, I used . So, should we just update
the  part of the doc or should we update the code for 'rebase'?
I'm unsure.

> In reality, however, it can take any commit-ish, as the "no we are
> not rebasing the current checkout" form of the command is merely a
> short-hand to check it out first [*1*].  It gives appearance that
> the change is making it more broad, but not making it broad enough.
> At the same time, more than 90% of the time, people give a branch
> name there.  Saying "branch-or-commit" for a short description of
> the command line (which is what synopsis section is) does not add
> that much value.  The body text of the description where we refer to
> the  parameter of course need to be updated to say "in
> addition, instead of a branch name, you can give a commit-ish that
> is *not* a branch name.  When you do so, instead of checking out the
> branch, the HEAD is detached at that commit before the history
> leading to it is rebased."
> 
> And because we have to say "it can be a non-branch commit-ish and
> here is what happens when you do so" anyway, I think the current
> synopsis as-is would be better than making it less clear and more
> scary by replacing  with other things like  or
> [ | ].
> 
> 
> [Footnote]
> 
> *1* As my "log --first-parent --oneline master..pu" is a strand of
> pearls each of which is a merge of a topic branch,
> 
> $ git rebase -i master pu~$n^2
> 
> can be a handy way to make a trial rebase (after double checking
> the result of "git tbdiff", I may do "git checkout -B topic" to
> overwrite the original or I may discard the result of rebasing).

Re: Question regarding "next" merge

[Junio C Hamano]

Dan Jacques  writes:

> I read the "what's cooking in Git" notes and saw that you were intending to
> introduce this patch set into "next". Johannes pointed out some quoting errors
> that break Windows builds, and I have incorporated fixes in my working copy.
>
> I was going to hold off on publishing v4 in case some of the other reviewers
> had additional comments, but if you would prefer, I can publish them now
> before you merge, so "next" doesn't break on Windows.

Please do so.  Even though I try to, I cannot follow and remember
all discussions on each and every topic, and noticing your own topic
getting mislabeled in "What's cooking" summary and yelling loudly at
me is the best way to help the project overall by preventing me from
mistakenly merging a topic that needs still an update.

And if you are *not* ready and need a bit more time to send in an
update, I'd prefer that you do *not* rush.  I can easily make a note
to wait for an update without merging the latest round of the topic
I have in my tree.  Also, I'll go offline towards the end of this
week, so even if you rushed, the updated one may not hit my tree
anyway.

Thanks for stopping me.

Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

[liam Beguin]

Hi Johannes,

On 27/11/17 06:04 PM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 26 Nov 2017, Liam Beguin wrote:
> 
>> diff --git a/Documentation/rebase-config.txt 
>> b/Documentation/rebase-config.txt
>> index 30ae08cb5a4b..0820b60f6e12 100644
>> --- a/Documentation/rebase-config.txt
>> +++ b/Documentation/rebase-config.txt
>> @@ -30,3 +30,22 @@ rebase.instructionFormat::
>>  A format string, as specified in linkgit:git-log[1], to be used for the
>>  todo list during an interactive rebase.  The format will
>>  automatically have the long commit hash prepended to the format.
>> +
>> +rebase.abbreviateCommands::
>> +If set to true, `git rebase` will use abbreviated command names in the
>> +todo list resulting in something like this:
>> +
>> +---
>> +p deadbee The oneline of the commit
>> +p fa1afe1 The oneline of the next commit
>> +...
>> +---
> 
> I *think* that AsciiDoc will render this in a different way from what we
> want, but I am not an AsciiDoc expert. In my hands, I always had to add a
> single + in an otherwise empty line to start a new indented paragraph *and
> then continue with non-indented lines*.
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index 810b7850748e..aa01e8bd9280 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -795,6 +795,13 @@ static const char *command_to_string(const enum 
>> todo_command command)
>>  die("Unknown command: %d", command);
>>  }
>>  
>> +static const char command_to_char(const enum todo_command command)
>> +{
>> +if (command < TODO_COMMENT && todo_command_info[command].c)
>> +return todo_command_info[command].c;
>> +return -1;
> 
> My initial reaction was: should we return comment_line_char instead of -1
> here? Only after reading how this is called did I realize that the idea is
> to use full command names if there is no abbreviation. Not sure whether
> this is worth a code comment. What do you think?
> 

I guess it probably deserves a comment!

>> +}
>> +
>>  static int is_noop(const enum todo_command command)
>>  {
>>  return TODO_NOOP <= command;
>> @@ -1242,15 +1249,16 @@ static int parse_insn_line(struct todo_item *item, 
>> const char *bol, char *eol)
>>  return 0;
>>  }
>>  
>> -for (i = 0; i < TODO_COMMENT; i++)
>> +for (i = 0; i < TODO_COMMENT; i++) {
>>  if (skip_prefix(bol, todo_command_info[i].str, )) {
>>  item->command = i;
>>  break;
>> -} else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
>> +} else if (bol[1] == ' ' && *bol == command_to_char(i)) {
>>  bol++;
>>  item->command = i;
>>  break;
>>  }
>> +}
>>  if (i >= TODO_COMMENT)
>>  return -1;
>>  
> 
> I would prefer this hunk to be skipped, it does not really do anything if
> I understand correctly.

Ok, I was not so sure about this but thought it was probably worth it.
Will remove.

> 
>> @@ -2443,8 +2451,8 @@ void append_signoff(struct strbuf *msgbuf, int 
>> ignore_footer, unsigned flag)
>>  strbuf_release();
>>  }
>>  
>> -int sequencer_make_script(int keep_empty, FILE *out,
>> -int argc, const char **argv)
>> +int sequencer_make_script(int keep_empty, int abbreviate_commands, FILE 
>> *out,
>> +  int argc, const char **argv)
>>  {
>>  char *format = NULL;
>>  struct pretty_print_context pp = {0};
>> @@ -2483,7 +2491,9 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  strbuf_reset();
>>  if (!keep_empty && is_original_commit_empty(commit))
>>  strbuf_addf(, "%c ", comment_line_char);
>> -strbuf_addf(, "pick %s ", oid_to_hex(>object.oid));
>> +strbuf_addf(, "%s %s ",
>> +abbreviate_commands ? "p" : "pick",
>> +oid_to_hex(>object.oid));
> 
> I guess the compiler will optimize this code so that the conditional is
> evaluated only once. Not that this is performance critical ;-)

Is your guess enough? :-) If not, how could I make sure this is optimized?
Should I do that check before the while() loop?

> 
>>  pretty_print_commit(, commit, );
>>  strbuf_addch(, '\n');
>>  fputs(buf.buf, out);
>> @@ -2539,7 +2549,7 @@ int add_exec_commands(const char *command)
>>  return 0;
>>  }
>>  
>> -int transform_todo_ids(int shorten_ids)
>> +int transform_todo_ids(int shorten_ids, int abbreviate_commands)
>>  {
>>  const char *todo_file = rebase_path_todo();
>>  struct todo_list todo_list = TODO_LIST_INIT;
>> @@ -2575,19 +2585,33 @@ int transform_todo_ids(int shorten_ids)
>>  todo_list.items[i + 1].offset_in_buf :
>>  todo_list.buf.len;
>>  
>> -if (item->command >= TODO_EXEC && 

Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

[liam Beguin]

Hi Peff,

Thanks for taking the time to test this, I'll squash that patch in v2.

On 27/11/17 06:11 PM, Jeff King wrote:
> On Tue, Nov 28, 2017 at 12:04:45AM +0100, Johannes Schindelin wrote:
> 
>>> +rebase.abbreviateCommands::
>>> +   If set to true, `git rebase` will use abbreviated command names in the
>>> +   todo list resulting in something like this:
>>> +
>>> +---
>>> +   p deadbee The oneline of the commit
>>> +   p fa1afe1 The oneline of the next commit
>>> +   ...
>>> +---
>>
>> I *think* that AsciiDoc will render this in a different way from what we
>> want, but I am not an AsciiDoc expert. In my hands, I always had to add a
>> single + in an otherwise empty line to start a new indented paragraph *and
>> then continue with non-indented lines*.
> 
> Good catch. Interestingly enough, my asciidoc seems to render this
> as desired for the docbook/roff version, but has screwed-up indentation
> for the HTML version.
> 
> Fixing it as you suggest makes it look good in both (and I think you can
> never go wrong with "+"-continuation, aside from making the source a bit
> uglier).
> 
> Squashable patch below for convenience, since I did try it.
> 
> -Peff
> 
> diff --git a/Documentation/rebase-config.txt b/Documentation/rebase-config.txt
> index 0820b60f6e..42e1ba7575 100644
> --- a/Documentation/rebase-config.txt
> +++ b/Documentation/rebase-config.txt
> @@ -34,18 +34,19 @@ rebase.instructionFormat::
>  rebase.abbreviateCommands::
>   If set to true, `git rebase` will use abbreviated command names in the
>   todo list resulting in something like this:
> -
> ++
>  ---
>   p deadbee The oneline of the commit
>   p fa1afe1 The oneline of the next commit
>   ...
>  ---
> -
> - instead of:
> -
> ++
> +instead of:
> ++
>  ---
>   pick deadbee The oneline of the commit
>   pick fa1afe1 The oneline of the next commit
>   ...
>  ---
> - Defaults to false.
> ++
> +Defaults to false.
> 

Liam

Re: [PATCH 4/5] rebase -i: learn to abbreviate command names

[liam Beguin]

Hi Junio,

On 27/11/17 12:19 AM, Junio C Hamano wrote:
> Liam Beguin  writes:
> 
>>  if (command == MAKE_SCRIPT && argc > 1)
>> -return !!sequencer_make_script(keep_empty, stdout, argc, argv);
>> +return !!sequencer_make_script(keep_empty, abbreviate_commands,
>> +   stdout, argc, argv);
> 
> This suggests that a preliminary clean-up to update the parameter
> list of sequencer_make_script() is in order just before this step.
> How about making it like so, perhaps:
> 
> int sequencer_make_script(FILE *out, int ac, char **av, unsigned flags)
> 
> where keep_empty becomes just one bit in that flags word.  Then another
> bit in the same flags word can be used for this option.
> 
> Otherwise, every time somebody comes up with a new and shiny feature
> for the function, we'd end up adding more to its parameter list.
> 

Will do.
Thanks, 

Liam

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[liam Beguin]

Hi Johannes,

Thanks for taking the time to review this.

On 27/11/17 05:42 PM, Johannes Schindelin wrote:
> Hi Liam,
> 
> could I ask for a favor? I'd like the oneline to start with
> 
>   rebase -i -x: ...
> 
> (this would help future me to realize what this commit touches already
> from the concise graph output I favor).

Sure, I'll update the commit subject.

> 
> On Sun, 26 Nov 2017, Liam Beguin wrote:
> 
>> Recent work on `git-rebase--interactive` aim to convert shell code to C.
>> Even if this is most likely not a big performance enhacement, let's
>> convert it too since a comming change to abbreviate command names requires
>> it to be updated.
> 
> Since Junio did not comment on the commit message: could you replace
> `aim` by `aims`, `enhacement` by `enhancement` and `comming` by `coming`?

Ow.. sorry about that! I'll fix those and make sure to proofread better next 
time!

> 
>> @@ -36,6 +37,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
>> char *prefix)
>>  N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
>>  OPT_CMDMODE(0, "rearrange-squash", ,
>>  N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
>> +OPT_CMDMODE(0, "add-exec", ,
>> +N_("insert exec commands in todo list"), ADD_EXEC),
> 
> Maybe `add-exec-commands`? I know it is longer to type, but these options do
> not need to be typed interactively and the longer name would be consistent
> with the function name.

Makes sense. It'll also be more consistent with the rest of the commands above.

> 
>> diff --git a/sequencer.c b/sequencer.c
>> index fa94ed652d2c..810b7850748e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  return 0;
>>  }
>>  
> 
> As the code in add_exec_commands() may appear convoluted (why not simply
> append the command after any pick?), the original comment would be really
> nice here:
> 
>   /*
>* Add commands after pick and (series of) squash/fixup commands
>* in the todo list.
>*/
> 

I'll make sure to include that comment.
The code is a bit convoluted as you say... I wanted to send it "as is" first
to get comments and update based on feedback from the list.

I just realized we could maybe add exec instructions only after pick commands
if we do add-exec-commands before rearrange-squash. I'll test it out.

>> +int add_exec_commands(const char *command)
>> +{
>> +const char *todo_file = rebase_path_todo();
>> +struct todo_list todo_list = TODO_LIST_INIT;
>> +int fd, res, i, first = 1;
>> +FILE *out;
>> +
>> +strbuf_reset(_list.buf);
> 
> The todo_list.buf has been initialized already (via TODO_LIST_INIT), no
> need to reset it again.
> 
>> +fd = open(todo_file, O_RDONLY);
>> +if (fd < 0)
>> +return error_errno(_("could not open '%s'"), todo_file);
>> +if (strbuf_read(_list.buf, fd, 0) < 0) {
>> +close(fd);
>> +return error(_("could not read '%s'."), todo_file);
>> +}
>> +close(fd);
> 
> As Junio pointed out so gently: there is a helper function that does this
> all very conveniently for us:
> 
>   if (strbuf_read_file(_list.buf, todo_file, 0) < 0)
>   return error_errno(_("could not read '%s'"), todo_file);
> 
> And as I realized looking at the surrounding code: you probably just
> inherited my inelegant code by copy-editing from another function in
> sequencer.c. Should you decide to add a preparatory patch to your patch
> series that converts these other callers, or even refactors all that code
> that reads the git-rebase-todo file and then parses it, I would be quite
> happy... :-) (although I would understand if you deemed this outside the
> purpose of your patch series).
> 

You guessed well, I mostly did copy-editing... I thought I found this code
a little confusing because I'm not used to as much pointer gymnastics but
it reassures me a bit to read this :-). I'll see if I can come up with a
better solution.

>> +res = parse_insn_buffer(todo_list.buf.buf, _list);
>> +if (res) {
>> +todo_list_release(_list);
>> +return error(_("unusable todo list: '%s'"), todo_file);
>> +}
> 
> The variable `res` is not really used here. Let's just put the
> parse_insn_buffer() call inside the if ().
> 

Will do.

>> +out = fopen(todo_file, "w");
>> +if (!out) {
>> +todo_list_release(_list);
>> +return error(_("unable to open '%s' for writing"), todo_file);
>> +}
>> +for (i = 0; i < todo_list.nr; i++) {
>> +struct todo_item *item = todo_list.items + i;
>> +int bol = item->offset_in_buf;
>> +const char *p = todo_list.buf.buf + bol;
>> +int eol = i + 1 < todo_list.nr ?
>> +todo_list.items[i + 1].offset_in_buf :
>> +todo_list.buf.len;
> 
> This smells like 

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[brian m. carlson]

On Mon, Nov 27, 2017 at 06:05:20PM -0500, Jeff King wrote:
> On Mon, Nov 27, 2017 at 08:09:32PM +, brian m. carlson wrote:
> 
> > > Show a message in the original terminal and get rid of it when the
> > > editor returns.
> > [...]
> > 
> > Sorry for coming to the topic so late, but it occurred to me that we
> > might want to conditionalize this on an advice.* flag.  I expect there
> > are some people who will never want to see this, and letting them turn
> > it off would be good.
> 
> I am torn between saying "yes please, I would absolutely set such an
> option myself" and "if we need advice.*, that is a good sign that the
> feature is mis-designed".

I myself would also set such an option.  More importantly, I think there
are other developers who would complain about such a message, and I'd
like to give them an easy way to turn it off.

Note that I am not altogether opposed to advice.*, since I personally
find it helpful in certain cases as an aide-mémoire of what state my
branch is in.

> Let me elaborate a bit on the latter.
> 
> My gut feeling is that this is absolutely the wrong place to put a
> message like this. We don't know enough about what the editor is doing,
> so we have to take pains to avoid a crufty message in the terminal,
> including:
> 
>   - playing ANSI-term trickery to erase the message
> 
>   - hard-coding (!) emacsclient as a special case

I agree that the editor is the right place to put this, but I also
understand that the people most likely to be helped by this are the
least likely to write such scripting.  I think this is especially so
because in my experience newer, less advanced users are more likely than
not to use graphical editors.

Git is an extraordinarily powerful piece of software, but it's also hard
for people to use (judging by my Twitter feed), so if we can make it
less painful for new users, I'm okay with an advice.* setting.

I'm slightly negative on hard-coding emacsclient, since I'm sure someone
will come up with another editor that does the same thing, and then we'd
have to update it.

> If the anti-cruft techniques I mentioned above work well in practice,
> then we get to have our cake and eat it, too. If they don't, then I'm
> not sure if the tradeoff is worth it.

I have a feeling that this may not work properly in editors that don't
support the concept of xterm alternate screens (such as the Linux
console) where the editor window is left on the screen, but it may be
that it works fine and I've just misunderstood how it's supposed to
work.  It may also be that we don't care about such cases, as any cruft
would have already scrolled off the screen.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

[liam Beguin]

Hi Junio,

On 27/11/17 12:14 AM, Junio C Hamano wrote:
> Liam Beguin  writes:
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index fa94ed652d2c..810b7850748e 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2492,6 +2492,52 @@ int sequencer_make_script(int keep_empty, FILE *out,
>>  return 0;
>>  }
>>  
>> +int add_exec_commands(const char *command)
>> +{
> 
> As the name of a public function, it does not feel that this hints
> it strongly enough that it is from and a part of sequencer.c API.
> 
>> +const char *todo_file = rebase_path_todo();
>> +struct todo_list todo_list = TODO_LIST_INIT;
>> +int fd, res, i, first = 1;
>> +FILE *out;
> 
> Having had to scan backwards while trying to see what the loop that
> uses this variable is doing and if it gets affected by things that
> happened before we entered the loop, I'd rather not to see 'first'
> initialized here, left unused for quite some time until the loop is
> entered.  It would make it a lot easier to follow if it is declared
> and left uninitilized here, and set to 1 immediately before the
> for() loop that uses it.
> 

I agree that moving 'first = 1' just above the for() loop makes it
more obvious. I'm not quite fond of how this is implemented, I just
'translated' the shell code and was hoping on maybe a few comments
on how to improve it.

>> +
>> +strbuf_reset(_list.buf);
>> +fd = open(todo_file, O_RDONLY);
>> +if (fd < 0)
>> +return error_errno(_("could not open '%s'"), todo_file);
>> +if (strbuf_read(_list.buf, fd, 0) < 0) {
>> +close(fd);
>> +return error(_("could not read '%s'."), todo_file);
>> +}
>> +close(fd);
> 
> Is this strbuf_read_file() written in longhand?

Thanks for pointing this out! I'll update. And as Johannes pointed out,
I've copied this from surrounding functions, I'll add a preparatory path
to update those too.

> 
>> +res = parse_insn_buffer(todo_list.buf.buf, _list);
>> +if (res) {
>> +todo_list_release(_list);
>> +return error(_("unusable todo list: '%s'"), todo_file);
>> +}
>> +
>> +out = fopen(todo_file, "w");
>> +if (!out) {
>> +todo_list_release(_list);
>> +return error(_("unable to open '%s' for writing"), todo_file);
>> +}
>> +for (i = 0; i < todo_list.nr; i++) {
>> +struct todo_item *item = todo_list.items + i;
>> +int bol = item->offset_in_buf;
>> +const char *p = todo_list.buf.buf + bol;
>> +int eol = i + 1 < todo_list.nr ?
>> +todo_list.items[i + 1].offset_in_buf :
>> +todo_list.buf.len;
> 
> Should bol and eol be of type size_t instead?  The values that get
> assigned to them from other structures are.
> 

Will do.
Thanks, 

Liam

Re: [PATCH 0/5] rebase -i: add config to abbreviate command names

[liam Beguin]

Hi Junio,

On 27/11/17 12:23 AM, Junio C Hamano wrote:
> Liam Beguin  writes:
> 
>> Liam Beguin (5):
>>   Documentation: move rebase.* configs to new file
>>   Documentation: use preferred name for the 'todo list' script
>>   rebase -i: add exec commands via the rebase--helper
>>   rebase -i: learn to abbreviate command names
>>   t3404: add test case for abbreviated commands
> 
> I didn't send any comment on [1&2/5] but they both looked good.
> 

Thanks for reviewing this. I'll go through your comments and post a
v2 shortly.

Thanks,
Liam

PS: I'm very sorry if someone received a few copies of this, I'm
having issues with my MUA! Hopefully, I've got it right this time...

[PATCH] strbuf: Remove unused stripspace function alias

[Elijah Newren]

In commit 63af4a8446 ("strbuf: make stripspace() part of strbuf",
2015-10-16), stripspace() was moved to strbuf and renamed to
strbuf_stripspace().  A "temporary" alias was added for the old name until
all topic branches had time to switch over.  They have had time, so remove
the old alias.

Signed-off-by: Elijah Newren 
---
 strbuf.h | 9 -
 1 file changed, 9 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 0a74acb236..14c8c10d66 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -480,15 +480,6 @@ extern int strbuf_normalize_path(struct strbuf *sb);
  */
 extern void strbuf_stripspace(struct strbuf *buf, int skip_comments);
 
-/**
- * Temporary alias until all topic branches have switched to use
- * strbuf_stripspace directly.
- */
-static inline void stripspace(struct strbuf *buf, int skip_comments)
-{
-   strbuf_stripspace(buf, skip_comments);
-}
-
 static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 {
if (strip_suffix_mem(sb->buf, >len, suffix)) {
-- 
2.15.0.408.g850bc54b15

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

[Elijah Newren]

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

diff --git a/merge-recursive.c b/merge-recursive.c
index cdf8588c75..e77d2b043d 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 261fb4917d..b1ca3d18e5 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2873,7 +2873,7 @@ test_expect_success '10b-setup: Overwrite untracked with 
dir rename + delete' '
)
 '
 
-test_expect_failure '10b-check: Overwrite untracked with dir rename + delete' '
+test_expect_success '10b-check: Overwrite untracked with dir rename + delete' '
(
cd 10b &&
 
@@ -2944,7 +2944,7 @@ test_expect_success '10c-setup: Overwrite untracked with 
dir rename/rename(1to2)
)
 '
 
-test_expect_failure '10c-check: Overwrite untracked with dir 
rename/rename(1to2)' '
+test_expect_success '10c-check: Overwrite untracked with dir 
rename/rename(1to2)' '
(
cd 10c &&
 
@@ -3013,7 +3013,7 @@ test_expect_success '10d-setup: Delete untracked with dir 
rename/rename(2to1)' '
)
 '
 
-test_expect_failure '10d-check: Delete 

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

[Elijah Newren]

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

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

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

[Elijah Newren]

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

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index 0ccabed4a2..1dcf010aa6 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -714,4 +714,108 @@ test_expect_success '3b-check: Avoid implicit rename if 
involved as source on cu
 #   of a rename on either side of a merge.
 ###
 
+
+###
+# SECTION 4: Partially renamed directory; still exists on both sides of merge
+#
+# What if we were to attempt to do directory rename detection when someone
+# "mostly" moved a directory but still left some files around, or,
+# equivalently, fully renamed a directory in one commmit and then recreated
+# that directory in a later commit adding some new files and then tried to
+# merge?
+#
+# It's hard to divine user intent in these cases, because you can make an
+# argument that, depending on the intermediate history of the side being
+# merged, that some users will want files in that directory to
+# automatically be detected and renamed, while users with a different
+# intermediate history wouldn't want that rename to happen.
+#
+# I think that it is best to simply not have directory rename detection
+# apply to such cases.  My reasoning for this is four-fold: (1) it's
+# easiest for users in general to figure out what happened if we don't
+# apply directory rename detection in any such case, (2) it's an easy rule
+# to explain ["We don't do directory rename detection if the directory
+# still exists on both sides of the merge"], (3) we can get some hairy
+# edge/corner cases that would be really confusing and possibly not even
+# representable in the index if we were to even try, and [related to 3] (4)
+# attempting to resolve this issue of divining user intent by examining
+# intermediate history goes against the spirit of three-way merges and is a
+# path towards crazy corner cases that are far more complex than what we're
+# already dealing with.
+#
+# This section contains a test for this partially-renamed-directory case.
+###
+
+# Testcase 4a, Directory split, with original directory still present
+#   (Related to testcase 1f)
+#   Commit O: z/{b,c,d,e}
+#   Commit A: y/{b,c,d}, z/e
+#   Commit B: z/{b,c,d,e,f}
+#   Expected: y/{b,c,d}, z/{e,f}
+#   NOTE: Even though most files from z moved to y, we don't want f to follow.
+
+test_expect_success '4a-setup: Directory split, with original directory still 
present' '
+   test_create_repo 4a &&
+   (
+   cd 4a &&
+
+   mkdir z &&
+   echo b >z/b &&
+   echo c >z/c &&
+   echo d >z/d &&
+   echo e >z/e &&
+   git add z &&
+   test_tick &&
+   git commit -m "O" &&
+
+   git branch O &&
+   git branch A &&
+   git branch B &&
+
+   git checkout A &&
+   mkdir y &&
+   git mv z/b y/ &&
+   git mv z/c y/ &&
+   git mv z/d y/ &&
+   test_tick &&
+   git commit -m "A" &&
+
+   git checkout B &&
+   echo f >z/f &&
+   git add z/f &&
+   test_tick &&
+   git commit -m "B"
+   )
+'
+
+test_expect_success '4a-check: Directory split, with original directory still 
present' '
+   (
+   cd 4a &&
+
+   git checkout A^0 &&
+
+   git merge -s recursive B^0 &&
+
+   test 5 -eq $(git ls-files -s | wc -l) &&
+   test 0 -eq $(git ls-files -u | wc -l) &&
+   test 0 -eq $(git ls-files -o | wc -l) &&
+
+   git rev-parse >actual \
+   HEAD:y/b HEAD:y/c HEAD:y/d HEAD:z/e HEAD:z/f &&
+   git rev-parse >expect \
+   O:z/b O:z/c O:z/d O:z/e B:z/f &&
+   test_cmp expect actual
+   )
+'
+
+###
+# Rules suggested by section 4:
+#
+#   Directory-rename-detection should be turned off for any directories (as
+#   a source for renames) that exist on both sides of the merge.  (The "as
+#   a source for renames" clarification is due to cases like 1c where
+#   the target directory exists on both sides and we do want the rename
+#   detection.)  But, sadly, see testcase 8b.
+###
+
 test_done
-- 
2.15.0.408.g850bc54b15

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

[Elijah Newren]

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

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

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

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 4f2a263b63..783bdbf59d 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 9444d6a9b9..00617dadf8 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -97,7 +97,10 @@ test_expect_failure 'will not overwrite unstaged changes in 
renamed file' '
git mv c1.c other.c &&
git commit -m rename &&
cp important other.c &&
-   git merge c1a &&
+   test_must_fail git merge c1a >out &&
+   test_i18ngrep "Refusing to lose dirty file at other.c" out &&
+   test -f other.c~HEAD &&
+   test $(git hash-object other.c~HEAD) = $(git rev-parse c1a:c1.c) &&
test_cmp important other.c
 '
 
-- 
2.15.0.408.g850bc54b15

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

[Elijah Newren]

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index d00786f719..16c52a4347 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 8ed85c79de..f64c7d273b 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3713,7 +3713,7 @@ test_expect_success '12b-setup: Moving one directory 
hierarchy into another' '
)
 '
 
-test_expect_failure '12b-check: Moving one directory hierarchy into another' '
+test_expect_success '12b-check: Moving one directory hierarchy into another' '
(
cd 12b &&
 
-- 
2.15.0.408.g850bc54b15

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

[Elijah Newren]

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

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

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

[Elijah Newren]

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 4adff2d538..0cb27c66e2 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 80d69d1401..d7f4cc80c1 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -29,6 +29,14 @@ struct merge_options {
struct string_list df_conflict_file_set;
 };
 
+struct dir_rename_entry {
+   struct hashmap_entry ent; /* must be the first member! */
+   char *dir;
+   unsigned non_unique_new_dir:1;
+   struct strbuf new_dir;
+   struct string_list possible_new_dirs;
+};
+
 /* merge_trees() but with recursive ancestor consolidation */
 int merge_recursive(struct merge_options *o,
struct commit *h1,
-- 
2.15.0.408.g850bc54b15

[PATCH v4 30/34] merge-recursive: fix overwriting dirty files involved in renames

[Elijah Newren]

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index e77d2b043d..2b8a5ca03b 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,
+   

[PATCH v4 27/34] merge-recursive: when comparing files, don't include trees

[Elijah Newren]

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

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

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

[PATCH v4 32/34] directory rename detection: new testcases showcasing a pair of bugs

[Elijah Newren]

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

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

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

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

[PATCH v4 21/34] merge-recursive: make a helper function for cleanup for handle_renames

[Elijah Newren]

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

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

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

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

[Elijah Newren]

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

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

[PATCH v4 16/34] merge-recursive: introduce new functions to handle rename logic

[Elijah Newren]

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

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

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

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

[PATCH v4 33/34] merge-recursive: avoid spurious rename/rename conflict from dir renames

[Elijah Newren]

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

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index fe42cabad0..d00786f719 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 5a750d5511..8ed85c79de 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2000,18 +2000,23 @@ test_expect_success '8b-check: Dual-directory rename, 
one into the others way, w
)
 '
 
-# Testcase 8c, rename+modify/delete
-#   (Related to testcases 5b and 8d)
+# Testcase 8c, modify/delete or rename+modify/delete?
+#   (Related to testcases 5b, 8d, and 9h)
 #   Commit O: z/{b,c,d}
 #   Commit A: y/{b,c}
 #   Commit B: z/{b,c,d_modified,e}
-#   Expected: y/{b,c,e}, CONFLICT(rename+modify/delete: x/d -> y/d or deleted)
+#   Expected: y/{b,c,e}, CONFLICT(modify/delete: on z/d)
 #
-#   Note: This testcase doesn't present any concerns for me...until you
-# compare it with testcases 5b and 8d.  See notes in 8d for more
-# details.
-
-test_expect_success '8c-setup: rename+modify/delete' '
+#   Note: It could easily be argued that the correct resolution here is
+# y/{b,c,e}, CONFLICT(rename/delete: z/d -> y/d vs deleted)
+# and that the modifed version of d should be present in y/ after
+# the merge, just marked as conflicted.  Indeed, I previously did
+# argue that.  But applying directory renames to the side of
+# history where a file is merely modified results in spurious
+# rename/rename(1to2) conflicts -- see testcase 9h.  See also
+# notes in 8d.
+
+test_expect_success '8c-setup: modify/delete or rename+modify/delete?' '
test_create_repo 8c &&
(
cd 8c &&
@@ -2044,29 +2049,29 @@ test_expect_success '8c-setup: rename+modify/delete' '
)
 '
 
-test_expect_success '8c-check: rename+modify/delete' '
+test_expect_success '8c-check: modify/delete or rename+modify/delete' '
(
cd 8c &&
 
git checkout A^0 &&
 
test_must_fail git merge -s recursive B^0 >out &&
-   test_i18ngrep "CONFLICT (rename/delete).* z/d.*y/d" out &&
+   test_i18ngrep "CONFLICT (modify/delete).* z/d" out &&
 
-   test 4 -eq $(git ls-files -s | wc -l) &&
-   test 1 -eq $(git ls-files -u | wc -l) &&
+   test 5 -eq $(git ls-files -s | wc -l) &&
+   test 2 -eq $(git ls-files -u | wc -l) &&
test 1 -eq $(git ls-files -o | wc -l) &&
 
git rev-parse >actual \
-   :0:y/b :0:y/c :0:y/e :3:y/d &&
+   :0:y/b :0:y/c :0:y/e :1:z/d :3:z/d &&
git rev-parse >expect \
-   O:z/b O:z/c B:z/e B:z/d &&
+   O:z/b O:z/c B:z/e O:z/d B:z/d &&
test_cmp expect actual &&
 
-   test_must_fail git rev-parse :1:y/d &&
-   test_must_fail git rev-parse :2:y/d &&
-   git ls-files -s y/d | grep ^100755 &&
-   test -f y/d
+   test_must_fail git rev-parse :2:z/d &&
+   git ls-files -s z/d | grep ^100755 &&
+   test -f z/d &&
+   ! test -f y/d
)
 '
 
@@ -2080,16 +2085,6 @@ test_expect_success '8c-check: rename+modify/delete' '
 #
 #   Note: It would also be somewhat reasonable to resolve this as
 # 

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

[Elijah Newren]

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

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

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

[Elijah Newren]

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

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

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

[Elijah Newren]

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

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

[PATCH v4 26/34] merge-recursive: check for file level conflicts then get new name

[Elijah Newren]

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

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

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

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

[Elijah Newren]

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

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

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

[Elijah Newren]

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

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index d00b274381..98c84e73d3 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1982,10 +1982,10 @@ int merge_trees(struct merge_options *o,
get_files_dirs(o, merge);
 
entries = get_unmerged();
-   record_df_conflict_files(o, entries);
re_head  = get_renames(o, head, common, head, merge, entries);
re_merge = get_renames(o, merge, common, head, merge, entries);
clean = process_renames(o, re_head, re_merge);
+   record_df_conflict_files(o, entries);
if (clean < 0)
goto cleanup;
for (i = entries->nr-1; 0 <= i; i--) {
-- 
2.15.0.408.g850bc54b15

[PATCH v4 25/34] merge-recursive: add computation of collisions due to dir rename & merging

[Elijah Newren]

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

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

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

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

[Elijah Newren]

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

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

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

[Elijah Newren]

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 2b8a5ca03b..fe42cabad0 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 9b16280d5e..f89cce4377 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3224,7 +3224,7 @@ test_expect_success '11b-setup: Avoid losing dirty file 
involved in directory re
)
 '
 
-test_expect_failure '11b-check: Avoid losing dirty file involved in directory 
rename' '
+test_expect_success '11b-check: Avoid losing dirty file involved in directory 
rename' '
(
cd 11b &&
 
@@ -3358,7 +3358,7 @@ test_expect_success '11d-setup: Avoid losing not-uptodate 
with rename + D/F conf
)
 '
 
-test_expect_failure '11d-check: Avoid losing not-uptodate with rename + D/F 
conflict' '
+test_expect_success '11d-check: Avoid losing not-uptodate with rename + D/F 
conflict' '
(
cd 11d &&
 
@@ -3432,7 +3432,7 @@ test_expect_success '11e-setup: Avoid deleting 
not-uptodate with dir rename/rena
)
 '
 
-test_expect_failure '11e-check: Avoid deleting not-uptodate with dir 
rename/rename(1to2)/add' '
+test_expect_success '11e-check: Avoid deleting not-uptodate with dir 
rename/rename(1to2)/add' '
(
cd 11e &&
 
@@ -3506,7 +3506,7 @@ test_expect_success '11f-setup: Avoid deleting 
not-uptodate with dir rename/rena
)
 '
 
-test_expect_failure '11f-check: Avoid deleting not-uptodate with dir 
rename/rename(2to1)' '
+test_expect_success '11f-check: Avoid deleting not-uptodate with dir 
rename/rename(2to1)' '
(
cd 11f &&
 
-- 
2.15.0.408.g850bc54b15

[PATCH v4 17/34] merge-recursive: fix leaks of allocated renames and diff_filepairs

[Elijah Newren]

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

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

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

[PATCH v4 23/34] merge-recursive: check for directory level conflicts

[Elijah Newren]

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

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

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

[PATCH v4 00/34] Add directory rename detection to git

[Elijah Newren]

This patchset introduces directory rename detection to merge-recursive.  See
  https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
for the first series (including design considerations, etc.), and follow-up
series can be found at
  https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/
  https://public-inbox.org/git/20171121080059.32304-1-new...@gmail.com/

Changes since V3:
  * Rebased on latest master (no conflicts, but figured I might as well)
  * Addressed issues mentioned in reviews of V3:
* Fixed style issues
* Made use of strbuf (note: new function added to strbuf.[ch] that
  takes a string_list*; is that okay?)
* Switched strndup to xstrndup for windows
  * Miscellaneous cleanups, perf, etc.:
* Skip pair early in get_directory_renames() if pair is not a rename
* Add helper cleanup functions to consolidate some code
* Make it clear that directory rename detection only operates on
  add ('A') and rename ('R') filepairs

Full tbdiff against V3:
 1: a0abcb1378 =  1: d95d5fffea Tighten and correct a few testcases for merging 
and cherry-picking
 2: e8745c4f1b =  2: b177dccc28 merge-recursive: fix logic ordering issue
 3: 0ae4156994 =  3: fe3de710e3 merge-recursive: add explanation for src_entry 
and dst_entry
 4: f13686ea6a =  4: 19b82b795a directory rename detection: basic testcases
 5: 16e7dea569 =  5: 3d23af19e9 directory rename detection: directory splitting 
testcases
 6: 51d9a5b059 =  6: d997681a4d directory rename detection: testcases to avoid 
taking detection too far
 7: 4b39f6bea3 =  7: 152e3d5d81 directory rename detection: partially renamed 
directory testcase/discussion
 8: 0668e65864 =  8: 441211c842 directory rename detection: files/directories 
in the way of some renames
 9: fcce655243 =  9: c66d3fb8b4 directory rename detection: testcases checking 
which side did the rename
10: 04213cc213 = 10: 4542c5fee4 directory rename detection: more involved 
edge/corner testcases
11: 21992300aa ! 11: 771b551982 directory rename detection: testcases exploring 
possibly suboptimal merges
@@ -36,7 +36,7 @@
 +#   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 previously, but the
++# 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.
 +
@@ -321,9 +321,11 @@
 +#   Commit B: w/{b,c}, z/d
 +#
 +# Possible Resolutions:
-+#   Previous git: z/d, CONFLICT(z/b -> y/b vs. w/b), CONFLICT(z/c -> y/c 
vs. w/c)
-+#   Expected: y/d, CONFLICT(z/b -> y/b vs. w/b), CONFLICT(z/c -> y/c 
vs. w/c)
-+#   Preferred:??
++#   w/o dir-rename detection: z/d, CONFLICT(z/b -> y/b vs. w/b),
++#  CONFLICT(z/c -> y/c vs. w/c)
++#   Currently expected:   y/d, CONFLICT(z/b -> y/b vs. w/b),
++#  CONFLICT(z/c -> y/c vs. w/c)
++#   Optimal:  ??
 +#
 +# Notes: In commit A, directory z got renamed to y.  In commit B, 
directory z
 +#did NOT get renamed; the directory is still present; instead it 
is
12: 727238bf64 = 12: f1f8b2e0bc directory rename detection: miscellaneous 
testcases to complete coverage
13: 44b25c31e2 = 13: 595905c828 directory rename detection: tests for handling 
overwriting untracked files
14: fa05a0240c = 14: 71ae85d203 directory rename detection: tests for handling 
overwriting dirty files
15: 131af0e3f2 = 15: e9ca40cf6a merge-recursive: move the get_renames() function
16: c7d1d15bec = 16: f4ff2a3c8b merge-recursive: introduce new functions to 
handle rename logic
17: 1ca5c22551 = 17: 84b57b2495 merge-recursive: fix leaks of allocated renames 
and diff_filepairs
18: 57288cf7b1 = 18: 03c1d06592 merge-recursive: make !o->detect_rename 
codepath more obvious
19: fd6602a327 = 19: cf9d708f1d merge-recursive: split out code for determining 
diff_filepairs
20: 8828dd4093 ! 20: ed33e1221f merge-recursive: add a new hashmap for storing 
directory renames
@@ -45,7 +45,7 @@
 +  hashmap_entry_init(entry, strhash(directory));
 +  entry->dir = directory;
 +  entry->non_unique_new_dir = 0;
-+  entry->new_dir = NULL;
++  strbuf_init(>new_dir, 0);
 +  string_list_init(>possible_new_dirs, 0);
 +}
 +
@@ -64,7 +64,7 @@
 +  struct hashmap_entry ent; /* must be the first member! */
 +  char *dir;
 +  unsigned non_unique_new_dir:1;
-+  char *new_dir;
++  struct strbuf new_dir;
 +  struct string_list possible_new_dirs;
 +};
 +
--:  --- > 21: 9b88f735c2 merge-recursive: make a helper function for 
cleanup for handle_renames
21: f1d3b1f7ab ! 22: 20c94136af merge-recursive: add get_directory_renames()
@@ -63,8 +63,8 @@
 +  new_len = end_of_new - 

[PATCH v4 28/34] merge-recursive: apply necessary modifications for directory renames

[Elijah Newren]

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

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

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

[PATCH v4 12/34] directory rename detection: miscellaneous testcases to complete coverage

[Elijah Newren]

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

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

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

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

[Elijah Newren]

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

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

[PATCH v4 18/34] merge-recursive: make !o->detect_rename codepath more obvious

[Elijah Newren]

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index cdd0afa047..da7c67eb82 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1329,8 +1329,6 @@ static struct string_list *get_renames(struct 
merge_options *o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
-   if (!o->detect_rename)
-   return renames;
 
diff_setup();
opts.flags.recursive = 1;
@@ -1648,6 +1646,12 @@ static int handle_renames(struct merge_options *o,
  struct string_list *entries,
  struct rename_info *ri)
 {
+   ri->head_renames = NULL;
+   ri->merge_renames = NULL;
+
+   if (!o->detect_rename)
+   return 1;
+
ri->head_renames  = get_renames(o, head, common, head, merge, entries);
ri->merge_renames = get_renames(o, merge, common, head, merge, entries);
return process_renames(o, ri->head_renames, ri->merge_renames);
@@ -1658,6 +1662,9 @@ static void cleanup_rename(struct string_list *rename)
const struct rename *re;
int i;
 
+   if (rename == NULL)
+   return;
+
for (i = 0; i < rename->nr; i++) {
re = rename->items[i].util;
diff_free_filepair(re->pair);
-- 
2.15.0.408.g850bc54b15

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

[Elijah Newren]

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

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

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

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

[Elijah Newren]

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index c5932d5c57..6aef357e78 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1384,6 +1384,138 @@ static struct diff_queue_struct *get_diffpairs(struct 
merge_options *o,
return ret;
 }
 
+static void get_renamed_dir_portion(const char *old_path, const char *new_path,
+   char **old_dir, char **new_dir)
+{
+   char *end_of_old, *end_of_new;
+   int old_len, new_len;
+
+   *old_dir = NULL;
+   *new_dir = NULL;
+
+   /* For
+*"a/b/c/d/foo.c" -> "a/b/something-else/d/foo.c"
+* the "d/foo.c" part is the same, we just want to know that
+*"a/b/c" was renamed to "a/b/something-else"
+* so, for this example, this function returns "a/b/c" in
+* *old_dir and "a/b/something-else" in *new_dir.
+*
+* Also, if the basename of the file changed, we don't care.  We
+* want to know which portion of the directory, if any, changed.
+*/
+   end_of_old = strrchr(old_path, '/');
+   end_of_new = strrchr(new_path, '/');
+
+   if (end_of_old == NULL || end_of_new == NULL)
+   return;
+   while (*--end_of_new == *--end_of_old &&
+  end_of_old != old_path &&
+  end_of_new != new_path)
+   ; /* Do nothing; all in the while loop */
+   /*
+* We've found the first non-matching character in the directory
+* paths.  That means the current directory we were comparing
+* represents the rename.  Move end_of_old and end_of_new back
+* to the full directory name.
+*/
+   if (*end_of_old == '/')
+   end_of_old++;
+   if (*end_of_old != '/')
+   end_of_new++;
+   end_of_old = strchr(end_of_old, '/');
+   end_of_new = strchr(end_of_new, '/');
+
+   /*
+* It may have been the case that old_path and new_path were the same
+* directory all along.  Don't claim a rename if they're the same.
+*/
+   old_len = end_of_old - old_path;
+   new_len = end_of_new - new_path;
+
+   if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
+   *old_dir = xstrndup(old_path, old_len);
+   *new_dir = xstrndup(new_path, new_len);
+   }
+}
+
+static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
+struct tree *tree)
+{
+   struct hashmap *dir_renames;
+   struct hashmap_iter iter;
+   struct dir_rename_entry *entry;
+   int i;
+
+   dir_renames = malloc(sizeof(struct hashmap));
+   dir_rename_init(dir_renames);
+   for (i = 0; i < pairs->nr; ++i) {
+   struct string_list_item *item;
+   int *count;
+   struct diff_filepair *pair = pairs->queue[i];
+   char *old_dir, *new_dir;
+
+   /* File not part of directory rename if it wasn't renamed */
+   if (pair->status != 'R')
+   continue;
+
+   get_renamed_dir_portion(pair->one->path, pair->two->path,
+   _dir,_dir);
+   if (!old_dir)
+   /* Directory didn't change at all; ignore this one. */
+   continue;
+
+   entry = dir_rename_find_entry(dir_renames, old_dir);
+   if (!entry) {
+   entry = xmalloc(sizeof(struct dir_rename_entry));
+   dir_rename_entry_init(entry, old_dir);
+   hashmap_put(dir_renames, entry);
+   } else {
+   free(old_dir);
+   }
+   item = string_list_lookup(>possible_new_dirs, new_dir);
+   if (!item) {
+   item = string_list_insert(>possible_new_dirs,
+ new_dir);
+   item->util = xcalloc(1, sizeof(int));
+   } else {
+   free(new_dir);
+   }
+   count = item->util;
+   *count += 1;
+   }
+
+   hashmap_iter_init(dir_renames, );
+   while ((entry = hashmap_iter_next())) {
+   int max = 0;
+   int bad_max = 0;
+   char *best = NULL;
+
+   for (i = 0; i < entry->possible_new_dirs.nr; i++) {
+   int *count = entry->possible_new_dirs.items[i].util;
+
+   if (*count == max)
+   bad_max = max;
+   else if (*count > max) {
+   

[PATCH v4 19/34] merge-recursive: split out code for determining diff_filepairs

[Elijah Newren]

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

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

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

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

[Elijah Newren]

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

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

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

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

[Elijah Newren]

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

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

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

Question regarding "next" merge

[Dan Jacques]

Junio,

I read the "what's cooking in Git" notes and saw that you were intending to
introduce this patch set into "next". Johannes pointed out some quoting errors
that break Windows builds, and I have incorporated fixes in my working copy.

I was going to hold off on publishing v4 in case some of the other reviewers
had additional comments, but if you would prefer, I can publish them now
before you merge, so "next" doesn't break on Windows.

Please advise! Thanks,
-Dan

Re: [PATCH] repository: fix a sparse 'using integer as NULL pointer' warning

[brian m. carlson]

On Tue, Nov 28, 2017 at 03:01:19AM +, Ramsay Jones wrote:
> 
> Commit 78a6766802 ("Integrate hash algorithm support with repo setup",
> 2017-11-12) added a 'const struct git_hash_algo *hash_algo' field to the
> repository structure, without modifying the initializer of the 'the_repo'
> variable. This does not actually introduce a bug, since the '0' initializer
> for the 'ignore_env:1' bit-field is interpreted as a NULL pointer (hence
> the warning), and the final field (now with no initializer) receives a
> default '0'.
> 
> Signed-off-by: Ramsay Jones 
> ---
> 
> Hi Junio,
> 
> I don't recall Brian doing a re-roll of the 'bc/hash-algo' branch[1], but
> now that it has been merged into the 'next' branch, sparse is barking on
> that branch too. This patch (slightly different to the last one) applies
> on top of 'next'.

Thanks for the patch; it's obviously correct.  I'll get sparse set up
for future patch series.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

What's cooking in git.git (Nov 2017, #08; Tue, 28)

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

Git 2.15.1 has been tagged, with many of the fixes that have already
been in 'master'.

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

* ew/rebase-mboxrd (2017-11-18) 1 commit
  (merged to 'next' on 2017-11-21 at 88eaaac334)
 + rebase: use mboxrd format to avoid split errors

 When "git rebase" prepared an mailbox of changes and fed it to "git
 am" to replay them, it was confused when a stray "From " happened
 to be in the log message of one of the replayed changes.  This has
 been corrected.


* jc/branch-name-sanity (2017-11-15) 4 commits
  (merged to 'next' on 2017-11-20 at 7236b0dde3)
 + builtin/branch: remove redundant check for HEAD
 + branch: correctly reject refs/heads/{-dash,HEAD}
 + branch: split validate_new_branchname() into two
 + branch: streamline "attr_only" handling in validate_new_branchname()

 "git branch" and "git checkout -b" are now forbidden from creating
 a branch whose name is "HEAD".


* jc/ignore-cr-at-eol (2017-11-08) 2 commits
  (merged to 'next' on 2017-11-20 at 9f7bae7cbd)
 + diff: --ignore-cr-at-eol
 + xdiff: reassign xpparm_t.flags bits

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


* jc/merge-base-fork-point-doc (2017-11-09) 1 commit
  (merged to 'next' on 2017-11-20 at 199b9255b4)
 + merge-base --fork-point doc: clarify the example and failure modes

 Clarify and enhance documentation for "merge-base --fork-point", as
 it was clear what it computed but not why/what for.


* ks/rebase-no-git-foo (2017-11-21) 1 commit
  (merged to 'next' on 2017-11-22 at 336552dbb7)
 + git-rebase: clean up dashed-usages in messages

 Mentions of "git-rebase" and "git-am" (dashed form) still remained
 in end-user visible strings emitted by the "git rebase" command;
 they have been corrected.


* ma/branch-list-paginate (2017-11-20) 3 commits
  (merged to 'next' on 2017-11-22 at eecd7f2369)
 + branch: change default of `pager.branch` to "on"
 + branch: respect `pager.branch` in list-mode only
 + t7006: add tests for how git branch paginates

 "git branch --list" learned to show its output through the pager by
 default when the output is going to a terminal, which is controlled
 by the pager.branch configuration variable.  This is similar to a
 recent change to "git tag --list".


* pw/sequencer-recover-from-unlockable-index (2017-11-16) 1 commit
  (merged to 'next' on 2017-11-20 at 36f0ed26ce)
 + sequencer: reschedule pick if index can't be locked

 The sequencer machinery (used by "git cherry-pick A..B", and "git
 rebase -i", among other things) would have lost a commit if stopped
 due to an unlockable index file, which has been fixed.


* rs/apply-inaccurate-eof-with-incomplete-line (2017-11-17) 1 commit
  (merged to 'next' on 2017-11-20 at c8aee1d135)
 + apply: update line lengths for --inaccurate-eof

 "git apply --inaccurate-eof" when used with "--ignore-space-change"
 triggered an internal sanity check, which has been fixed.


* rs/config-write-section-fix (2017-11-18) 1 commit
  (merged to 'next' on 2017-11-21 at a7f3f93e0c)
 + config: flip return value of write_section()

 There was a recent semantic mismerge in the codepath to write out a
 section of a configuration section, which has been corrected.


* rs/include-comments-before-the-function-header (2017-11-21) 6 commits
  (merged to 'next' on 2017-11-22 at d7255a836f)
 + grep: show non-empty lines before functions with -W
 + grep: update boundary variable for pre-context
 + t7810: improve check of -W with user-defined function lines
 + xdiff: show non-empty lines before functions with -W
 + xdiff: factor out is_func_rec()
 + t4051: add test for comments preceding function lines

 "git grep -W", "git diff -W" and their friends learned a heuristic
 to extend a pre-context beyond the line that matches the "function
 pattern" (aka "diff.*.xfuncname") to include a comment block, if
 exists, that immediately precedes it.


* rv/sendemail-tocmd-in-config-and-completion (2017-11-14) 2 commits
  (merged to 'next' on 2017-11-20 at 75cf6e2fc3)
 + completion: add git config sendemail.tocmd
 + Documentation/config: add sendemail.tocmd to list preceding "See 
git-send-email(1)"

 Teach "sendemail.tocmd" to places that know about "sendemail.to",
 like documentation and shell completion (in contrib/).


* sb/test-cherry-pick-submodule-getting-in-a-way (2017-11-15) 2 commits
  (merged to 'next' on 2017-11-20 at fe7016689e)
 + merge-recursive: handle addition of submodule on our side of history
 + t/3512: demonstrate unrelated 

A note from the maintainer

[Junio C Hamano]

Welcome to the Git development community.

This message is written by the maintainer and talks about how Git
project is managed, and how you can work with it.

* Mailing list and the community

The development is primarily done on the Git mailing list. Help
requests, feature proposals, bug reports and patches should be sent to
the list address .  You don't have to be
subscribed to send messages.  The convention on the list is to keep
everybody involved on Cc:, so it is unnecessary to say "Please Cc: me,
I am not subscribed".

As an anti-spam measure, the mailing list software rejects messages
that are not text/plain and drops them on the floor.  If you are a
GMail user, you'd want to make sure "Plain text mode" is checked.

Before sending patches, please read Documentation/SubmittingPatches
and Documentation/CodingGuidelines to familiarize yourself with the
project convention.

If you sent a patch and you did not hear any response from anybody for
several days, it could be that your patch was totally uninteresting,
but it also is possible that it was simply lost in the noise.  Please
do not hesitate to send a reminder message in such a case.  Messages
getting lost in the noise may be a sign that those who can evaluate
your patch don't have enough mental/time bandwidth to process them
right at the moment, and it often helps to wait until the list traffic
becomes calmer before sending such a reminder.

The list archive is available at a few public sites:

http://public-inbox.org/git/
http://marc.info/?l=git
http://www.spinics.net/lists/git/

For those who prefer to read it over NNTP:

nntp://news.public-inbox.org/inbox.comp.version-control.git
nntp://news.gmane.org/gmane.comp.version-control.git

are available.

When you point at a message in a mailing list archive, using its
message ID is often the most robust (if not very friendly) way to do
so, like this:


http://public-inbox.org/git/pine.lnx.4.58.0504150753440.7...@ppc970.osdl.org

Often these web interfaces accept the message ID with enclosing <>
stripped (like the above example to point at one of the most important
message in the Git list).

Some members of the development community can sometimes be found on
the #git and #git-devel IRC channels on Freenode.  Their logs are
available at:

http://colabti.org/irclogger/irclogger_log/git
http://colabti.org/irclogger/irclogger_log/git-devel

There is a volunteer-run newsletter to serve our community ("Git Rev
News" http://git.github.io/rev_news/rev_news.html).

Git is a member project of software freedom conservancy, a non-profit
organization (https://sfconservancy.org/).  To reach a committee of
liaisons to the conservancy, contact them at .


* Reporting bugs

When you think git does not behave as you expect, please do not stop
your bug report with just "git does not work".  "I used git in this
way, but it did not work" is not much better, neither is "I used git
in this way, and X happend, which is broken".  It often is that git is
correct to cause X happen in such a case, and it is your expectation
that is broken. People would not know what other result Y you expected
to see instead of X, if you left it unsaid.

Please remember to always state

 - what you wanted to achieve;

 - what you did (the version of git and the command sequence to reproduce
   the behavior);

 - what you saw happen (X above);

 - what you expected to see (Y above); and

 - how the last two are different.

See http://www.chiark.greenend.org.uk/~sgtatham/bugs.html for further
hints.

If you think you found a security-sensitive issue and want to disclose
it to us without announcing it to wider public, please contact us at
our security mailing list .  This is
a closed list that is limited to people who need to know early about
vulnerabilities, including:

  - people triaging and fixing reported vulnerabilities
  - people operating major git hosting sites with many users
  - people packaging and distributing git to large numbers of people

where these issues are discussed without risk of the information
leaking out before we're ready to make public announcements.


* Repositories and documentation.

My public git.git repositories are at:

  git://git.kernel.org/pub/scm/git/git.git/
  https://kernel.googlesource.com/pub/scm/git/git
  git://repo.or.cz/alt-git.git/
  https://github.com/git/git/
  git://git.sourceforge.jp/gitroot/git-core/git.git/
  git://git-core.git.sourceforge.net/gitroot/git-core/git-core/

This one shows not just the main integration branches, but also
individual topics broken out:

  git://github.com/gitster/git/

A few web interfaces are found at:

  http://git.kernel.org/cgit/git/git.git
  https://kernel.googlesource.com/pub/scm/git/git
  http://repo.or.cz/w/alt-git.git

Preformatted documentation from the tip of the "master" branch can be
found in:

  

[ANNOUNCE] Git v2.15.1

[Junio C Hamano]

The latest maintenance release Git v2.15.1 is now available at
the usual places.

The tarballs are found at:

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

The following public repositories all have a copy of the 'v2.15.1'
tag and the 'maint' 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



Git v2.15.1 Release Notes
=

Fixes since v2.15
-

 * TravisCI build updates.

 * "auto" as a value for the columnar output configuration ought to
   judge "is the output consumed by humans?" with the same criteria as
   "auto" for coloured output configuration, i.e. either the standard
   output stream is going to tty, or a pager is in use.  We forgot the
   latter, which has been fixed.

 * The experimental "color moved lines differently in diff output"
   feature was buggy around "ignore whitespace changes" edges, which
   has been corrected.

 * Instead of using custom line comparison and hashing functions to
   implement "moved lines" coloring in the diff output, use the pair
   of these functions from lower-layer xdiff/ code.

 * Some codepaths did not check for errors when asking what branch the
   HEAD points at, which have been fixed.

 * "git commit", after making a commit, did not check for errors when
   asking on what branch it made the commit, which has been corrected.

 * "git status --ignored -u" did not stop at a working tree of a
   separate project that is embedded in an ignored directory and
   listed files in that other project, instead of just showing the
   directory itself as ignored.

 * A broken access to object databases in recent update to "git grep
   --recurse-submodules" has been fixed.

 * A recent regression in "git rebase -i" that broke execution of git
   commands from subdirectories via "exec" instruction has been fixed.

 * "git check-ref-format --branch @{-1}" bit a "BUG()" when run
   outside a repository for obvious reasons; clarify the documentation
   and make sure we do not even try to expand the at-mark magic in
   such a case, but still call the validation logic for branch names.

 * Command line completion (in contrib/) update.

 * Description of blame.{showroot,blankboundary,showemail,date}
   configuration variables have been added to "git config --help".

 * After an error from lstat(), diff_populate_filespec() function
   sometimes still went ahead and used invalid data in struct stat,
   which has been fixed.

 * UNC paths are also relevant in Cygwin builds and they are now
   tested just like Mingw builds.

 * Correct start-up sequence so that a repository could be placed
   immediately under the root directory again (which was broken at
   around Git 2.13).

 * The credential helper for libsecret (in contrib/) has been improved
   to allow possibly prompting the end user to unlock secrets that are
   currently locked (otherwise the secrets may not be loaded).

 * Updates from GfW project.

 * "git rebase -i" recently started misbehaving when a submodule that
   is configured with 'submodule..ignore' is dirty; this has
   been corrected.

 * Some error messages did not quote filenames shown in it, which have
   been fixed.

 * Building with NO_LIBPCRE1_JIT did not disable it, which has been fixed.

 * We used to add an empty alternate object database to the system
   that does not help anything; it has been corrected.

 * Error checking in "git imap-send" for empty response has been
   improved.

 * An ancient bug in "git apply --ignore-space-change" codepath has
   been fixed.

 * There was a recent semantic mismerge in the codepath to write out a
   section of a configuration section, which has been corrected.

Also contains various documentation updates and code clean-ups.



Changes since v2.15.0 are as follows:

Adam Dinwoodie (3):
  t5580: add Cygwin support
  rebase -i: fix comment typo
  doc/SubmittingPatches: correct subject guidance

Andrey Okoshkin (2):
  commit: check result of resolve_ref_unsafe
  diff: fix lstat() error handling in diff_populate_filespec()

Brandon Williams (1):
  wt-status: actually ignore submodules when requested

Carlos Martín Nieto (1):
  diff: --indent-heuristic is no longer experimental

Charles Bailey (2):
  t4201: make use of abbreviation in the test more robust
  grep: fix NO_LIBPCRE1_JIT to fully disable JIT

Dennis Kaarsemaker (1):
  credential-libsecret: unlock locked secrets

Jacob Keller (1):
  sequencer: pass absolute GIT_DIR to exec commands

Jakub Bereżański (2):
  t0302: check helper can handle empty credentials
  wincred: handle empty username/password correctly

Jean Carlo Machado (1):
  fix typos in 2.15.0 release notes

Jeff King (11):
  t4015: refactor --color-moved 

Re: bug deleting "unmerged" branch (2.12.3)

[Junio C Hamano]

"Ulrich Windl"  writes:

> I think if more than one branches are pointing to the same commit,
> one should be allowed to delete all but the last one without
> warning. Do you agree?

That comes from a viewpoint that the only purpose "branch -d" exists
in addition to "branch -D" is to protect objects from "gc".  Those
who added the safety feature may have shared that view originally,
but it turns out that it protects another important thing you are
forgetting.

Imagine that two topics, 'topicA' and 'topicB', were independently
forked from 'master', and then later we wanted to add a feature that
depends on these two topics.  Since the 'feature' forked, there may
have been other developments, and we ended up in this topology:

---o---o---o---o---o---M
\   \  
 \   o---A---o---F
  \ /  
   o---o---o---o---B

where A, B and F are the tips of 'topicA', 'topicB' and 'feature'
branches right now [*1*].

Now imagine we are on 'master' and just made 'topicB' graduate.  We
would have this topology.

---o---o---o---o---o---o---M
\   \ /
 \   o---A---o---F   /
  \ /   /
   o---o---o---o---B

While we do have 'topicA' and 'feature' branches still in flight,
we are done with 'topicB'.  Even though the tip of 'topicA' is
reachable from the tip of 'feature', the fact that the branch points
at 'A' is still relevant.  If we lose that information right now,
we'd have to go find it when we (1) want to further enhance the
topic by checking out and building on 'topicA', and (2) want to
finally get 'topicA' graduate to 'master'.

Because removal of a topic (in this case 'topicB') is often done
after a merge of that topic is made into an integration branch,
"branch -d" that protects branches that are yet to be merged to the
current branch catches you if you said "branch -d topic{A,B}" (or
other equivalent forms, most likely you'd have a script that spits
out list of branches and feed it to "xargs branch -d").

So, no, I do not agree.


[Footnotes]

*1* Since the 'feature' started developing, there were a few commits
added to 'topicB' but because the feature does not depend on
these enhancements to that topic, B is ahead of the commit that
was originally merged with the tip of 'topicA' to form the
'feature' branch.

[PATCH] pretty: fix buffer over-read with %> and %

[mwnx]

A buffer over-read of the format string would occur with unterminated
formats of the form '%>(#' and '%<(#', where '#' represents a number.

This error can be witnessed by running git log under valgrind like so:

valgrind git log -n1 --format='%<(42'

This was due to the fact that the "not found" case for strcspn() was
being checked in the same way that one checks the "not found" case for
strchr(), i.e. by checking for a NULL pointer return value. Instead, one
must check for the end of the string since strcspn() points to the
character where it finished its search (which will be a '\0' if
unsuccessful).

Signed-off-by: Maxwell Nixie 
---
Hope I got everything right.

 pretty.c  | 2 +-
 t/t4205-log-pretty-formats.sh | 6 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 2f6b0ae6c..4c70bad45 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
const char *end = start + strcspn(start, ",)");
char *next;
int width;
-   if (!end || end == start)
+   if (!*end || end == start)
return 0;
width = strtol(start, , 10);
if (next == start || width == 0)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 591f35daa..4d9555962 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' '
test_cmp expect actual
 '
 
+test_expect_success 'unterminated alignment formatting' '
+   git log -n1 --format="%<(42" >actual &&
+   echo "%<(42" >expected &&
+   test_cmp expected actual
+'
+
 test_done
-- 
2.15.0.319.gb3398b820

Re: [PATCH v2 0/3] rebase: give precise error message

[Junio C Hamano]

Kaartic Sivaraam  writes:

> Something like the following diff with additional changes to other
> places that refer to ,
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 67d48e688..ba4a545bf 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -9,9 +9,9 @@ SYNOPSIS
>  
>  [verse]
>  'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
> -   [ []]
> +   [ []]
>  'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
> -   --root []
> +   --root []
>  'git rebase' --continue | --skip | --abort | --quit | --edit-todo
>  
>  DESCRIPTION
>
>
> If  is the correct substitute , I could try to send a
> patch that fixes this.

I do not think the above is a good change in the first place for at
least two reasons.  By saying , the updated text says "not just
branches but you can also give tags and remote-tracking branches".
In reality, however, it can take any commit-ish, as the "no we are
not rebasing the current checkout" form of the command is merely a
short-hand to check it out first [*1*].  It gives appearance that
the change is making it more broad, but not making it broad enough.
At the same time, more than 90% of the time, people give a branch
name there.  Saying "branch-or-commit" for a short description of
the command line (which is what synopsis section is) does not add
that much value.  The body text of the description where we refer to
the  parameter of course need to be updated to say "in
addition, instead of a branch name, you can give a commit-ish that
is *not* a branch name.  When you do so, instead of checking out the
branch, the HEAD is detached at that commit before the history
leading to it is rebased."

And because we have to say "it can be a non-branch commit-ish and
here is what happens when you do so" anyway, I think the current
synopsis as-is would be better than making it less clear and more
scary by replacing  with other things like  or
[ | ].


[Footnote]

*1* As my "log --first-parent --oneline master..pu" is a strand of
pearls each of which is a merge of a topic branch,

$ git rebase -i master pu~$n^2

can be a handy way to make a trial rebase (after double checking
the result of "git tbdiff", I may do "git checkout -B topic" to
overwrite the original or I may discard the result of rebasing).

Re: [PATCH] travis-ci: avoid new tcl/tk build requirement

[Junio C Hamano]

Todd Zullinger  writes:

> I pulled the travis docker image used in clang/gcc builds[1] and can
> see it has both tcl and tk packages installed.  The linux32 builds use
> a docker image[2] which does not contain tcl or tk.
>
> [1] travisci/ci-garnet:packer-1503972846
> [2] daald/ubuntu32:xenial
>
> If we wanted, we could set BYPASS_TCLTK_CHECK only for the linux32
> builds.  I think it's probably better to do it globally rather than
> rely on the travis containers implicitly having tcl/tk installed.
>
>> The patch looks good to me. Thanks!
>>
>> I wonder if it would be better to squash it into my patch or to keep
>> it separate. I am ok with both ways.
>
> I fine either way too.  This is still just on pu, so squashing it in
> seems like the way to go.  I only made it separate to cause travis to
> run the tests. :)

I am OK either way but this may be giving an early warning that a
typical mix of packages installed on a box will lead many people to
encounter this glitch in the real world, and we may need a more
seamless solution than a workaround.

Keeping this "fix" separate from the original commit that introduced
the issue would allow us more easily to refer real users who will be
hit by this glitch to it, telling them to do something similar to
their build environment.

[PATCH] pathspec: only match across submodule boundaries when requested

[Brandon Williams]

Commit 74ed43711fd (grep: enable recurse-submodules to work on 
objects, 2016-12-16) taught 'tree_entry_interesting()' to be able to
match across submodule boundaries in the presence of wildcards.  This is
done by performing literal matching up to the first wildcard and then
punting to the submodule itself to perform more accurate pattern
matching.  Instead of introducing a new flag to request this behavior,
commit 74ed43711fd overloaded the already existing 'recursive' flag in
'struct pathspec' to request this behavior.

This leads to a bug where whenever any other caller has the 'recursive'
flag set as well as a pathspec with wildcards that all submodules will
be indicated as matches.  One simple example of this is:

git init repo
cd repo

git init submodule
git -C submodule commit -m initial --allow-empty

touch "[bracket]"
git add "[bracket]"
git commit -m bracket
git add submodule
git commit -m submodule

git rev-list HEAD -- "[bracket]"

Fix this by introducing the new flag 'recurse_submodules' in 'struct
pathspec' and using this flag to determine if matches should be allowed
to cross submodule boundaries.

Signed-off-by: Brandon Williams 
---
 builtin/grep.c|  1 +
 pathspec.h|  1 +
 t/t4208-log-magic-pathspec.sh | 17 +
 tree-walk.c   |  5 +++--
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 5a6cfe6b4..3ca4ac80d 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1015,6 +1015,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
   prefix, argv + i);
pathspec.max_depth = opt.max_depth;
pathspec.recursive = 1;
+   pathspec.recurse_submodules = !!recurse_submodules;
 
 #ifndef NO_PTHREADS
if (list.nr || cached || show_in_pager)
diff --git a/pathspec.h b/pathspec.h
index 6420d1080..099a170c2 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -24,6 +24,7 @@ struct pathspec {
int nr;
unsigned int has_wildcard:1;
unsigned int recursive:1;
+   unsigned int recurse_submodules:1;
unsigned magic;
int max_depth;
struct pathspec_item {
diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
index 935df6a65..bd583af0e 100755
--- a/t/t4208-log-magic-pathspec.sh
+++ b/t/t4208-log-magic-pathspec.sh
@@ -93,4 +93,21 @@ test_expect_success 'command line pathspec parsing for "git 
log"' '
git log --merge -- a
 '
 
+test_expect_success 'tree_entry_interesting does not match past submodule 
boundaries' '
+   test_when_finished "rm -rf repo submodule" &&
+   git init submodule &&
+   test_commit -C submodule initial &&
+   git init repo &&
+   >"repo/[bracket]" &&
+   git -C repo add "[bracket]" &&
+   git -C repo commit -m bracket &&
+   git -C repo rev-list HEAD -- "[bracket]" >expect &&
+
+   git -C repo submodule add ../submodule &&
+   git -C repo commit -m submodule &&
+
+   git -C repo rev-list HEAD -- "[bracket]" >actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/tree-walk.c b/tree-walk.c
index 684f0e337..63a87ed66 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -1011,7 +1011,8 @@ static enum interesting do_match(const struct name_entry 
*entry,
 * character.  More accurate matching can then
 * be performed in the submodule itself.
 */
-   if (ps->recursive && S_ISGITLINK(entry->mode) &&
+   if (ps->recurse_submodules &&
+   S_ISGITLINK(entry->mode) &&
!ps_strncmp(item, match + baselen,
entry->path,
item->nowildcard_len - baselen))
@@ -1060,7 +1061,7 @@ static enum interesting do_match(const struct name_entry 
*entry,
 * character.  More accurate matching can then
 * be performed in the submodule itself.
 */
-   if (ps->recursive && S_ISGITLINK(entry->mode) &&
+   if (ps->recurse_submodules && S_ISGITLINK(entry->mode) &&
!ps_strncmp(item, match, base->buf + base_offset,
item->nowildcard_len)) {
strbuf_setlen(base, base_offset + baselen);
-- 
2.15.0.531.g2ccb3012c9-goog

Re: Bug in pathspec handling (in conjunction with submodules)

[Brandon Williams]

On 11/26, Johannes Schindelin wrote:
> Hi Duy & Brandon,
> 
> in 74ed43711fd (grep: enable recurse-submodules to work on  objects,
> 2016-12-16), the do_match() function in tree-walk.c was changed so that it
> can recurse across submodule boundaries.
> 
> However, there is a bug, and I *think* there may be two bugs actually. Or
> even three.
> 
> First of all, here is an MCVE that I distilled from
> https://github.com/git-for-windows/git/issues/1371:
> 
>   git init repo
>   cd repo
> 
>   git init submodule
>   git -C submodule commit -m initial --allow-empty
> 
>   touch "[bracket]"
>   git add "[bracket]"
>   git commit -m bracket
>   git add submodule
>   git commit -m submodule
> 
>   git rev-list HEAD -- "[bracket]"
> 
> Nothing fancy, just adding a file with brackets in the name, then a
> submodule, then showing the commit history filtered by the funny file
> name.
> 
> However, the log prints *both* commits. Clearly the submodule commit
> should *not* be shown.
> 
> Now, how does this all happen?
> 
> Since the pathspec contains brackets, parse_pathspec() marks it as
> containing wildcards and sets nowildcard_len to 0.
> 
> Now, note that [bracket] *is* a wildcard expression: it should only match
> a single character that is one of  a, b, c, e, k, r or t.
> 
> I think this is the first bug: `git rev-list` should not even match the
> commit that adds the file [bracket] because its file name does not match
> that expression. From where I sit, it would appear that f1a2ddbbc2d
> (tree_entry_interesting(): optimize wildcard matching when base is
> matched, 2010-12-15) simply added the fnmatch() code without disabling the
> literal match_entry() code when the pathspec contains a pattern.

I can see both sides to this, wanting to try matching literally first
and then trying the wildcards, so I don't really have an opinion on
how/if we should fix that.

> 
> But it does not stop there: there is *another* bug which causes the
> pattern to somehow match the submodule. I *guess* the idea of
> https://github.com/git/git/commit/74ed43711#diff-7a08243175f2cae66aedf53f7dce3bdfR1015
> was to allow a pattern like *.c to match files in a submodule, but the
> pattern [bracket] should not match any file in submodule/. I think that
> that code needs to be a little bit more careful to try to match the
> submodule's name against the pattern (it seems to interpret nowildcard_len
> == 0 to mean that the wildcard is `*`).

This is a much bigger issue and I'm surprised it took this long to find
this bug.  And of course its due to one of my earlier contributions to
the project :)

> 
> However, the commit introducing that code wanted to teach *grep* (not
> *rev-list*) a new trick, and it relies on the `recursive` flag of the
> pathspec to be set.

This is the root cause of the bug.  The added code to match against
submodules was intended to allow for matching past submodule boundaries
for those commands (like grep) which are recursing submodules.  So
really there should be an additional flag which is passed in to trigger
this logic instead of relying on the recursive flag of the pathspec.  Or
we can add a recurse_submodules flag to the pathspec struct and respect
that flag instead of the 'recursive' flag.

I have a quick patch to do just that which I'll send shortly.

> 
> And now it gets really interesting. Or confusing, depending on your mental
> condition. This recursive flag of the pathspec is set in
> ll_diff_tree_paths() (yep, changing the flag in the passed-in opt
> structure... which I found a bit... unexpected, given the function name, I
> would have been less surprised if that function only diff'ed the trees and
> used the options without changing the options). That flag-change was
> introduced in
> https://github.com/git/git/commit/bc96cc87dbb2#diff-15203e8cd8ee9191113894de9d97a8a6R149
> which is another patch that changed the tree diff machinery to accommodate
> `git grep` (but maybe not really paying a lot of attention to the fact
> that the same machinery is called repeatedly by the revision machinery,
> too).
> 
> I am really confused by this code mainly due to the fact that the term
> "recursive" is pretty ambiguous in that context: does it refer to
> directories/tree objects, or to submodules? I guess it is used for both
> when there should be two flags so that rev-list can recurse over tree
> objects but not submodules (unless told to do so).
> 
> The problem, of course, is that `git rev-list HEAD -- '[bracket]'` never
> recurses into the submodule. And therefore, the promised "more accurate
> matching [...] in the submodule" is never performed. And the commit adding
> the submodule is never pruned.
> 
> Since I am not really familiar with all that tree diff code (and as a
> general rule to protect my mental health, I try my best to stay away from
> submodules, too), but you two are, may I ask you gentle people to have a
> closer look to fix those bugs?
> 
> Thanks,
> 

[add-default-config] add --default option to git config.

[Soukaina NAIT HMID]

From: Soukaina NAIT HMID 

Signed-off-by: Soukaina NAIT HMID 
---
 Documentation/git-config.txt |   4 ++
 builtin/config.c |  34 -
 config.c |  10 +++
 config.h |   1 +
 t/t1300-repo-config.sh   | 161 +++
 5 files changed, 209 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 4edd09fc6b074..5d5cd58fdae37 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -179,6 +179,10 @@ See also <>.
specified user.  This option has no effect when setting the
value (but you can use `git config section.variable ~/`
from the command line to let your shell do the expansion).
+--color::
+   Find the color configured for `name` (e.g. `color.diff.new`) and
+   output it as the ANSI color escape sequence to the standard
+   output. 
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index d13daeeb55927..5e5b998b7c892 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -30,6 +30,7 @@ static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
+static const char *default_value;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -52,6 +53,8 @@ static int show_origin;
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
+#define TYPE_COLOR (1<<4)
+
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -80,11 +83,13 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT),
OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
OPT_BIT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
+   OPT_BIT(0, "color", , N_("find the color configured"), 
TYPE_COLOR),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes_opt, N_("respect include 
directives on lookup")),
OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, standard input, blob, command line)")),
+   OPT_STRING(0, "default", _value, N_("default-value"), N_("sets 
default value when no value is returned from config")),
OPT_END(),
 };
 
@@ -159,6 +164,13 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
return -1;
strbuf_addstr(buf, v);
free((char *)v);
+   }
+   else if (types == TYPE_COLOR) {
+   char *v = xmalloc(COLOR_MAXLEN);
+   if (git_config_color(, key_, value_) < 0)
+   return -1;
+   strbuf_addstr(buf, v);
+   free((char *)v);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -244,8 +256,16 @@ static int get_value(const char *key_, const char *regex_)
config_with_options(collect_config, ,
_config_source, _options);
 
-   ret = !values.nr;
+   if (!values.nr && default_value && types) {
+   struct strbuf *item;
+   ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+   item = [values.nr++];
+   if(format_config(item, key_, default_value) < 0){
+   values.nr = 0;
+   }
+   }
 
+   ret = !values.nr;
for (i = 0; i < values.nr; i++) {
struct strbuf *buf = values.items + i;
if (do_all || i == values.nr - 1)
@@ -268,6 +288,7 @@ static int get_value(const char *key_, const char *regex_)
return ret;
 }
 
+
 static char *normalize_value(const char *key, const char *value)
 {
if (!value)
@@ -281,6 +302,17 @@ static char *normalize_value(const char *key, const char 
*value)
 * when retrieving the value.
 */
return xstrdup(value);
+   if (types == TYPE_COLOR)
+   {
+   char *v = xmalloc(COLOR_MAXLEN);
+   if (git_config_color(, key, value) == 0)
+   {
+   free((char *)v);
+   return xstrdup(value);
+   }
+   free((char *)v);
+   die("cannot parse color '%s'", value);
+   }
if (types == TYPE_INT)
return xstrfmt("%"PRId64, git_config_int64(key, value));
if (types == TYPE_BOOL)
diff --git a/config.c b/config.c
index 903abf9533b18..5c5daffeb6723 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 

[PATCH 2/4] sha1dc_git.h: re-arrange an ifdef chain for a subsequent change

[Ævar Arnfjörð Bjarmason]

A subsequent change will change the semantics of DC_SHA1_SUBMODULE in
a way that would require moving these checks around, so start by
moving them around without any functional changes.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 sha1dc_git.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1dc_git.h b/sha1dc_git.h
index a8c2729278..41e1c3fd3f 100644
--- a/sha1dc_git.h
+++ b/sha1dc_git.h
@@ -1,9 +1,9 @@
 /* Plumbing with collition-detecting SHA1 code */
 
-#ifdef DC_SHA1_SUBMODULE
-#include "sha1collisiondetection/lib/sha1.h"
-#elif defined(DC_SHA1_EXTERNAL)
+#ifdef DC_SHA1_EXTERNAL
 #include 
+#elif defined(DC_SHA1_SUBMODULE)
+#include "sha1collisiondetection/lib/sha1.h"
 #else
 #include "sha1dc/sha1.h"
 #endif
-- 
2.15.0.403.gc27cc4dac6

[PATCH 0/4] SHA1DC fixes & fully moving to a git.git submodule

[Ævar Arnfjörð Bjarmason]

We've now had a couple of Git releases where we've used
sha1collisiondetection/ if it's checked out, but have gracefully
fallen back on sha1dc/ if it's not there.

This series makes it a hard requirement, without 4/4 you'll still be
able to do NO_DC_SHA1_SUBMODULE=UnfortunatelyYes, but with it even the
ability to do that is removed, i.e. we're fully on the submodule
(unless you have it as an external library).

1/4 should be destined straight for inclusion since it's a bugfix to
the existing logic, and 2/4 could tag along with it, but none of this
is urgent, so I'd figured I'd sent it all as one series and see what
people think.

Ævar Arnfjörð Bjarmason (4):
  Makefile: don't error out under DC_SHA1_EXTERNAL if
DC_SHA1_SUBMODULE=auto
  sha1dc_git.h: re-arrange an ifdef chain for a subsequent change
  Makefile: use the sha1collisiondetection submodule by default
  sha1dc: remove in favor of using sha1collisiondetection as a submodule

 Makefile  |   29 +-
 sha1dc/.gitattributes |1 -
 sha1dc/LICENSE.txt|   30 -
 sha1dc/sha1.c | 1900 -
 sha1dc/sha1.h |  110 ---
 sha1dc/ubc_check.c|  372 --
 sha1dc/ubc_check.h|   52 --
 sha1dc_git.h  |6 +-
 8 files changed, 14 insertions(+), 2486 deletions(-)
 delete mode 100644 sha1dc/.gitattributes
 delete mode 100644 sha1dc/LICENSE.txt
 delete mode 100644 sha1dc/sha1.c
 delete mode 100644 sha1dc/sha1.h
 delete mode 100644 sha1dc/ubc_check.c
 delete mode 100644 sha1dc/ubc_check.h

-- 
2.15.0.403.gc27cc4dac6

[PATCH 1/4] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto

[Ævar Arnfjörð Bjarmason]

Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If
git.git has a sha1collisiondetection submodule checked out the logic
to set DC_SHA1_SUBMODULE=auto would interact badly with the check for
whether DC_SHA1_SUBMODULE was set.

It would error out, meaning that there's no way to build git with
DC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule.

Instead, adjust the logic to only fire if the variable is to something
else than "auto" which would mean it's a mistake on the part of
whoever's building git, not just the Makefile tripping over its own
logic.

1. 3964cbbb5c ("sha1dc: allow building with the external sha1dc
   library", 2017-08-15)
2. cac87dc01d ("sha1collisiondetection: automatically enable when
   submodule is populated", 2017-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index e53750ca01..8fe8278126 100644
--- a/Makefile
+++ b/Makefile
@@ -1497,7 +1497,9 @@ else
LIB_OBJS += sha1dc_git.o
 ifdef DC_SHA1_EXTERNAL
ifdef DC_SHA1_SUBMODULE
+ifneq ($(DC_SHA1_SUBMODULE),auto)
 $(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)
+endif
endif
BASIC_CFLAGS += -DDC_SHA1_EXTERNAL
EXTLIBS += -lsha1detectcoll
-- 
2.15.0.403.gc27cc4dac6

[PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Ævar Arnfjörð Bjarmason]

Change the build process so that instead of needing to supply
DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection
submodule instead of the copy of the same code shipped in the sha1dc
directory, it uses the submodule by default unless
NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.

This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally
use sha1collisiondetection as a submodule", 2017-07-01). Git has now
shipped with the submodule in git.git for two major releases, if we're
ever going to migrate to fully using it instead of perpetually
maintaining both sha1collisiondetection and the sha1dc directory this
is a logical first step.

This change removes the "auto" logic Junio added in
cac87dc01d ("sha1collisiondetection: automatically enable when
submodule is populated", 2017-07-01), I feel that automatically
falling back to using sha1dc would defeat the point, which is to smoke
out any remaining users of git.git who have issues cloning the
submodule for whatever reason.

Instead the Makefile will emit an error if the contents of the
submodule aren't checked out (line-wrapped. GNU make emits this all on
one line):

Makefile:1031: *** The sha1collisiondetection submodule is not
checked out. Please make it available, either by cloning with
--recurse-submodules, or by running "git submodule update
--init". If you can't use it for whatever reason you can define
NO_DC_SHA1_SUBMODULE=UnfortunatelyYes.  Stop.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Makefile | 32 +++-
 sha1dc_git.h |  2 +-
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index 8fe8278126..b5308bc3ca 100644
--- a/Makefile
+++ b/Makefile
@@ -167,11 +167,12 @@ all::
 # Without this option, i.e. the default behavior is to build git with its
 # own built-in code (or submodule).
 #
-# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
-# sha1collisiondetection shipped as a submodule instead of the
-# non-submodule copy in sha1dc/. This is an experimental option used
-# by the git project to migrate to using sha1collisiondetection as a
-# submodule.
+# Define NO_DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
+# sha1collisiondetection library shipped as a non-submodule copy in
+# sha1dc/, instead of using the sha1collisiondetection submodule. This
+# option will eventually go away. Clone git with
+# "--recurse-submodules" or run "git submodule update --init" after
+# cloning.
 #
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.
@@ -1025,8 +1026,15 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
-ifeq ($(wildcard 
sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
-DC_SHA1_SUBMODULE = auto
+ifndef NO_DC_SHA1_SUBMODULE
+ifndef DC_SHA1_EXTERNAL
+ifneq ($(wildcard 
sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
+$(error The sha1collisiondetection submodule is not checked out. \
+Please make it available, either by cloning with --recurse-submodules, \
+or by running "git submodule update --init". If you can't use it for \
+whatever reason define NO_DC_SHA1_SUBMODULE=UnfortunatelyYes)
+endif
+endif
 endif
 
 include config.mak.uname
@@ -1496,19 +1504,17 @@ else
BASIC_CFLAGS += -DSHA1_DC
LIB_OBJS += sha1dc_git.o
 ifdef DC_SHA1_EXTERNAL
-   ifdef DC_SHA1_SUBMODULE
-ifneq ($(DC_SHA1_SUBMODULE),auto)
-$(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both)
-endif
+   ifdef NO_DC_SHA1_SUBMODULE
+$(error Only set DC_SHA1_EXTERNAL or NO_DC_SHA1_SUBMODULE, not both)
endif
BASIC_CFLAGS += -DDC_SHA1_EXTERNAL
EXTLIBS += -lsha1detectcoll
 else
-ifdef DC_SHA1_SUBMODULE
+ifndef NO_DC_SHA1_SUBMODULE
LIB_OBJS += sha1collisiondetection/lib/sha1.o
LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
-   BASIC_CFLAGS += -DDC_SHA1_SUBMODULE
 else
+   BASIC_CFLAGS += -DNO_DC_SHA1_SUBMODULE
LIB_OBJS += sha1dc/sha1.o
LIB_OBJS += sha1dc/ubc_check.o
 endif
diff --git a/sha1dc_git.h b/sha1dc_git.h
index 41e1c3fd3f..1bcc4c473c 100644
--- a/sha1dc_git.h
+++ b/sha1dc_git.h
@@ -2,7 +2,7 @@
 
 #ifdef DC_SHA1_EXTERNAL
 #include 
-#elif defined(DC_SHA1_SUBMODULE)
+#elif !defined(NO_DC_SHA1_SUBMODULE)
 #include "sha1collisiondetection/lib/sha1.h"
 #else
 #include "sha1dc/sha1.h"
-- 
2.15.0.403.gc27cc4dac6

[PATCH] git-prompt: fix reading files with windows line endings

[Robert Abel]

If any of the files read by __git_eread have \r\n line endings, read
will only strip \n, leaving \r. This results in an ugly prompt, where
instead of

user@pc MINGW64 /path/to/repo (BARE:master)

the last parenthesis is printed over the beginning of the prompt like

)ser@pc MINGW64 /path/to/repo (BARE:master

Signed-off-by: Robert Abel 
---
 contrib/completion/git-prompt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c6cbef38c2..71a64e7959 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -282,7 +282,7 @@ __git_eread ()
 {
local f="$1"
shift
-   test -r "$f" && read "$@" <"$f"
+   test -r "$f" && read "$@" <"$f" && export $@="${!@%$'\r'}"
 }
 
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
-- 
2.13.0.windows.1

git-prompt: fix reading files with windows line endings

[Robert Abel]

I noticed today that my git prompt using msys-git on Windows got a bit broken.
After investigating I found that the git-prompt doesn't handle the case when
__git_eread reads Windows line endings \r\n. It will only strip \n, leaving
the \r.

I noticed this when I created a repository with msys-git, did some tasks and
later wanted to check the bare. Apparently, another tool on my PC went wild
and replaced all line endings in all text files it could find, breaking my git
prompt.

[PATCH v3] diff: support anchoring line(s)

[Jonathan Tan]

Teach diff a new algorithm, one that attempts to prevent user-specified
lines from appearing as a deletion or addition in the end result. The
end user can use this by specifying "--anchored=" one or more
times when using Git commands like "diff" and "show".

Signed-off-by: Jonathan Tan 
---
v3 addresses Junio's comments.

> This makes sense, but "--diff-algorithm=patience" would want to do
> the same, I suspect, so the loop would want to become a little
> helper function "clear_patience_anchors(options)" or something like
> that.

Done. Also updated a test to test the interaction of
"--diff-algorithm=patience" with "--anchored=<>".

> This may be too little to matter, but I'd find
>
>   printf "%s\n" a b c >pre
>
> vastly easier to read.  Or perhaps just use
>
>   test_write_lines a b c >pre

Thanks for the pointer to test_write_lines - I used that and it is
indeed clearer.
---
 Documentation/diff-options.txt |  10 +
 diff.c |  31 -
 diff.h |   4 ++
 t/t4064-diff-anchored.sh   | 100 +
 xdiff/xdiff.h  |   4 ++
 xdiff/xpatience.c  |  42 ++---
 6 files changed, 184 insertions(+), 7 deletions(-)
 create mode 100755 t/t4064-diff-anchored.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index dd0dba5b1..6ce39fb69 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -80,6 +80,16 @@ endif::git-format-patch[]
 --histogram::
Generate a diff using the "histogram diff" algorithm.
 
+--anchored=::
+   Generate a diff using the "anchored diff" algorithm.
++
+This option may be specified more than once.
++
+If a line exists in both the source and destination, exists only once,
+and starts with this text, this algorithm attempts to prevent it from
+appearing as a deletion or addition in the output. It uses the "patience
+diff" algorithm internally.
+
 --diff-algorithm={patience|minimal|histogram|myers}::
Choose a diff algorithm. The variants are as follows:
 +
diff --git a/diff.c b/diff.c
index 0763e8926..7149ff627 100644
--- a/diff.c
+++ b/diff.c
@@ -3210,6 +3210,8 @@ static void builtin_diff(const char *name_a,
ecbdata.opt = o;
ecbdata.header = header.len ?  : NULL;
xpp.flags = o->xdl_opts;
+   xpp.anchors = o->anchors;
+   xpp.anchors_nr = o->anchors_nr;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
xecfg.flags = XDL_EMIT_FUNCNAMES;
@@ -3302,6 +3304,8 @@ static void builtin_diffstat(const char *name_a, const 
char *name_b,
memset(, 0, sizeof(xpp));
memset(, 0, sizeof(xecfg));
xpp.flags = o->xdl_opts;
+   xpp.anchors = o->anchors;
+   xpp.anchors_nr = o->anchors_nr;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
if (xdi_diff_outf(, , diffstat_consume, diffstat,
@@ -4487,6 +4491,21 @@ static int parse_ws_error_highlight_opt(struct 
diff_options *opt, const char *ar
return 1;
 }
 
+/*
+ * Clear the anchors configured for the PATIENCE_DIFF algorithm.
+ *
+ * This must be called whenever a command-line argument indicating that
+ * the patience algorithm is to be used is seen, because the patience
+ * and anchored algorithms both use PATIENCE_DIFF internally.
+ */
+static void clear_patience_anchors(struct diff_options *options)
+{
+   int i;
+   for (i = 0; i < options->anchors_nr; i++)
+   free(options->anchors[i]);
+   options->anchors_nr = 0;
+}
+
 int diff_opt_parse(struct diff_options *options,
   const char **av, int ac, const char *prefix)
 {
@@ -4594,9 +4613,10 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_SET(options, INDENT_HEURISTIC);
else if (!strcmp(arg, "--no-indent-heuristic"))
DIFF_XDL_CLR(options, INDENT_HEURISTIC);
-   else if (!strcmp(arg, "--patience"))
+   else if (!strcmp(arg, "--patience")) {
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
-   else if (!strcmp(arg, "--histogram"))
+   clear_patience_anchors(options);
+   } else if (!strcmp(arg, "--histogram"))
options->xdl_opts = DIFF_WITH_ALG(options, HISTOGRAM_DIFF);
else if ((argcount = parse_long_opt("diff-algorithm", av, ))) {
long value = parse_algorithm_value(optarg);
@@ -4607,7 +4627,14 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_CLR(options, NEED_MINIMAL);
options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
options->xdl_opts |= value;
+   if (value == XDF_PATIENCE_DIFF)
+   clear_patience_anchors(options);
  

Re: Feature request: Reduce amount of diff in patch

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 18:09 +0200, KES wrote:
> Hi.
> 
> I get often patches which can be minimized:
> 

I guess the one below can't be (see below).


> @@ -60,11 +64,8 @@ sub _get_filter {
>  address=>  { -like => \[ '?',  "%$search%" ] },
>  company=>  { -like => \[ '?',  "%$search%" ] },
>  country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
> -]);
>  
> -$users =  $users->search( $filter, {
> -prefetch => { Packages => { Ips => { Subnet => { Server => 
> 'Locality' ,
> -});
> +]);
>  
>  
>  return $users;
> 
> This patch can be minimized to:
> 
> @@ -60,11 +64,8 @@ sub _get_filter {
>  address=>  { -like => \[ '?',  "%$search%" ] },
>  company=>  { -like => \[ '?',  "%$search%" ] },
>  country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
>  ]);
>  
> -$users =  $users->search( $filter, {
> -prefetch => { Packages => { Ips => { Subnet => { Server => 
> 'Locality' ,
> -});
> 
>  
>  return $users;
> 
> May you please fix the git to generate minimized patches?
> 

You seemed to have overlooked the empty line above the ']);' in the
original patch. So, your minimized version isn't actually equivalent to
the original one. Further, when trying to recreate your patch I get the
following,

diff --git a/diff-test b/diff-test
index 1d5dc1b..f3ec38f 100644
--- a/diff-test
+++ b/diff-test
@@ -1,10 +1,8 @@
 address=>  { -like => \[ '?',  "%$search%" ] },
 company=>  { -like => \[ '?',  "%$search%" ] },
 country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
+
 ]);
 
-$users =  $users->search( $filter, {
-prefetch => { Packages => { Ips => { Subnet => { Server => 'Locality' 
,
-});
 
 return $users;

I use git built from the source (2.15.0.531.g2ccb3012c)

-- 
Kaartic

Re: [PATCH v2 3/3] rebase: rebasing can also be done when HEAD is detached

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 11:31 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > +   if test "$branch_or_commit" = "HEAD" &&
> > +!(git symbolic-ref -q HEAD)
> 
> Did you need a subshell here? 

No. That's a consequence of me not remembering that I would span a sub-
shell with a simple '()' when I was doing that part. (partial
transition from C to shell)


>  Now with a proper test with
> "symbolic-ref -q HEAD", I wonder if you'd need to check if the
> original was named HEAD in the first place (I do not feel strongly
> enough to say that checking is wrong, at least not yet, but the
> above does make me wonder), and instead something like
> 
>   if ! git symbolic-ref -q HEAD
>   then
>   ...
> 
> might be sufficient.  I dunno.
> 

It does  seem you're right. The only thing we would be losing is the
short-circuiting when $branch_or_commit is not HEAD (which I suspect to
be the case most of the time). So, I'm not sure if I should remove the
check (of course, I'll change the check to avoid spawning a sub-shell).


Thanks, 
Kaartic

Re: [RFC PATCH v2] builtin/worktree: enhance worktree removal

[Kaartic Sivaraam]

On Tuesday 28 November 2017 09:34 AM, Junio C Hamano wrote:

Eric Sunshine  writes:


With this approach, validate_worktree() will print an error message
saying that the worktree directory is missing before the control info
is actually removed. Kaartic's original patch silenced the message
(and _all_ error messages from validate_worktree()) by passing 1 as
the second argument. That seemed heavy-handed, so I suggested keeping
the validate_worktree() call as-is but doing the directory-missing
check first as a special case.

But perhaps that special case isn't necessary.


I do not think the user minds to see "there is no such directory
there"; actually that would be beneficial, even.

But you are right; validate_worktree() would need to become aware of
the possibility that it can be passed such a "corrupt" worktree to
handle if that approach is followed.

The case we are discussing, i.e. the user removed the directory
without telling Git to give it a chance to remove control
information, may be common enough that it becomes a worthwhile
addition to make the "quiet" boolean that occupies a whole int to an
unsigned that is a collection of bits, and pass "missing_ok" bit in
that flag word to the validate_worktree() to tell that the caller
can tolerate the "user removed it while we were looking the other
way" case and can handle it gracefully.  But that would be a lot
larger change, and "the caller checks before calling validate" as
done with this [RFC v2] may be a good way to add the feature with
the least impact to the codebase.


I had envisioned a simple 'goto remove_control_info' rather than extra
nesting or refactoring, but that's a minor point.


Yes, use of goto is also a good way to avoid having to worry about
the future evolution of the codeflow in this function.

So perhaps

if (the directory is no longer there)
goto cleanup;
if (the worktree does not validate)
return -1;
... the original code to (carefully) remove the
... worktree itself

cleanup:

... remove control info
... free resources held in variables
... return from the function

is what we want?



Probably but I'm not interested in going for a v3 that does that as I 
just wanted to show that worktree remove could be enhanced in this 
aspect and show how it could be done. So, I'll leave this in the 
#leftoverbits for the person who would be re-rolling nd/worktree-move.


---
Kaartic

[PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

When the N-th previous thing checked out syntax (@{-N}) is used
with '--branch' option of check-ref-format the result might not
always be a valid branch name (see NOTE below). This is because
@{-N} is used to refer to the N-th last checked out "thing" which
might be any commit (sometimes a branch). The documentation thus
does a wrong thing by promoting it as the "previous branch syntax".

So, correctly state @{-N} is the syntax for specifying "N-th last
thing checked out" and also state that the result of using @{-N}
might also result in a "commit hash".

NOTE: Though a commit-hash is a "syntactically" valid branch name,
it is generally not considered as one for the use cases of
"git check-ref-format --branch". That's because a user who does
"git check-ref-format --branch @{-$N}" would except the output
to be a "existing" branch name apart from it being syntactically
valid.

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/git-check-ref-format.txt | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt 
b/Documentation/git-check-ref-format.txt
index cf0a0b7df..5ddb562d0 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,20 @@ reference name expressions (see linkgit:gitrevisions[7]):
 . at-open-brace `@{` is used as a notation to access a reflog entry.
 
 With the `--branch` option, the command takes a name and checks if
-it can be used as a valid branch name (e.g. when creating a new
-branch).  The rule `git check-ref-format --branch $name` implements
+it can be used as a valid branch name e.g. when creating a new branch
+(except for one exception related to the previous checkout syntax
+noted below). The rule `git check-ref-format --branch $name` implements
 may be stricter than what `git check-ref-format refs/heads/$name`
 says (e.g. a dash may appear at the beginning of a ref component,
 but it is explicitly forbidden at the beginning of a branch name).
 When run with `--branch` option in a repository, the input is first
-expanded for the ``previous branch syntax''
-`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
-were on.  This option should be used by porcelains to accept this
-syntax anywhere a branch name is expected, so they can act as if you
-typed the branch name.
+expanded for the ``previous checkout syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checkout using "git checkout" operation. This option should be
+used by porcelains to accept this syntax anywhere a branch name is
+expected, so they can act as if you typed the branch name. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit hash when the N-th last thing checked out was not a branch.
 
 OPTIONS
 ---
@@ -116,7 +119,7 @@ OPTIONS
 EXAMPLES
 
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.531.g2ccb3012c

Feature request: Reduce amount of diff in patch

[KES]

Hi.

I get often patches which can be minimized:

@@ -60,11 +64,8 @@ sub _get_filter {
 address=>  { -like => \[ '?',  "%$search%" ] },
 company=>  { -like => \[ '?',  "%$search%" ] },
 country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
-]);
 
-$users =  $users->search( $filter, {
-prefetch => { Packages => { Ips => { Subnet => { Server => 'Locality' 
,
-});
+]);
 
 
 return $users;

This patch can be minimized to:

@@ -60,11 +64,8 @@ sub _get_filter {
 address=>  { -like => \[ '?',  "%$search%" ] },
 company=>  { -like => \[ '?',  "%$search%" ] },
 country_code =>  { '=' => \[ 'UPPER(?)' => $search ] },
 ]);
 
-$users =  $users->search( $filter, {
-prefetch => { Packages => { Ips => { Subnet => { Server => 'Locality' 
,
-});

 
 return $users;

May you please fix the git to generate minimized patches?

Thank you

Re: [PATCH] travis-ci: avoid new tcl/tk build requirement

[Todd Zullinger]

Christian Couder wrote:

Junio C Hamano wrote:

It seems that TravisCI objects ;-)

   https://travis-ci.org/git/git/jobs/307745929


Interesting that the main builds passed.  I don't know what the default
64-bit linuxinstall looks like in travis, so I presume it includes
tcl/tk or something.


Yeah, interesting. I am cc'ing Lars who perhaps knows.


I pulled the travis docker image used in clang/gcc builds[1] and can
see it has both tcl and tk packages installed.  The linux32 builds use
a docker image[2] which does not contain tcl or tk.

[1] travisci/ci-garnet:packer-1503972846
[2] daald/ubuntu32:xenial

If we wanted, we could set BYPASS_TCLTK_CHECK only for the linux32
builds.  I think it's probably better to do it globally rather than
rely on the travis containers implicitly having tcl/tk installed.


The patch looks good to me. Thanks!

I wonder if it would be better to squash it into my patch or to keep
it separate. I am ok with both ways.


I fine either way too.  This is still just on pu, so squashing it in
seems like the way to go.  I only made it separate to cause travis to
run the tests. :)

--
Todd
~~
In the beginning the Universe was created. This has made a lot of
people very angry and has been widely regarded as a bad move.
   -- Douglas Adams

Re: bug deleting "unmerged" branch (2.12.3)

[Johannes Schindelin]

Hi Ulrich,

On Tue, 28 Nov 2017, Ulrich Windl wrote:

> During a rebase that turned out to be heavier than expected 8-( I
> decided to keep the old branch by creating a temporary branch name to
> the commit of the branch to rebase (which was still the old commit ID at
> that time).
>
> When done rebasing, I attached a new name to the new (rebased) branch,
> deleted the old name (pointing at the same rebase commit), then
> recreated the old branch from the temporary branch name (created to
> remember the commit id).
>
> When I wanted to delete the temporary branch (which is of no use now), I
> got a message that the branch is unmerged.

This is actually as designed, at least for performance reasons (it is not
exactly cheap to figure out whether a given commit is contained in any
other branch).

> I think if more than one branches are pointing to the same commit, one
> should be allowed to delete all but the last one without warning. Do you
> agree?

No, respectfully disagree, because I have found myself with branches
pointing to the same commit, even if the branches served different
purposes. I really like the current behavior where you can delete a
branch with `git branch -d` as long as it is contained in its upstream
branch.

Ciao,
Johannes

GET BACK TO ME FOR YOUR ATM CARD

[Secretary General]

3 Whitehall Court London
SW1A 2EL United Kingdom
Attention:Beneficairy.

NOTE: If you received this message in your SPAM/BULK folder, it is because of 
the restrictions imposed by your Mail/Internet Service Provider, we urge you to 
treat it genuinely.

How are you today? Hope all is well with you and family? You may not understand 
why this mail came to you. I wish to inform you that we have been having a 
meeting with the IMF for the past 1 month now which ends yesterday in regards 
to innocent individual who has been a victim of scam that was going on in 
African and Europe, this Organization(UNITED NATIONS) have agreed to compensate 
them with the sum of 750,000.00.

This includes every foreign contractors that may have not received their 
Contract sum, and some people that have an unfinished Transaction/International 
Businesses that failed due to one problem or the other.
 

Going through our data base your email contacts and name was found on the list 
and that is why we are contacting you for you to receive your own compensation 
and this said money will be paid via ATM MASTER CARD Payment Method which is 
the easiest and safer way of receiving without any hindrance and it will be 
issued to you from our Authorized Bank in Nigeria(Africa) You are advised to 
contact
 

Mr.Herbert Wigwe
Email: unlegrepafr...@gmail.com

As he is our Legal Representative in Nigeria, contact him immediately for your 
own ATM MASTER CARD, you should send him your full Name and telephone number 
your confidential address where you want him to send the Card.

Hoping to hear from you as soon as you receive your MASTER CARD.

Best regards,
Secretary-General

Re: [PATCH] travis-ci: avoid new tcl/tk build requirement

[Christian Couder]

On Tue, Nov 28, 2017 at 3:37 PM, Todd Zullinger  wrote:
> A build requirement on tcl/tk was added in 01c54284f1 (Makefile: check
> that tcl/tk is installed, 2017-11-20).  For building and running the
> tests, we don't need tcl/tk installed.  Bypass the requirement.
>
> Signed-off-by: Todd Zullinger 
> ---
>
> Junio C Hamano wrote:
>> It seems that TravisCI objects ;-)
>>
>>https://travis-ci.org/git/git/jobs/307745929
>
> Interesting that the main builds passed.  I don't know what the default
> 64-bit linuxinstall looks like in travis, so I presume it includes
> tcl/tk or something.

Yeah, interesting. I am cc'ing Lars who perhaps knows.

> In any case, perhaps something like this is what we want?  We could use
> NO_TCLTK or ensure that tcl/tk is installed in all environments.  I used
> the BYPASS_TCLTK_CHECK option since the tests have been running without
> tcl/tk previously.  If they become required for the tests, this can be
> adjusted.
>
> I have a travis job running with this change here:
>
> https://travis-ci.org/tmzullinger/git/builds/308452464
>
> So far the only failure is (what looks like) an unrelated one in the
> GETTEXT_POISON build.

Yeah, I can't see how test failures in the t/ directory could be related.

>  .travis.yml  | 1 +
>  ci/run-linux32-docker.sh | 1 +
>  2 files changed, 2 insertions(+)

The patch looks good to me. Thanks!

I wonder if it would be better to squash it into my patch or to keep
it separate. I am ok with both ways.

DIRECTOR IN CHARGE: DR.PATRICE TEME

[U N-Headquarters]

UN Visitor Centre
Department of Public Information
United Nations Headquarters
Room DHL-1B-154
New York, NY 10017
E-mail:un...@teewars.org

United Nations Compensation Unit, In Affiliation with World Bank Our Ref: 
UN/WBO/042UK/2015.

Congratulations Beneficiary,

How are you today  Hope all is well with you and family  You may not understand 
why this mail came to you. We have been having a meeting for the past 7 months 
which just ended few days ago with the secretary to the UNITED NATIONS. This 
email is to all the people that have been scammed in any part of the world, the 
UNITED NATIONS in Affiliation with WORLD BANK have agreed to compensate them 
with the sum of USD US$980,000.00 Dollars.

This includes every foreign contractors that may have not received their 
contract sum, and people that have had an unfinished transaction or 
international businesses that failed due to Government problems etc. We found 
your name in the list of those who are to benefit from these compensation 
exercise and that is why we are contacting you, this have been agreed upon and 
have been signed. You are advised to contact Dr.PATRICE TEME of our paying 
center in Africa, as he is our representative in Nigeria, contact him 
immediately for your Cheque/ International Bank Draft of US$980,000.00 Dollars.

This fund is in form of a Bank Draft for security purpose ok  So he will send 
it to you and you can clear it in any bank of your choice. Therefore, you 
should send him your full Name and telephone number your correct mailing 
address where you want him to send the Draft to you. Contact Dr.PATRICE TEME of 
MAGNUM PLC PAYMENT CENTER with your payment code:ST/DPI/829 immediately for 
your Cheque at the given address below:

DIRECTOR IN CHARGE: DR.PATRICE TEME
E-MAIL:info-magnumb...@ukcompanies.org
TELEPHONE:+ 234-817-008-4240
FAX: +234-817-008-4240

I apologize on behalf of my organization for any delay you might have 
encountered in receiving your fund in the past. Thanks and God bless you and 
your family. Hoping to hear from you as soon as you cash your Bank Draft. 
Making the world a better place.

You are required to contact the above person and furnish him with the following 
of your information that will be required to avoid any mistakes:-

1. Your Full Name :
2. Your Home/Mobile Telephone No:
3. Your Home or Office Address :
4. Age/Occupation/Marital Status:
5. Scanned copy of your identification:

Congratulations, and I look forward to hear from you as soon as you confirm 
your payment making the world a better place

DIRECTOR IN CHARGE: DR.PATRICE TEME

[U N-Headquarters]

UN Visitor Centre
Department of Public Information
United Nations Headquarters
Room DHL-1B-154
New York, NY 10017
E-mail:un...@teewars.org

United Nations Compensation Unit, In Affiliation with World Bank Our Ref: 
UN/WBO/042UK/2015.

Congratulations Beneficiary,

How are you today  Hope all is well with you and family  You may not understand 
why this mail came to you. We have been having a meeting for the past 7 months 
which just ended few days ago with the secretary to the UNITED NATIONS. This 
email is to all the people that have been scammed in any part of the world, the 
UNITED NATIONS in Affiliation with WORLD BANK have agreed to compensate them 
with the sum of USD US$980,000.00 Dollars.

This includes every foreign contractors that may have not received their 
contract sum, and people that have had an unfinished transaction or 
international businesses that failed due to Government problems etc. We found 
your name in the list of those who are to benefit from these compensation 
exercise and that is why we are contacting you, this have been agreed upon and 
have been signed. You are advised to contact Dr.PATRICE TEME of our paying 
center in Africa, as he is our representative in Nigeria, contact him 
immediately for your Cheque/ International Bank Draft of US$980,000.00 Dollars.

This fund is in form of a Bank Draft for security purpose ok  So he will send 
it to you and you can clear it in any bank of your choice. Therefore, you 
should send him your full Name and telephone number your correct mailing 
address where you want him to send the Draft to you. Contact Dr.PATRICE TEME of 
MAGNUM PLC PAYMENT CENTER with your payment code:ST/DPI/829 immediately for 
your Cheque at the given address below:

DIRECTOR IN CHARGE: DR.PATRICE TEME
E-MAIL:info-magnumb...@ukcompanies.org
TELEPHONE:+ 234-817-008-4240
FAX: +234-817-008-4240

I apologize on behalf of my organization for any delay you might have 
encountered in receiving your fund in the past. Thanks and God bless you and 
your family. Hoping to hear from you as soon as you cash your Bank Draft. 
Making the world a better place.

You are required to contact the above person and furnish him with the following 
of your information that will be required to avoid any mistakes:-

1. Your Full Name :
2. Your Home/Mobile Telephone No:
3. Your Home or Office Address :
4. Age/Occupation/Marital Status:
5. Scanned copy of your identification:

Congratulations, and I look forward to hear from you as soon as you confirm 
your payment making the world a better place

Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 11:40 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > When the N-th previous thing checked out sytax is used with
> > '--branch' option of check-ref-format the results might not
> > always be a valid branch name
> 
> I wonder if you want to rephrase this, because 40-hex object name is
> syntactically a valid branch name.  It's (1) cumbersome to type and
> (2) may not be what the user expects.
> 

You're right. Actually a previous draft of that log message did say
something like,

Though a commit-hash might be a valid branch name, it isn't
something that's expected by the users of "check-ref-format".

I removed as I thought it would be unnecessary. It seems I took the
wrong decision. Will fix it. :-)

> I have a mild suspicion that "git checkout -B @{-1}" would want to
> error out instead of creating a valid new branch whose name is
> 40-hex that happen to be the name of the commit object you were
> detached at previously.
> 

I thought this the other way round. Rather than letting the callers
error out when @{-N} didn't expand to a branch name, I thought we
should not be expanding @{-N} syntax for "check-ref-format --branch" at
all to make a "stronger guarantee" that the result is "always" a valid
branch name. Then I thought it might be too restrictive and didn't
mention it. So, I dunno.


> I am not sure if "check-ref-format --branch" should the same; it is
> more about the syntax and the 40-hex _is_ valid there, so...

I'm not sure what you were trying to say here, sorry.


-- 
Kaartic

[PATCH] travis-ci: avoid new tcl/tk build requirement

[Todd Zullinger]

A build requirement on tcl/tk was added in 01c54284f1 (Makefile: check
that tcl/tk is installed, 2017-11-20).  For building and running the
tests, we don't need tcl/tk installed.  Bypass the requirement.

Signed-off-by: Todd Zullinger 
---

Junio C Hamano wrote:
> It seems that TravisCI objects ;-)
>
>https://travis-ci.org/git/git/jobs/307745929

Interesting that the main builds passed.  I don't know what the default
64-bit linuxinstall looks like in travis, so I presume it includes
tcl/tk or something.

In any case, perhaps something like this is what we want?  We could use
NO_TCLTK or ensure that tcl/tk is installed in all environments.  I used
the BYPASS_TCLTK_CHECK option since the tests have been running without
tcl/tk previously.  If they become required for the tests, this can be
adjusted.

I have a travis job running with this change here:

https://travis-ci.org/tmzullinger/git/builds/308452464

So far the only failure is (what looks like) an unrelated one in the
GETTEXT_POISON build.

 .travis.yml  | 1 +
 ci/run-linux32-docker.sh | 1 +
 2 files changed, 2 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 281f101f31..9e57caa83d 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -23,6 +23,7 @@ addons:
 
 env:
   global:
+- BYPASS_TCLTK_CHECK=1
 - DEVELOPER=1
 # The Linux build installs the defined dependency versions below.
 # The OS X build installs the latest available versions. Keep that
diff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh
index 0edf63acfa..8c2b32f7b3 100755
--- a/ci/run-linux32-docker.sh
+++ b/ci/run-linux32-docker.sh
@@ -13,6 +13,7 @@ docker pull daald/ubuntu32:xenial
 
 docker run \
--interactive \
+   --env BYPASS_TCLTK_CHECK \
--env DEVELOPER \
--env DEFAULT_TEST_TARGET \
--env GIT_PROVE_OPTS \
-- 
2.15.0

Re: [PATCH] git-gui: Prevent double UTF-8 conversion

[Johannes Schindelin]

Hi Łukasz,

On Tue, 28 Nov 2017, Łukasz Stelmach wrote:

> Set encoding to utf-8 for file descriptors used to receive data from git
> commands.

The patch only changes it for the `cat-file` command used to read the
latest commit message for amending.

Maybe the commit message should be adjusted to reflect as much?

> With encoding on the file descriptor set to "binary" Tcl (8.6 in my
> case) does double conversion which breaks e.g. author name in amended
> commits.

Is the problem in question occurring when a commit message contains
non-ASCII characters encoded in UTF-8 and you click the "Amend Last
Commit" radio button on the right side above the "Commit Message" text
box?

I tested this with Tcl/Tk 8.6.7-1 of Git for Windows' SDK, and with Tcl/Tk
8.6.0+9 of Ubuntu 16.04.3 LTS, in both cases the encoding of a commit
message 'äöü' was handled correctly with "binary"...

Could you give more details about your system so it is easier for me to
understand where the difference comes from?

Having said that, I also tested it *with* your patch, and it does not
break anything.

> So, this is the second attempt to draw some attention to this patch,
> since the first one has somewhat failed.

Yeah, sadly Pat (who is still listed as the active Git GUI maintainer) has
fallen very, very silent for over a year.

Pinging about the status of the patch was definitely the right thing to
do.

Ciao,
Johannes

bug deleting "unmerged" branch (2.12.3)

[Ulrich Windl]

Hi!

During a rebase that turned out to be heavier than expected 8-( I decided to 
keep the old branch by creating a temporary branch name to the commit of the 
branch to rebase (which was still the old commit ID at that time).
When done rebasing, I attached a new name to the new (rebased) branch, deleted 
the old name (pointing at the same rebase commit), then recreated the old 
branch from the temporary branch name (created to remember the commit id).
When I wanted to delete the temporary branch (which is of no use now), I got a 
message that the branch is unmerged.
I think if more than one branches are pointing to the same commit, one should 
be allowed to delete all but the last one without warning. Do you agree?

Regards,
Ulrich

DIRECTOR IN CHARGE: DR.PATRICE TEME

[U N-Headquarters]

UN Visitor Centre
Department of Public Information
United Nations Headquarters
Room DHL-1B-154
New York, NY 10017
E-mail:un...@teewars.org

United Nations Compensation Unit, In Affiliation with World Bank Our Ref: 
UN/WBO/042UK/2015.

Congratulations Beneficiary,

How are you today  Hope all is well with you and family  You may not understand 
why this mail came to you. We have been having a meeting for the past 7 months 
which just ended few days ago with the secretary to the UNITED NATIONS. This 
email is to all the people that have been scammed in any part of the world, the 
UNITED NATIONS in Affiliation with WORLD BANK have agreed to compensate them 
with the sum of USD US$980,000.00 Dollars.

This includes every foreign contractors that may have not received their 
contract sum, and people that have had an unfinished transaction or 
international businesses that failed due to Government problems etc. We found 
your name in the list of those who are to benefit from these compensation 
exercise and that is why we are contacting you, this have been agreed upon and 
have been signed. You are advised to contact Dr.PATRICE TEME of our paying 
center in Africa, as he is our representative in Nigeria, contact him 
immediately for your Cheque/ International Bank Draft of US$980,000.00 Dollars.

This fund is in form of a Bank Draft for security purpose ok  So he will send 
it to you and you can clear it in any bank of your choice. Therefore, you 
should send him your full Name and telephone number your correct mailing 
address where you want him to send the Draft to you. Contact Dr.PATRICE TEME of 
MAGNUM PLC PAYMENT CENTER with your payment code:ST/DPI/829 immediately for 
your Cheque at the given address below:

DIRECTOR IN CHARGE: DR.PATRICE TEME
E-MAIL:info-magnumb...@ukcompanies.org
TELEPHONE:+ 234-817-008-4240
FAX: +234-817-008-4240

I apologize on behalf of my organization for any delay you might have 
encountered in receiving your fund in the past. Thanks and God bless you and 
your family. Hoping to hear from you as soon as you cash your Bank Draft. 
Making the world a better place.

You are required to contact the above person and furnish him with the following 
of your information that will be required to avoid any mistakes:-

1. Your Full Name :
2. Your Home/Mobile Telephone No:
3. Your Home or Office Address :
4. Age/Occupation/Marital Status:
5. Scanned copy of your identification:

Congratulations, and I look forward to hear from you as soon as you confirm 
your payment making the world a better place

DIRECTOR IN CHARGE: DR.PATRICE TEME

[U N-Headquarters]

UN Visitor Centre
Department of Public Information
United Nations Headquarters
Room DHL-1B-154
New York, NY 10017
E-mail:un...@teewars.org

United Nations Compensation Unit, In Affiliation with World Bank Our Ref: 
UN/WBO/042UK/2015.

Congratulations Beneficiary,

How are you today  Hope all is well with you and family  You may not understand 
why this mail came to you. We have been having a meeting for the past 7 months 
which just ended few days ago with the secretary to the UNITED NATIONS. This 
email is to all the people that have been scammed in any part of the world, the 
UNITED NATIONS in Affiliation with WORLD BANK have agreed to compensate them 
with the sum of USD US$980,000.00 Dollars.

This includes every foreign contractors that may have not received their 
contract sum, and people that have had an unfinished transaction or 
international businesses that failed due to Government problems etc. We found 
your name in the list of those who are to benefit from these compensation 
exercise and that is why we are contacting you, this have been agreed upon and 
have been signed. You are advised to contact Dr.PATRICE TEME of our paying 
center in Africa, as he is our representative in Nigeria, contact him 
immediately for your Cheque/ International Bank Draft of US$980,000.00 Dollars.

This fund is in form of a Bank Draft for security purpose ok  So he will send 
it to you and you can clear it in any bank of your choice. Therefore, you 
should send him your full Name and telephone number your correct mailing 
address where you want him to send the Draft to you. Contact Dr.PATRICE TEME of 
MAGNUM PLC PAYMENT CENTER with your payment code:ST/DPI/829 immediately for 
your Cheque at the given address below:

DIRECTOR IN CHARGE: DR.PATRICE TEME
E-MAIL:info-magnumb...@ukcompanies.org
TELEPHONE:+ 234-817-008-4240
FAX: +234-817-008-4240

I apologize on behalf of my organization for any delay you might have 
encountered in receiving your fund in the past. Thanks and God bless you and 
your family. Hoping to hear from you as soon as you cash your Bank Draft. 
Making the world a better place.

You are required to contact the above person and furnish him with the following 
of your information that will be required to avoid any mistakes:-

1. Your Full Name :
2. Your Home/Mobile Telephone No:
3. Your Home or Office Address :
4. Age/Occupation/Marital Status:
5. Scanned copy of your identification:

Congratulations, and I look forward to hear from you as soon as you confirm 
your payment making the world a better place

DIRECTOR IN CHARGE: DR.PATRICE TEME

[U N-Headquarters]

UN Visitor Centre
Department of Public Information
United Nations Headquarters
Room DHL-1B-154
New York, NY 10017
E-mail:un...@teewars.org

United Nations Compensation Unit, In Affiliation with World Bank Our Ref: 
UN/WBO/042UK/2015.

Congratulations Beneficiary,

How are you today  Hope all is well with you and family  You may not understand 
why this mail came to you. We have been having a meeting for the past 7 months 
which just ended few days ago with the secretary to the UNITED NATIONS. This 
email is to all the people that have been scammed in any part of the world, the 
UNITED NATIONS in Affiliation with WORLD BANK have agreed to compensate them 
with the sum of USD US$980,000.00 Dollars.

This includes every foreign contractors that may have not received their 
contract sum, and people that have had an unfinished transaction or 
international businesses that failed due to Government problems etc. We found 
your name in the list of those who are to benefit from these compensation 
exercise and that is why we are contacting you, this have been agreed upon and 
have been signed. You are advised to contact Dr.PATRICE TEME of our paying 
center in Africa, as he is our representative in Nigeria, contact him 
immediately for your Cheque/ International Bank Draft of US$980,000.00 Dollars.

This fund is in form of a Bank Draft for security purpose ok  So he will send 
it to you and you can clear it in any bank of your choice. Therefore, you 
should send him your full Name and telephone number your correct mailing 
address where you want him to send the Draft to you. Contact Dr.PATRICE TEME of 
MAGNUM PLC PAYMENT CENTER with your payment code:ST/DPI/829 immediately for 
your Cheque at the given address below:

DIRECTOR IN CHARGE: DR.PATRICE TEME
E-MAIL:info-magnumb...@ukcompanies.org
TELEPHONE:+ 234-817-008-4240
FAX: +234-817-008-4240

I apologize on behalf of my organization for any delay you might have 
encountered in receiving your fund in the past. Thanks and God bless you and 
your family. Hoping to hear from you as soon as you cash your Bank Draft. 
Making the world a better place.

You are required to contact the above person and furnish him with the following 
of your information that will be required to avoid any mistakes:-

1. Your Full Name :
2. Your Home/Mobile Telephone No:
3. Your Home or Office Address :
4. Age/Occupation/Marital Status:
5. Scanned copy of your identification:

Congratulations, and I look forward to hear from you as soon as you confirm 
your payment making the world a better place

Re: [PATCH v3 4/4] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Johannes Schindelin]

Hi Dan, and (based on the timezone recorded in your mail:) good morning!

On Mon, 27 Nov 2017, Dan Jacques wrote:

> > In Git for Windows, we have an almost identical patch:
> >
> > https://github.com/git-for-windows/git/commit/bdd739bb2b0b
> >
> > We just guard the call to system_path() behind a test whether podir is
> > already absolute, but these days, system_path() does that itself.
> >
> > I am too little of a Perl expert to be helpful with the other patches, but
> > I would gladly runa build & test on Windows if you direct me to an
> > easily-pullable branch.
> 
> Oh interesting - I've only peripherally looked at Git-for-Windows code,
> since Chromium uses its packages verbatim (thanks, BTW!). I think you're
> correct though - this patch set seems to be doing the same thing.

Excellent, thanks for confirming.

> I've been force-pushing my changes to the "runtime-prefix" branch of my
> Git fork for travis.ci testing. The latest commit on that branch adds a
> "config.mak" for testing, so one commit from the branch head will
> contain the sum set of this patch series applied at (or near) Git's
> master branch:
> 
> https://github.com/danjacques/git/tree/runtime-prefix~1
> 
> Let me know if this is what you are looking for, and if I can offer any
> help with Windows testing. Thanks!

Thank you, that was exactly what I was looking for.

BTW I think that your config.mak settings are partially unnecessary: the
gitexecdir and template_dir should be identical, and sysconfdir, too (at
least unless you override prefix).

I triggered a build (with the config.mak commit, because it does not
really matter), and it failed immediately due to a quoting issue: the path
separator is a semicolon on Windows and therefore must be quoted (so that
it is not misinterpreted as ending the command). There were a couple of
other issues, too, and I opened a PR here:

https://github.com/danjacques/git/pull/1

Ciao,
Dscho

DIRECTOR IN CHARGE: DR.PATRICE TEME

[U N-Headquarters]

UN Visitor Centre
Department of Public Information
United Nations Headquarters
Room DHL-1B-154
New York, NY 10017
E-mail:un...@teewars.org

United Nations Compensation Unit, In Affiliation with World Bank Our Ref: 
UN/WBO/042UK/2015.

Congratulations Beneficiary,

How are you today  Hope all is well with you and family  You may not understand 
why this mail came to you. We have been having a meeting for the past 7 months 
which just ended few days ago with the secretary to the UNITED NATIONS. This 
email is to all the people that have been scammed in any part of the world, the 
UNITED NATIONS in Affiliation with WORLD BANK have agreed to compensate them 
with the sum of USD US$980,000.00 Dollars.

This includes every foreign contractors that may have not received their 
contract sum, and people that have had an unfinished transaction or 
international businesses that failed due to Government problems etc. We found 
your name in the list of those who are to benefit from these compensation 
exercise and that is why we are contacting you, this have been agreed upon and 
have been signed. You are advised to contact Dr.PATRICE TEME of our paying 
center in Africa, as he is our representative in Nigeria, contact him 
immediately for your Cheque/ International Bank Draft of US$980,000.00 Dollars.

This fund is in form of a Bank Draft for security purpose ok  So he will send 
it to you and you can clear it in any bank of your choice. Therefore, you 
should send him your full Name and telephone number your correct mailing 
address where you want him to send the Draft to you. Contact Dr.PATRICE TEME of 
MAGNUM PLC PAYMENT CENTER with your payment code:ST/DPI/829 immediately for 
your Cheque at the given address below:

DIRECTOR IN CHARGE: DR.PATRICE TEME
E-MAIL:info-magnumb...@ukcompanies.org
TELEPHONE:+ 234-817-008-4240
FAX: +234-817-008-4240

I apologize on behalf of my organization for any delay you might have 
encountered in receiving your fund in the past. Thanks and God bless you and 
your family. Hoping to hear from you as soon as you cash your Bank Draft. 
Making the world a better place.

You are required to contact the above person and furnish him with the following 
of your information that will be required to avoid any mistakes:-

1. Your Full Name :
2. Your Home/Mobile Telephone No:
3. Your Home or Office Address :
4. Age/Occupation/Marital Status:
5. Scanned copy of your identification:

Congratulations, and I look forward to hear from you as soon as you confirm 
your payment making the world a better place

Re: [PATCH v2 0/3] rebase: give precise error message

[Kaartic Sivaraam]

On Tue, 2017-11-28 at 11:25 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> > 1. "git rebase  " does nothing
> 
> Not limited to "rebase", you do not muck with remote-tracking branch
> in your local repository, so it would be a bug if the above updated
> where the remote-tracking branch points at.
> 
> The form of "git rebase" with one extra argument (i.e. not rebasing
> the history that leads to the current checkout) is mere shorthand of
> checking that extra thing out before doing the rebase, i.e.
> 
>   $ git rebase origin/next origin/maint
> 
> first checks out origin/maint (you'd get on a detached HEAD) and
> rebase the history leading to the detached HEAD on top of
> origin/next.  If it fast-forwards (and it should if you are talking
> about 'maint' and 'next' I publish), you'll end up sitting on a
> detached HEAD that points at origin/next.
> 
> There is nothing to see here.
> 

You're right. It was my mistake. It seems I didn't notice that I was
already on 'origin/next' before I did,

    $ git rebase origin/next origin/maint

So (obviously) I thought it did nothing, sorry.


> > 2. It's possible to do "git rebase  "
> 
> Again, that's designed behaviour you can trigger by giving 
> (not ).  Very handy when you do not trust your upstream or
> yourself's ability to resolve potential conflicts as a trial run
> before really committing to perform the rebase, e.g.
> 
>   $ git rebase origin master^0
> 

I can't comment about usefulness as I haven't used rebase in this way
but I'm pretty sure that this should be mentioned in the
"Documentation" to help those might be in bare need of syntax like this
to discover it.

Something like the following diff with additional changes to other
places that refer to ,

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e688..ba4a545bf 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -9,9 +9,9 @@ SYNOPSIS
 
 [verse]
 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
-   [ []]
+   [ []]
 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
-   --root []
+   --root []
 'git rebase' --continue | --skip | --abort | --quit | --edit-todo
 
 DESCRIPTION


If  is the correct substitute , I could try to send a
patch that fixes this.


-- 
Kaartic

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 28 Nov 2017, at 00:18, Junio C Hamano  wrote:
> 
> lars.schnei...@autodesk.com writes:
> 
>> diff to v2:
>>- shortened and localized the "waiting" message
>>- detect "emacsclient" and suppress "waiting" message
> 
> Thanks for moving this forward.
> 
> 
>> +static const char *close_notice = NULL;
> 
> Because this thing is "static", the variable is NULL when the first
> call to this function is made, and the value left in the variable
> when a call returns will be seen by the next call.
> 
>> +if (isatty(2) && !close_notice) {
> 
> Declaring a "static" variable initialized to NULL and checking its
> NULL-ness upfront is a common pattern to make sure that the code
> avoids repeated computation of the same thing.  The body of the if
> statement is run only when standard error stream is a tty (hinting
> an interactive session) *and* close_notice is (still) NULL.
> 
>> +char *term = getenv("TERM");
>> +
>> +if (term && strcmp(term, "dumb"))
>> +/*
>> + * go back to the beginning and erase the
>> + * entire line if the terminal is capable
>> + * to do so, to avoid wasting the vertical
>> + * space.
>> + */
>> +close_notice = "\r\033[K";
>> +else if (term && strstr(term, "emacsclient"))
>> +/*
>> + * `emacsclient` (or `emacsclientw` on Windows) 
>> already prints
>> + * ("Waiting for Emacs...") if a file is opened 
>> for editing.
>> + * Therefore, we don't need to print the editor 
>> launch info.
>> + */
>> +;
>> +else
>> +/* otherwise, complete and waste the line */
>> +close_notice = _("done.\n");
>> +}
> 
> It assigns a non-NULL value to close_notice unless the editor is
> emacsclient (modulo the bug that "emacsclient" is to be compared
> with EDITOR, GIT_EDITOR, core.editor etc. -- git_editor() can be
> used to pick the right one).  For a user of that particular editor,
> it is left as NULL.  Because it is unlikely that EDITOR etc. would
> change across calls to this function, for them, and only for them,
> the above is computed to yield the same result every time this
> function is called.
> 
> That feels a bit uneven, doesn't it?
> 
> There are two possible solutions:
> 
> 1. drop "static" from the declaration to stress the fact that the
>   variable and !close_notice in the upfront if() statement is not
>   avoiding repeated computation of the same thing, or
> 
> 2. arrange that "emacsclient" case also participates in "avoid
>   repeated computation" dance.  While at it, swap the order of
>   checking isatty(2) and !close_notice (aka "have we done this
>   already?)--because we do not expect us swapping file descriptor
>   #2 inside this single process, we'd be repeatedly asking
>   isatty(2) for the same answer.
> 
> The former may be simpler and easier, as an editor invocation would
> not be a performance critical codepath.
> 
> If we want to do the latter, a cleaner way may be to have another
> static "static int use_notice_checked = 0;" declared, and then
> 
>   if (!use_notice_checked && isatty(2)) {
>   ... what you currently have, modulo the
>   ... fix for the editor thing, and set
>   ... close_notice to a string (or NULL).
>use_notice_checked = 1;
>   }
> 
> The users of close_notice after this part that use !close_notice
> as "do not give the notice at all" flag and also as "this is the
> string to show after editor comes back" can stay the same if you go
> this route.  That would be solution 2a.
> 
> Of course, you can instead use close_notice = "" (empty string) as a
> signal "we checked and we know that we are not using the notice
> thing".  If you go that route, then the users after this if statement
> that sets up close_notice upon the first call would say !*close_notice
> instead of !close_notice when they try to see if the notice is in use.
> That would be solution 2b.
> 
> I personally think any one of 1., 2a., or 2b. is fine.

Thanks for your thoughts! I will go with 1. I also think that this is 
no performance critical codepath and therefore we should go with the
simplest/most maintainable solution.


Thanks,
Lars

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 28 Nov 2017, at 00:05, Jeff King  wrote:
> 
> On Mon, Nov 27, 2017 at 08:09:32PM +, brian m. carlson wrote:
> 
>>> Show a message in the original terminal and get rid of it when the
>>> editor returns.
>> [...]
>> 
>> Sorry for coming to the topic so late, but it occurred to me that we
>> might want to conditionalize this on an advice.* flag.  I expect there
>> are some people who will never want to see this, and letting them turn
>> it off would be good.
> 
> I am torn between saying "yes please, I would absolutely set such an
> option myself" and "if we need advice.*, that is a good sign that the
> feature is mis-designed".
> 
> Let me elaborate a bit on the latter.
> 
> My gut feeling is that this is absolutely the wrong place to put a
> message like this. We don't know enough about what the editor is doing,
> so we have to take pains to avoid a crufty message in the terminal,
> including:
> 
>  - playing ANSI-term trickery to erase the message
> 
>  - hard-coding (!) emacsclient as a special case
> 
> And that's why I say that "advice.*" is a bad sign, because it means
> those other techniques are failing, and somebody is seeing and being
> annoyed by the cruft.

I agree with your cruft assessments, especially regarding the hard-coded 
emacsclient thingy. 

However, I really like Brian's "advice" suggestion. Would you be more
in favor of this change if we don't do emacsclient hardcoding and rely on 
"advice.openEditor"? Maybe we could also remove the term trickery but
this seems to be convenient in practice. 

According to the docs advice.* defaults to "true". For my use case it would
be ok if "advice.openEditor" would default to "false" as I distribute
a common Git config via "include.path" to my users. However, that is likely
confusing to the "advice" machinery and users.


> The right place for this message, IMHO, is for the editor itself (or a
> wrapper script) to say "hey, I'm opening a new window" (like emacsclient
> does).

I 100% agree. However, as you mentioned, the world isn't perfect.

Git's core concepts are pretty simple and most people understand them
if you explain them visually. However, applying/using these concepts
is often the problem. If you want to use Git efficiently then you need
to know lots of other things. Being command-line savvy is one of them.
From experience I know that this is hard for many people. Especially
Windows users as they are not used to BASH and Unix-like command-line
tools. That's why I think "advice.openEditor" could help a few people.

- Lars

Re: [PATCH v3 4/4] exec_cmd: RUNTIME_PREFIX on some POSIX systems

[Johannes Schindelin]

Hi Junio,

On Tue, 28 Nov 2017, Junio C Hamano wrote:

> Dan Jacques  writes:
> 
> >> In Git for Windows, we have an almost identical patch:
> >>
> >> https://github.com/git-for-windows/git/commit/bdd739bb2b0b
> >>
> >> We just guard the call to system_path() behind a test whether podir is
> >> already absolute, but these days, system_path() does that itself.
> >>
> >> I am too little of a Perl expert to be helpful with the other patches, but
> >> I would gladly runa build & test on Windows if you direct me to an
> >> easily-pullable branch.
> >
> > Oh interesting - I've only peripherally looked at Git-for-Windows code,
> > since Chromium uses its packages verbatim (thanks, BTW!). I think you're
> > correct though - this patch set seems to be doing the same thing.
> >
> > I've been force-pushing my changes to the "runtime-prefix" branch of my Git
> > fork for travis.ci testing. The latest commit on that branch adds a
> > "config.mak" for testing, so one commit from the branch head will contain
> > the sum set of this patch series applied at (or near) Git's master branch:
> >
> > https://github.com/danjacques/git/tree/runtime-prefix~1
> >
> > Let me know if this is what you are looking for, and if I can offer any
> > help with Windows testing. Thanks!
> 
> FWIW, I plan to include this somewhere on 'pu' for today's
> integration cycle, so dj/runtime-prefix topic branch would also be
> what can easily be grabbed.

Thanks for the offer.

Having said that, I prefer to work with Dan's branch directly, as that
would be the branch that would need changes in case I need to patch
anything. It's better to save the time on the roundtrip through your
branch (that may display other side effects, too, as you almost certainly
chose a different base commit than Dan did).

Also, I could easily offer the changes in a PR which is *a lot* more
convenient on my side.

Ciao,
Dscho

Re: [PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Takuto Ikuta]

Hi,

Thank you for merging!

Takuto

2017-11-28 20:27 GMT+09:00 Johannes Schindelin :
> Hi,
>
> On Tue, 28 Nov 2017, Takuto Ikuta wrote:
>
>> As long as this PR is included in next Git for Windows release, I
>> won't suffer from slow git fetch.
>> https://github.com/git-for-windows/git/pull/1372
>>
>> But I sent you 2 PRs to follow right way.
>> https://github.com/git-for-windows/git/pull/1379
>> https://github.com/git-for-windows/git/pull/1380
>> Feel free to merge these PRs.
>
> I amended them slightly, and merged them.
>
> Thank you,
> Dscho



-- 
Takuto Ikuta
Software Engineer in Tokyo
Chrome Infrastructure (goma team)

Re: [PATCH] Use OBJECT_INFO_QUICK to speedup git fetch-pack

[Johannes Schindelin]

Hi,

On Tue, 28 Nov 2017, Takuto Ikuta wrote:

> As long as this PR is included in next Git for Windows release, I
> won't suffer from slow git fetch.
> https://github.com/git-for-windows/git/pull/1372
> 
> But I sent you 2 PRs to follow right way.
> https://github.com/git-for-windows/git/pull/1379
> https://github.com/git-for-windows/git/pull/1380
> Feel free to merge these PRs.

I amended them slightly, and merged them.

Thank you,
Dscho

[PATCH] git-gui: Prevent double UTF-8 conversion

[Łukasz Stelmach]

Set encoding to utf-8 for file descriptors used to receive data from
git commands.

With encoding on the file descriptor set to "binary" Tcl (8.6 in my case)
does double conversion which breaks e.g. author name in amended commits.

For example "\305\201ukasz" (as written by git cat-file) becomes
"\303\205\302\201ukasz".

Signed-off-by: Łukasz Stelmach 
---
 git-gui/lib/commit.tcl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

So, this is the second attempt to draw some attention to this patch,
since the first one has somewhat failed.

https://marc.info/?l=git=150331641702091=2

diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 83620b7cb..bcb6499a0 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -26,7 +26,7 @@ You are currently in the middle of a merge that has not been 
fully completed.  Y
set parents [list]
if {[catch {
set fd [git_read cat-file commit $curHEAD]
-   fconfigure $fd -encoding binary -translation lf
+   fconfigure $fd -encoding utf-8 -translation lf
# By default commits are assumed to be in utf-8
set enc utf-8
while {[gets $fd line] > 0} {
-- 
2.11.0