Git development
 help / color / mirror / Atom feed
* Re: [PATCHv7] pathspec: give better message for submodule related pathspec error
From: Junio C Hamano @ 2017-01-09 22:53 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git, bmwill, peff
In-Reply-To: <20170109224330.28405-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1]. The usual response from the mailing
> list is link to old discussions[2], and acknowledging the problem
> stating it is known.
>
> This patch accomplishes two things:
>
>   1. Switch assert() to die("BUG") to give a more readable message.
>
>   2. Take one of the cases where we hit a BUG and turn it into a normal
>      "there was something wrong with the input" message.
>
>      This assertion triggered for cases where there wasn't a programming
>      bug, but just bogus input. In particular, if the user asks for a
>      pathspec that is inside a submodule, we shouldn't assert() or
>      die("BUG"); we should tell the user their request is bogus.
>

Is it only me who sees funny black rectangles in front of these four
lines instead of blanks, by the way?

>   This comes as a single patch again, replacing sb/pathspec-errors.
>   It goes directly on top of bw/pathspec-cleanup.
>   
>   v7:
>   do not rely on "test_commit -C" being there, nor the infrastructure
>   to request a "good" submodule upstream. Just create a submodule outselves
>   to test in.
>   

Thanks.

> diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
> new file mode 100755
> index 0000000000..d952ae2cae
> --- /dev/null
> +++ b/t/t6134-pathspec-in-submodule.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +
> +test_description='test case exclude pathspec'
> +
> +TEST_CREATE_SUBMODULE=yes

Did you mean to keep this?

> +. ./test-lib.sh
> +
> +test_expect_success 'setup a submodule' '
> +	test_create_repo pretzel &&
> +	(
> +		cd pretzel &&
> +		touch a &&

This is better spelled as

		: >a &&

because use of touch, when you do not care about the file timestamp,
is misleading.

Thanks.

^ permalink raw reply

* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-09 22:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, davvid, j6t, sbeller, simon
In-Reply-To: <xmqqvatot5ob.fsf@gitster.mtv.corp.google.com>

[-- Attachment #1: Type: text/plain, Size: 5501 bytes --]

On 2017-01-09 14:05, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if it makes more sense to always move to toplevel upfront
>> and consistently use path from the toplevel, perhaps like the patch
>
> s/the patch/the attached patch/ I meant.
>
>> does.  The first hunk is what you wrote but only inside MERGE_RR
>> block, and the second hunk deals with converting end-user supplied
>> paths that are relative to the original relative to the top-level.
>>
>> The tweaking of $orderfile you have in the first hunk may have to be
>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>> in the second hunk to avoid confusion in case orderfile specified by
>> the end user happens to be the same as a valid revname
>> (e.g. "master").
>
> And here is a squash-able patch to illustrate what I mean.

Thanks for this; I'll cook up a reroll.

I tried this approach before I emailed the v3 reroll, except I left out 
the "--" argument to rev-parse.  This caused the tests to fail due to 
"ambiguous argument: unknown revision or path not in the working tree". 
I didn't think to add the "--" because I got it in my head that "--" 
shouldn't be used with --prefix because it shows up in the output, plus 
the example in the rev-parse documentation doesn't use it.  I'll patch 
rev-parse's man page to use "--" in the example.

Thanks,
Richard

>
> I removed both of the comment blocks as the code always works with
> the worktree-relative pathname after this patch while adjusting
> end-user supplied paths from relative to original cwd.  As that is
> how the core parts of the system (including the parts written in C)
> work, even though an explanation you did in the log message is
> needed to explain why the change was needed and what the change
> intended to do to readers of "git log", it is not necessary to
> explain it to the readers of the latest code, which is what the
> in-code comment is about.
>
> The single-liner addition to the test creates a branch whose name is
> the same as the specified orderfile to deliberately create a
> confusing situation.  I haven't tried, but I am fairly sure that the
> test will demonstrate how broken the orderfile=$(...) in the
> original is, if you apply the test part of the attached patch,
> without the changes to git-mergetool.sh, to your version.
>
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 22f56c25a2..21f82d5b58 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -454,53 +454,34 @@ main () {
>  	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
>  	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
>
> -	if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> +	prefix=$(git rev-parse --show-prefix) || exit 1
> +	cd_to_toplevel
> +
> +	if test -n "$orderfile"
>  	then
> -		# The pathnames output by the 'git rerere remaining'
> -		# command below are relative to the top-level
> -		# directory but the 'git diff --name-only' command
> -		# further below expects the pathnames to be relative
> -		# to the current working directory.  Thus, we cd to
> -		# the top-level directory before running 'git diff
> -		# --name-only'.  We change directories even earlier
> -		# (before running 'git rerere remaining') in case 'git
> -		# rerere remaining' is ever changed to output
> -		# pathnames relative to the current working directory.
> -		#
> -		# Changing directories breaks a relative $orderfile
> -		# pathname argument, so fix it up to be relative to
> -		# the top-level directory.
> -
> -		prefix=$(git rev-parse --show-prefix) || exit 1
> -		cd_to_toplevel
> -		if test -n "$orderfile"
> -		then
> -			orderfile=$(git rev-parse --prefix "$prefix" "$orderfile") || exit 1
> -		fi
> +		orderfile=$(
> +			git rev-parse --prefix "$prefix" -- "$orderfile" |
> +			sed -e 1d
> +		)
> +	fi
>
> +	if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
> +	then
>  		set -- $(git rerere remaining)
>  		if test $# -eq 0
>  		then
>  			print_noop_and_exit
>  		fi
> +	elif test $# -ge 0
> +	then
> +		eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> +		shift
>  	fi
>
> -	# Note:  The pathnames output by 'git diff --name-only' are
> -	# relative to the top-level directory, but it expects input
> -	# pathnames to be relative to the current working directory.
> -	# Thus:
> -	#   * Either cd_to_toplevel must not be run before this or all
> -	#     relative input pathnames must be converted to be
> -	#     relative to the top-level directory (or absolute).
> -	#   * Either cd_to_toplevel must be run after this or all
> -	#     relative output pathnames must be converted to be
> -	#     relative to the current working directory (or absolute).
>  	files=$(git -c core.quotePath=false \
>  		diff --name-only --diff-filter=U \
>  		${orderfile:+"-O$orderfile"} -- "$@")
>
> -	cd_to_toplevel
> -
>  	if test -z "$files"
>  	then
>  		print_noop_and_exit
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index dfd641d34b..180dd7057a 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -678,6 +678,11 @@ test_expect_success 'diff.orderFile configuration is honored' '
>  		b
>  		a
>  	EOF
> +
> +	# make sure "order-file" that is ambiguous between
> +	# rev and path is understood correctly.
> +	git branch order-file HEAD &&
> +
>  	git mergetool --no-prompt --tool myecho >output &&
>  	git grep --no-index -h -A2 Merging: output >actual &&
>  	test_cmp expect actual
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply

* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-09 23:02 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <xmqqinpnuaxl.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> I ran out of time for the day and haven't looked at individual
> patches yet.  I may find other issues, but from the interdiff, that
> was the only thing that I found even worse than the previous round,
> and everything else was either the same or has improved.

... I forgot to say a few things:

 * In the meantime, I think this is overall better than what we had
   for the past few weeks; I'll try to replace the copy in my tree
   with this round, even though this may not be the final one.

 * There were a few style issues [*1*] pointed out by the
   checkpatch.pl script borrowed from the kernel land.

 * And finally, you wrote "This marks the count down to '3': two
   more patch series after this (really tiny ones) and we have a
   faster rebase -i."  This is seriously an exciting news.  Thanks.

I probably am still forgetting to say a few more things, but I'd
send this message out in this shape for now.  Thanks.


[Footnote]

*1*

Subject: [PATCH v3 10/38] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
ERROR: do not use assignment in if condition
#118: FILE: sequencer.c:725:
+		if (buf.buf[0] != comment_line_char ||

ERROR: do not use assignment in if condition
#142: FILE: sequencer.c:749:
+		if (!(head_commit = lookup_commit_reference(head)))

ERROR: do not use assignment in if condition
#144: FILE: sequencer.c:751:
+		if (!(head_message = get_commit_buffer(head_commit, NULL)))

ERROR: do not use assignment in if condition
#167: FILE: sequencer.c:774:
+	if (!(message = get_commit_buffer(commit, NULL)))

total: 4 errors, 1 warnings, 320 lines checked
------------------------------------------------
Subject: [PATCH v3 12/38] sequencer (rebase -i): write an author-script file
ERROR: do not use assignment in if condition
#35: FILE: sequencer.c:500:
+		else if ((eol = strchr(message, '\n')))

total: 1 errors, 0 warnings, 62 lines checked
------------------------------------------------
Subject: [PATCH v3 23/38] sequencer (rebase -i): copy commit notes at end
ERROR: open brace '{' following function declarations go on the next line
#36: FILE: sequencer.c:862:
+static void flush_rewritten_pending(void) {

ERROR: do not use assignment in if condition
#41: FILE: sequencer.c:867:
+	if (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 &&

total: 2 errors, 0 warnings, 112 lines checked

^ permalink raw reply

* Re: [PATCHv7] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 23:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Williams, Jeff King
In-Reply-To: <xmqqeg0bu9oq.fsf@gitster.mtv.corp.google.com>

On Mon, Jan 9, 2017 at 2:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Is it only me who sees funny black rectangles in front of these four
> lines instead of blanks, by the way?

I copied this part literally from Jeff. My mail display-client (gmail
at the time)
did mangle it apparently, (it inserted some non-breaking white space characters
as well as a character that I can not really identify, as it is no
valid UTF8 and bogus
ASCII)


>> +TEST_CREATE_SUBMODULE=yes
>
> Did you mean to keep this?

No.


> because use of touch, when you do not care about the file timestamp,
> is misleading.

makes sense.

I'll reroll again.

>
> Thanks.

^ permalink raw reply

* [PATCHv8] pathspec: give better message for submodule related pathspec error
From: Stefan Beller @ 2017-01-09 23:16 UTC (permalink / raw)
  To: gitster; +Cc: git, bmwill, peff, Stefan Beller
In-Reply-To: <CAGZ79kZVKFvijfwEfmiaahz2VYgaa=m2m-ty2tbOXh2toO_muQ@mail.gmail.com>

Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.

This patch accomplishes two things:

  1. Switch assert() to die("BUG") to give a more readable message.

  2. Take one of the cases where we hit a BUG and turn it into a normal
     "there was something wrong with the input" message.

     This assertion triggered for cases where there wasn't a programming
     bug, but just bogus input. In particular, if the user asks for a
     pathspec that is inside a submodule, we shouldn't assert() or
     die("BUG"); we should tell the user their request is bogus.

     The only reason we did not check for it, is the expensive nature
     of such a check, so callers avoid setting the flag
     PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
     to bogus input, the expense of CPU cycles spent outweighs the user
     wondering what went wrong, so run that check unconditionally before
     dying with a more generic error message.

Note: There is a case (e.g. "git -C submodule add .") in which we call
strip_submodule_slash_expensive, as git-add requests it via the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to
trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,
such that we executed

	if (item->nowildcard_len < prefixlen)
		item->nowildcard_len = prefixlen;

and prefixlen was not adapted (e.g. it was computed from "submodule/")
So in the die_inside_submodule_path function we also need handle paths,
that were stripped before, i.e. are the exact submodule path. This
is why the conditions in die_inside_submodule_path are slightly
different than in strip_submodule_slash_expensive.

[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
    https://www.spinics.net/lists/git/msg249473.html

Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

  This comes as a single patch again, replacing sb/pathspec-errors.
  It goes directly on top of bw/pathspec-cleanup.

  v8:
  cleaned white spaces in commit message
  removed TEST_CREATE_SUBMODULE=yes
  : > instead of touch.

  v7:
  do not rely on "test_commit -C" being there, nor the infrastructure
  to request a "good" submodule upstream. Just create a submodule outselves
  to test in.

  Thanks,
  Stefan

 pathspec.c                       | 35 +++++++++++++++++++++++++++++++++--
 t/t6134-pathspec-in-submodule.sh | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index ff2509ddd1..7ababb3159 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)
 	}
 }
 
+static void die_inside_submodule_path(struct pathspec_item *item)
+{
+	int i;
+
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		int ce_len = ce_namelen(ce);
+
+		if (!S_ISGITLINK(ce->ce_mode))
+			continue;
+
+		if (item->len < ce_len ||
+		    !(item->match[ce_len] == '/' || item->match[ce_len] == '\0') ||
+		    memcmp(ce->name, item->match, ce_len))
+			continue;
+
+		die(_("Pathspec '%s' is in submodule '%.*s'"),
+		    item->original, ce_len, ce->name);
+	}
+}
+
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
 	}
 
 	/* sanity checks, pathspec matchers assume these are sane */
-	assert(item->nowildcard_len <= item->len &&
-	       item->prefix         <= item->len);
+	if (item->nowildcard_len > item->len ||
+	    item->prefix         > item->len) {
+		/*
+		 * This case can be triggered by the user pointing us to a
+		 * pathspec inside a submodule, which is an input error.
+		 * Detect that here and complain, but fallback in the
+		 * non-submodule case to a BUG, as we have no idea what
+		 * would trigger that.
+		 */
+		die_inside_submodule_path(item);
+		die ("BUG: item->nowildcard_len > item->len || item->prefix > item->len)");
+	}
 }
 
 static int pathspec_item_cmp(const void *a_, const void *b_)
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 0000000000..fd401ca605
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+	test_create_repo pretzel &&
+	: >pretzel/a &&
+	git -C pretzel add a &&
+	git -C pretzel commit -m "add a file" -- a &&
+	git submodule add ./pretzel sub &&
+	git commit -a -m "add submodule" &&
+	git submodule deinit --all
+'
+
+cat <<EOF >expect
+fatal: Pathspec 'sub/a' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule' '
+	echo a >sub/a &&
+	test_must_fail git add sub/a 2>actual &&
+	test_cmp expect actual
+'
+
+cat <<EOF >expect
+fatal: Pathspec '.' is in submodule 'sub'
+EOF
+
+test_expect_success 'error message for path inside submodule from within submodule' '
+	test_must_fail git -C sub add . 2>actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.11.0.rc2.30.g7c4be45.dirty


^ permalink raw reply related

* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Junio C Hamano @ 2017-01-09 23:29 UTC (permalink / raw)
  To: Richard Hansen; +Cc: git, davvid, j6t, sbeller, simon
In-Reply-To: <xmqqvatot5ob.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I wonder if it makes more sense to always move to toplevel upfront
>> and consistently use path from the toplevel, perhaps like the patch
>
> s/the patch/the attached patch/ I meant.
>
>> does.  The first hunk is what you wrote but only inside MERGE_RR
>> block, and the second hunk deals with converting end-user supplied
>> paths that are relative to the original relative to the top-level.
>>
>> The tweaking of $orderfile you have in the first hunk may have to be
>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>> in the second hunk to avoid confusion in case orderfile specified by
>> the end user happens to be the same as a valid revname
>> (e.g. "master").
>
> And here is a squash-able patch to illustrate what I mean.

By the way, I didn't think this through, but how is the orderfile
that comes from the configuration file handled when it is not an
absolute path?  I think it is _wrong_ to take it as relative to
where the user started the program.  The -O<file> parameter from the
command line, when <file> is not absolute, should be taken as
relative to where the user _thinks_ s/he is, but when it comes from
the diff.orderfile configuration and it is not absolute, it should
be taken as relative to the top of the working tree.  As we always
cd_to_top with the suggested SQUASH, it means that the orderfile
that came from the configuration does not have to be touched, while
the orderfile given via -O<file> on the command line needs
prefixing.




^ permalink raw reply

* [PATCH v4 00/14] fix mergetool+rerere+subdir regression
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109054238.42599-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

If rerere is enabled, no pathnames are given, and mergetool is run
from a subdirectory, mergetool always prints "No files need merging".
Fix the bug.

This regression was introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Changes since v3:
  * Update rev-parse docs to use "--" in the --prefix example.
  * Unconditionally run cd_to_toplevel at the beginning to simplify
    the control flow.  Thanks Junio!
  * Test -O<pathname> when there is also a branch named <pathname>.

Richard Hansen (14):
  .mailmap: Use my personal email address as my canonical
  rev-parse doc: use "--" in the --prefix example
  t7610: update branch names to match test number
  t7610: Move setup code to the 'setup' test case.
  t7610: use test_when_finished for cleanup tasks
  t7610: don't rely on state from previous test
  t7610: run 'git reset --hard' after each test to clean up
  t7610: delete some now-unnecessary 'git reset --hard' lines
  t7610: always work on a test-specific branch
  t7610: don't assume the checked-out commit
  t7610: spell 'git reset --hard' consistently
  t7610: add test case for rerere+mergetool+subdir bug
  mergetool: take the "-O" out of $orderfile
  mergetool: fix running in subdir when rerere enabled

 .mailmap                        |   2 +
 Documentation/git-rev-parse.txt |   3 +-
 git-mergetool.sh                |  20 ++-
 t/t7610-mergetool.sh            | 281 ++++++++++++++++++++++++----------------
 4 files changed, 186 insertions(+), 120 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply

* [PATCH v4 01/14] .mailmap: Use my personal email address as my canonical
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

When I changed employers my work address changed from rhansen@bbn.com
to hansenr@google.com.  Rather than map my old work address to my new,
map them both to my permanent personal email address.  (I will still
use my work address in commits I submit so that my employer gets some
credit.)

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 .mailmap | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.mailmap b/.mailmap
index 9cc33e925..9c87a3840 100644
--- a/.mailmap
+++ b/.mailmap
@@ -192,6 +192,8 @@ Philippe Bruhat <book@cpan.org>
 Ralf Thielow <ralf.thielow@gmail.com> <ralf.thielow@googlemail.com>
 Ramsay Jones <ramsay@ramsayjones.plus.com> <ramsay@ramsay1.demon.co.uk>
 René Scharfe <l.s.r@web.de> <rene.scharfe@lsrfire.ath.cx>
+Richard Hansen <rhansen@rhansen.org> <hansenr@google.com>
+Richard Hansen <rhansen@rhansen.org> <rhansen@bbn.com>
 Robert Fitzsimons <robfitz@273k.net>
 Robert Shearman <robertshearman@gmail.com> <rob@codeweavers.com>
 Robert Zeh <robert.a.zeh@gmail.com>
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 02/14] rev-parse doc: use "--" in the --prefix example
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]

The "--" argument avoids "ambiguous argument: unknown revision or
path not in the working tree" errors when a pathname argument refers
to a non-existent file.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/git-rev-parse.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index b6c6326cd..f1190765f 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -91,7 +91,8 @@ repository.  For example:
 ----
 prefix=$(git rev-parse --show-prefix)
 cd "$(git rev-parse --show-toplevel)"
-eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")"
+eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
+shift
 ----
 
 --verify::
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 04/14] t7610: Move setup code to the 'setup' test case.
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 3682 bytes --]

Multiple test cases depend on these hunks, so move them to the 'setup'
test case.  This is a step toward making the tests more independent so
that if one test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 65 ++++++++++++++++++++++++++++------------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..550838a1c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -55,6 +55,22 @@ test_expect_success 'setup' '
 	git rm file12 &&
 	git commit -m "branch1 changes" &&
 
+	git checkout -b delete-base branch1 &&
+	mkdir -p a/a &&
+	(echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
+	git add a/a/file.txt &&
+	git commit -m"base file" &&
+	git checkout -b move-to-b delete-base &&
+	mkdir -p b/b &&
+	git mv a/a/file.txt b/b/file.txt &&
+	(echo one; echo two; echo 4) >b/b/file.txt &&
+	git commit -a -m"move to b" &&
+	git checkout -b move-to-c delete-base &&
+	mkdir -p c/c &&
+	git mv a/a/file.txt c/c/file.txt &&
+	(echo one; echo two; echo 3) >c/c/file.txt &&
+	git commit -a -m"move to c" &&
+
 	git checkout -b stash1 master &&
 	echo stash1 change file11 >file11 &&
 	git add file11 &&
@@ -86,6 +102,23 @@ test_expect_success 'setup' '
 	git rm file11 &&
 	git commit -m "master updates" &&
 
+	git clean -fdx &&
+	git checkout -b order-file-start master &&
+	echo start >a &&
+	echo start >b &&
+	git add a b &&
+	git commit -m start &&
+	git checkout -b order-file-side1 order-file-start &&
+	echo side1 >a &&
+	echo side1 >b &&
+	git add a b &&
+	git commit -m side1 &&
+	git checkout -b order-file-side2 order-file-start &&
+	echo side2 >a &&
+	echo side2 >b &&
+	git add a b &&
+	git commit -m side2 &&
+
 	git config merge.tool mytool &&
 	git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
 	git config mergetool.mytool.trustExitCode true &&
@@ -244,21 +277,7 @@ test_expect_success 'mergetool takes partial path' '
 '
 
 test_expect_success 'mergetool delete/delete conflict' '
-	git checkout -b delete-base branch1 &&
-	mkdir -p a/a &&
-	(echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
-	git add a/a/file.txt &&
-	git commit -m"base file" &&
-	git checkout -b move-to-b delete-base &&
-	mkdir -p b/b &&
-	git mv a/a/file.txt b/b/file.txt &&
-	(echo one; echo two; echo 4) >b/b/file.txt &&
-	git commit -a -m"move to b" &&
-	git checkout -b move-to-c delete-base &&
-	mkdir -p c/c &&
-	git mv a/a/file.txt c/c/file.txt &&
-	(echo one; echo two; echo 3) >c/c/file.txt &&
-	git commit -a -m"move to c" &&
+	git checkout move-to-c &&
 	test_must_fail git merge move-to-b &&
 	echo d | git mergetool a/a/file.txt &&
 	! test -f a/a/file.txt &&
@@ -607,26 +626,12 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
 '
 
 test_expect_success 'diff.orderFile configuration is honored' '
+	git checkout order-file-side2 &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
 	echo b >order-file &&
 	echo a >>order-file &&
-	git checkout -b order-file-start master &&
-	echo start >a &&
-	echo start >b &&
-	git add a b &&
-	git commit -m start &&
-	git checkout -b order-file-side1 order-file-start &&
-	echo side1 >a &&
-	echo side1 >b &&
-	git add a b &&
-	git commit -m side1 &&
-	git checkout -b order-file-side2 order-file-start &&
-	echo side2 >a &&
-	echo side2 >b &&
-	git add a b &&
-	git commit -m side2 &&
 	test_must_fail git merge order-file-side1 &&
 	cat >expect <<-\EOF &&
 		Merging:
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 05/14] t7610: use test_when_finished for cleanup tasks
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 9927 bytes --]

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 71 +++++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 550838a1c..f62ceffdc 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,11 @@ test_expect_success 'custom mergetool' '
 '
 
 test_expect_success 'mergetool crlf' '
+	test_when_finished "git reset --hard" &&
+	# This test_config line must go after the above reset line so that
+	# core.autocrlf is unconfigured before reset runs.  (The
+	# test_config command uses test_when_finished internally and
+	# test_when_finished is LIFO.)
 	test_config core.autocrlf true &&
 	git checkout -b test$test_count branch1 &&
 	test_must_fail git merge master >/dev/null 2>&1 &&
@@ -161,9 +166,7 @@ test_expect_success 'mergetool crlf' '
 	test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
 	git submodule update -N &&
 	test "$(cat submod/bar)" = "master submodule" &&
-	git commit -m "branch1 resolved with mergetool - autocrlf" &&
-	test_config core.autocrlf false &&
-	git reset --hard
+	git commit -m "branch1 resolved with mergetool - autocrlf"
 '
 
 test_expect_success 'mergetool in subdir' '
@@ -194,6 +197,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	test_must_fail git merge master &&
@@ -202,8 +206,7 @@ test_expect_success 'mergetool skips autoresolved' '
 	( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
 	( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
 	output="$(git mergetool --no-prompt)" &&
-	test "$output" = "No files need merging" &&
-	git reset --hard
+	test "$output" = "No files need merging"
 '
 
 test_expect_success 'mergetool merges all from subdir' '
@@ -223,6 +226,7 @@ test_expect_success 'mergetool merges all from subdir' '
 '
 
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
+	test_when_finished "git reset --hard" &&
 	test_config rerere.enabled true &&
 	rm -rf .git/rr-cache &&
 	git checkout -b test$test_count branch1 &&
@@ -232,8 +236,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
 	( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
 	git submodule update -N &&
 	output="$(yes "n" | git mergetool --no-prompt)" &&
-	test "$output" = "No files need merging" &&
-	git reset --hard
+	test "$output" = "No files need merging"
 '
 
 test_expect_success 'conflicted stash sets up rerere'  '
@@ -264,6 +267,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
 '
 
 test_expect_success 'mergetool takes partial path' '
+	test_when_finished "git reset --hard" &&
 	git reset --hard &&
 	test_config rerere.enabled false &&
 	git checkout -b test$test_count branch1 &&
@@ -272,11 +276,11 @@ test_expect_success 'mergetool takes partial path' '
 
 	( yes "" | git mergetool subdir ) &&
 
-	test "$(cat subdir/file3)" = "master new sub" &&
-	git reset --hard
+	test "$(cat subdir/file3)" = "master new sub"
 '
 
 test_expect_success 'mergetool delete/delete conflict' '
+	test_when_finished "git reset --hard HEAD" &&
 	git checkout move-to-c &&
 	test_must_fail git merge move-to-b &&
 	echo d | git mergetool a/a/file.txt &&
@@ -288,29 +292,30 @@ test_expect_success 'mergetool delete/delete conflict' '
 	git reset --hard HEAD &&
 	test_must_fail git merge move-to-b &&
 	! echo a | git mergetool a/a/file.txt &&
-	! test -f a/a/file.txt &&
-	git reset --hard HEAD
+	! test -f a/a/file.txt
 '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
+	test_when_finished "git reset --hard HEAD" &&
 	test_config mergetool.keepBackup true &&
 	test_must_fail git merge move-to-b &&
 	: >expect &&
 	echo d | git mergetool a/a/file.txt 2>actual &&
 	test_cmp expect actual &&
-	! test -d a &&
-	git reset --hard HEAD
+	! test -d a
 '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
+	test_when_finished "git reset --hard HEAD" &&
 	test_config mergetool.keepTemporaries false &&
 	test_must_fail git merge move-to-b &&
 	echo d | git mergetool a/a/file.txt &&
-	! test -d a &&
-	git reset --hard HEAD
+	! test -d a
 '
 
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
+	test_when_finished "git reset --hard HEAD" &&
+	test_when_finished "git clean -fdx" &&
 	test_config mergetool.keepTemporaries true &&
 	test_must_fail git merge move-to-b &&
 	! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -321,12 +326,11 @@ test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
 	file_REMOTE_.txt
 	EOF
 	ls -1 a/a | sed -e "s/[0-9]*//g" >actual &&
-	test_cmp expect actual &&
-	git clean -fdx &&
-	git reset --hard HEAD
+	test_cmp expect actual
 '
 
 test_expect_success 'deleted vs modified submodule' '
+	test_when_finished "git reset --hard HEAD" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	mv submod submod-movedaside &&
@@ -391,8 +395,7 @@ test_expect_success 'deleted vs modified submodule' '
 	test "$(cat submod/bar)" = "master submodule" &&
 	output="$(git mergetool --no-prompt)" &&
 	test "$output" = "No files need merging" &&
-	git commit -m "Merge resolved by keeping module" &&
-	git reset --hard HEAD
+	git commit -m "Merge resolved by keeping module"
 '
 
 test_expect_success 'file vs modified submodule' '
@@ -479,6 +482,7 @@ test_expect_success 'submodule in subdirectory' '
 		git commit -m "add initial versions"
 		)
 	) &&
+	test_when_finished "rm -rf subdir/subdir_module" &&
 	git submodule add git://example.com/subsubmodule subdir/subdir_module &&
 	git add subdir/subdir_module &&
 	git commit -m "add submodule in subdirectory" &&
@@ -523,8 +527,7 @@ test_expect_success 'submodule in subdirectory' '
 	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
 	git submodule update -N &&
 	test "$(cat subdir/subdir_module/file15)" = "test$test_count.a" &&
-	git commit -m "branch1 resolved with mergetool" &&
-	rm -rf subdir/subdir_module
+	git commit -m "branch1 resolved with mergetool"
 '
 
 test_expect_success 'directory vs modified submodule' '
@@ -578,34 +581,34 @@ test_expect_success 'directory vs modified submodule' '
 '
 
 test_expect_success 'file with no base' '
+	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
 	git checkout -b test$test_count branch1 &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool mybase -- both &&
 	>expected &&
-	test_cmp both expected &&
-	git reset --hard master >/dev/null 2>&1
+	test_cmp both expected
 '
 
 test_expect_success 'custom commands override built-ins' '
+	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
 	git checkout -b test$test_count branch1 &&
 	test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
 	test_config mergetool.defaults.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool defaults -- both &&
 	echo master both added >expected &&
-	test_cmp both expected &&
-	git reset --hard master >/dev/null 2>&1
+	test_cmp both expected
 '
 
 test_expect_success 'filenames seen by tools start with ./' '
+	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
 	git checkout -b test$test_count branch1 &&
 	test_config mergetool.writeToTemp false &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
-	grep ^\./both_LOCAL_ actual >/dev/null &&
-	git reset --hard master >/dev/null 2>&1
+	grep ^\./both_LOCAL_ actual >/dev/null
 '
 
 test_lazy_prereq MKTEMP '
@@ -614,6 +617,7 @@ test_lazy_prereq MKTEMP '
 '
 
 test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
+	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
 	git checkout -b test$test_count branch1 &&
 	test_config mergetool.writeToTemp true &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -621,11 +625,11 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool myecho -- both >actual &&
 	test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
-	grep /both_LOCAL_ actual >/dev/null &&
-	git reset --hard master >/dev/null 2>&1
+	grep /both_LOCAL_ actual >/dev/null
 '
 
 test_expect_success 'diff.orderFile configuration is honored' '
+	test_when_finished "git reset --hard >/dev/null" &&
 	git checkout order-file-side2 &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -640,10 +644,10 @@ test_expect_success 'diff.orderFile configuration is honored' '
 	EOF
 	git mergetool --no-prompt --tool myecho >output &&
 	git grep --no-index -h -A2 Merging: output >actual &&
-	test_cmp expect actual &&
-	git reset --hard >/dev/null
+	test_cmp expect actual
 '
 test_expect_success 'mergetool -Oorder-file is honored' '
+	test_when_finished "git reset --hard >/dev/null 2>&1" &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
@@ -667,8 +671,7 @@ test_expect_success 'mergetool -Oorder-file is honored' '
 	EOF
 	git mergetool -Oorder-file --no-prompt --tool myecho >output &&
 	git grep --no-index -h -A2 Merging: output >actual &&
-	test_cmp expect actual &&
-	git reset --hard >/dev/null 2>&1
+	test_cmp expect actual
 '
 
 test_done
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 08/14] t7610: delete some now-unnecessary 'git reset --hard' lines
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

Tests now always run 'git reset --hard' at the end (even if they
fail), so it's no longer necessary to run 'git reset --hard' at the
beginning of a test.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 55587504e..7d5e1df88 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -184,7 +184,6 @@ test_expect_success 'mergetool in subdir' '
 
 test_expect_success 'mergetool on file in parent dir' '
 	test_when_finished "git reset --hard" &&
-	git reset --hard &&
 	git submodule update -N &&
 	(
 		cd subdir &&
@@ -277,7 +276,6 @@ test_expect_success 'conflicted stash sets up rerere'  '
 
 test_expect_success 'mergetool takes partial path' '
 	test_when_finished "git reset --hard" &&
-	git reset --hard &&
 	test_config rerere.enabled false &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 10/14] t7610: don't assume the checked-out commit
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]

Always check out the required commit at the beginning of the test so
that a failure in a previous test does not cause the test to work off
of the wrong commit.

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index efcf5c3f1..54164a320 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -184,7 +184,7 @@ test_expect_success 'mergetool in subdir' '
 
 test_expect_success 'mergetool on file in parent dir' '
 	test_when_finished "git reset --hard" &&
-	git checkout -b test$test_count &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	(
 		cd subdir &&
@@ -218,7 +218,7 @@ test_expect_success 'mergetool skips autoresolved' '
 
 test_expect_success 'mergetool merges all from subdir' '
 	test_when_finished "git reset --hard" &&
-	git checkout -b test$test_count &&
+	git checkout -b test$test_count branch1 &&
 	test_config rerere.enabled false &&
 	(
 		cd subdir &&
@@ -306,7 +306,7 @@ test_expect_success 'mergetool delete/delete conflict' '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
 	test_when_finished "git reset --hard HEAD" &&
-	git checkout -b test$test_count &&
+	git checkout -b test$test_count move-to-c &&
 	test_config mergetool.keepBackup true &&
 	test_must_fail git merge move-to-b &&
 	: >expect &&
@@ -317,7 +317,7 @@ test_expect_success 'mergetool produces no errors when keepBackup is used' '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
 	test_when_finished "git reset --hard HEAD" &&
-	git checkout -b test$test_count &&
+	git checkout -b test$test_count move-to-c &&
 	test_config mergetool.keepTemporaries false &&
 	test_must_fail git merge move-to-b &&
 	echo d | git mergetool a/a/file.txt &&
@@ -327,7 +327,7 @@ test_expect_success 'mergetool honors tempfile config for deleted files' '
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
 	test_when_finished "git reset --hard HEAD" &&
 	test_when_finished "git clean -fdx" &&
-	git checkout -b test$test_count &&
+	git checkout -b test$test_count move-to-c &&
 	test_config mergetool.keepTemporaries true &&
 	test_must_fail git merge move-to-b &&
 	! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -663,7 +663,7 @@ test_expect_success 'diff.orderFile configuration is honored' '
 '
 test_expect_success 'mergetool -Oorder-file is honored' '
 	test_when_finished "git reset --hard >/dev/null 2>&1" &&
-	git checkout -b test$test_count &&
+	git checkout -b test$test_count order-file-side2 &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 13/14] mergetool: take the "-O" out of $orderfile
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

This will make it easier for a future commit to convert a relative
orderfile pathname to either absolute or relative to the top-level
directory.  It also improves code readability.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 git-mergetool.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..b506896dc 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -421,7 +421,7 @@ main () {
 			prompt=true
 			;;
 		-O*)
-			orderfile="$1"
+			orderfile="${1#-O}"
 			;;
 		--)
 			shift
@@ -465,7 +465,7 @@ main () {
 
 	files=$(git -c core.quotePath=false \
 		diff --name-only --diff-filter=U \
-		${orderfile:+"$orderfile"} -- "$@")
+		${orderfile:+"-O$orderfile"} -- "$@")
 
 	cd_to_toplevel
 
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 11/14] t7610: spell 'git reset --hard' consistently
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 6615 bytes --]

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 54164a320..c031ecd9e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -289,23 +289,23 @@ test_expect_success 'mergetool takes partial path' '
 '
 
 test_expect_success 'mergetool delete/delete conflict' '
-	test_when_finished "git reset --hard HEAD" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count move-to-c &&
 	test_must_fail git merge move-to-b &&
 	echo d | git mergetool a/a/file.txt &&
 	! test -f a/a/file.txt &&
-	git reset --hard HEAD &&
+	git reset --hard &&
 	test_must_fail git merge move-to-b &&
 	echo m | git mergetool a/a/file.txt &&
 	test -f b/b/file.txt &&
-	git reset --hard HEAD &&
+	git reset --hard &&
 	test_must_fail git merge move-to-b &&
 	! echo a | git mergetool a/a/file.txt &&
 	! test -f a/a/file.txt
 '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
-	test_when_finished "git reset --hard HEAD" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count move-to-c &&
 	test_config mergetool.keepBackup true &&
 	test_must_fail git merge move-to-b &&
@@ -316,7 +316,7 @@ test_expect_success 'mergetool produces no errors when keepBackup is used' '
 '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
-	test_when_finished "git reset --hard HEAD" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count move-to-c &&
 	test_config mergetool.keepTemporaries false &&
 	test_must_fail git merge move-to-b &&
@@ -325,7 +325,7 @@ test_expect_success 'mergetool honors tempfile config for deleted files' '
 '
 
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
-	test_when_finished "git reset --hard HEAD" &&
+	test_when_finished "git reset --hard" &&
 	test_when_finished "git clean -fdx" &&
 	git checkout -b test$test_count move-to-c &&
 	test_config mergetool.keepTemporaries true &&
@@ -342,7 +342,7 @@ test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
 '
 
 test_expect_success 'deleted vs modified submodule' '
-	test_when_finished "git reset --hard HEAD" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	mv submod submod-movedaside &&
@@ -560,7 +560,7 @@ test_expect_success 'directory vs modified submodule' '
 	test "$(cat submod/file16)" = "not a submodule" &&
 	rm -rf submod.orig &&
 
-	git reset --hard >/dev/null 2>&1 &&
+	git reset --hard &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
 	test ! -e submod.orig &&
@@ -572,7 +572,8 @@ test_expect_success 'directory vs modified submodule' '
 	( cd submod && git clean -f && git reset --hard ) &&
 	git submodule update -N &&
 	test "$(cat submod/bar)" = "master submodule" &&
-	git reset --hard >/dev/null 2>&1 && rm -rf submod-movedaside &&
+	git reset --hard &&
+	rm -rf submod-movedaside &&
 
 	git checkout -b test$test_count.c master &&
 	git submodule update -N &&
@@ -582,7 +583,7 @@ test_expect_success 'directory vs modified submodule' '
 	git submodule update -N &&
 	test "$(cat submod/bar)" = "master submodule" &&
 
-	git reset --hard >/dev/null 2>&1 &&
+	git reset --hard &&
 	git submodule update -N &&
 	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
@@ -590,13 +591,13 @@ test_expect_success 'directory vs modified submodule' '
 	( yes "r" | git mergetool submod ) &&
 	test "$(cat submod/file16)" = "not a submodule" &&
 
-	git reset --hard master >/dev/null 2>&1 &&
+	git reset --hard master &&
 	( cd submod && git clean -f && git reset --hard ) &&
 	git submodule update -N
 '
 
 test_expect_success 'file with no base' '
-	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool mybase -- both &&
@@ -605,7 +606,7 @@ test_expect_success 'file with no base' '
 '
 
 test_expect_success 'custom commands override built-ins' '
-	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
 	test_config mergetool.defaults.trustExitCode true &&
@@ -616,7 +617,7 @@ test_expect_success 'custom commands override built-ins' '
 '
 
 test_expect_success 'filenames seen by tools start with ./' '
-	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	test_config mergetool.writeToTemp false &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -632,7 +633,7 @@ test_lazy_prereq MKTEMP '
 '
 
 test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
-	test_when_finished "git reset --hard master >/dev/null 2>&1" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	test_config mergetool.writeToTemp true &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -644,7 +645,7 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
 '
 
 test_expect_success 'diff.orderFile configuration is honored' '
-	test_when_finished "git reset --hard >/dev/null" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count order-file-side2 &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -662,7 +663,7 @@ test_expect_success 'diff.orderFile configuration is honored' '
 	test_cmp expect actual
 '
 test_expect_success 'mergetool -Oorder-file is honored' '
-	test_when_finished "git reset --hard >/dev/null 2>&1" &&
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count order-file-side2 &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
@@ -678,7 +679,7 @@ test_expect_success 'mergetool -Oorder-file is honored' '
 	git mergetool -O/dev/null --no-prompt --tool myecho >output &&
 	git grep --no-index -h -A2 Merging: output >actual &&
 	test_cmp expect actual &&
-	git reset --hard >/dev/null 2>&1 &&
+	git reset --hard &&
 
 	git config --unset diff.orderFile &&
 	test_must_fail git merge order-file-side1 &&
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 2721 bytes --]

The pathnames output by the 'git rerere remaining' command are
relative to the top-level directory but the 'git diff --name-only'
command expects its pathname arguments to be relative to the current
working directory.  Run cd_to_toplevel before running 'git diff
--name-only' and adjust any relative pathnames so that 'git mergetool'
does not fail when run from a subdirectory with rerere enabled.

This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Richard Hansen <hansenr@google.com>
---
 git-mergetool.sh     | 16 ++++++++++++++--
 t/t7610-mergetool.sh |  7 ++++++-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index b506896dc..cba6bbd05 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -454,6 +454,15 @@ main () {
 	merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 	merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
 
+	prefix=$(git rev-parse --show-prefix) || exit 1
+	cd_to_toplevel
+
+	if test -n "$orderfile"
+	then
+		orderfile=$(git rev-parse --prefix "$prefix" -- "$orderfile") || exit 1
+		orderfile=$(printf %s\\n "$orderfile" | sed -e 1d)
+	fi
+
 	if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
 	then
 		set -- $(git rerere remaining)
@@ -461,14 +470,17 @@ main () {
 		then
 			print_noop_and_exit
 		fi
+	elif test $# -ge 0
+	then
+		files_quoted=$(git rev-parse --sq --prefix "$prefix" -- "$@") || exit 1
+		eval "set -- $files_quoted"
+		shift
 	fi
 
 	files=$(git -c core.quotePath=false \
 		diff --name-only --diff-filter=U \
 		${orderfile:+"-O$orderfile"} -- "$@")
 
-	cd_to_toplevel
-
 	if test -z "$files"
 	then
 		print_noop_and_exit
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index b36fde1c0..820fc8597 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -234,7 +234,7 @@ test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
 	)
 '
 
-test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+test_expect_success 'mergetool merges all from subdir (rerere enabled)' '
 	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	test_config rerere.enabled true &&
@@ -677,6 +677,11 @@ test_expect_success 'diff.orderFile configuration is honored' '
 		b
 		a
 	EOF
+
+	# make sure "order-file" that is ambiguous between
+	# rev and path is understood correctly.
+	git branch order-file HEAD &&
+
 	git mergetool --no-prompt --tool myecho >output &&
 	git grep --no-index -h -A2 Merging: output >actual &&
 	test_cmp expect actual
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 12/14] t7610: add test case for rerere+mergetool+subdir bug
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

If rerere is enabled and mergetool is run from a subdirectory,
mergetool always prints "No files need merging".  Add an expected
failure test case for this situation.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index c031ecd9e..b36fde1c0 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -216,7 +216,7 @@ test_expect_success 'mergetool skips autoresolved' '
 	test "$output" = "No files need merging"
 '
 
-test_expect_success 'mergetool merges all from subdir' '
+test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
 	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	test_config rerere.enabled false &&
@@ -234,6 +234,25 @@ test_expect_success 'mergetool merges all from subdir' '
 	)
 '
 
+test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count branch1 &&
+	test_config rerere.enabled true &&
+	rm -rf .git/rr-cache &&
+	(
+		cd subdir &&
+		test_must_fail git merge master &&
+		( yes "r" | git mergetool ../submod ) &&
+		( yes "d" "d" | git mergetool --no-prompt ) &&
+		test "$(cat ../file1)" = "master updated" &&
+		test "$(cat ../file2)" = "master new" &&
+		test "$(cat file3)" = "master new sub" &&
+		( cd .. && git submodule update -N ) &&
+		test "$(cat ../submod/bar)" = "master submodule" &&
+		git commit -m "branch2 resolved by mergetool from subdir"
+	)
+'
+
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
 	test_when_finished "git reset --hard" &&
 	test_config rerere.enabled true &&
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 09/14] t7610: always work on a test-specific branch
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 3492 bytes --]

Create and use a test-specific branch when the test might create a
commit.  This is not always necessary for correctness, but it improves
debuggability by ensuring a commit created by test #N shows up on the
testN branch, not the branch for test #N-1.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 7d5e1df88..efcf5c3f1 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -184,6 +184,7 @@ test_expect_success 'mergetool in subdir' '
 
 test_expect_success 'mergetool on file in parent dir' '
 	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count &&
 	git submodule update -N &&
 	(
 		cd subdir &&
@@ -217,6 +218,7 @@ test_expect_success 'mergetool skips autoresolved' '
 
 test_expect_success 'mergetool merges all from subdir' '
 	test_when_finished "git reset --hard" &&
+	git checkout -b test$test_count &&
 	test_config rerere.enabled false &&
 	(
 		cd subdir &&
@@ -288,7 +290,7 @@ test_expect_success 'mergetool takes partial path' '
 
 test_expect_success 'mergetool delete/delete conflict' '
 	test_when_finished "git reset --hard HEAD" &&
-	git checkout move-to-c &&
+	git checkout -b test$test_count move-to-c &&
 	test_must_fail git merge move-to-b &&
 	echo d | git mergetool a/a/file.txt &&
 	! test -f a/a/file.txt &&
@@ -304,6 +306,7 @@ test_expect_success 'mergetool delete/delete conflict' '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
 	test_when_finished "git reset --hard HEAD" &&
+	git checkout -b test$test_count &&
 	test_config mergetool.keepBackup true &&
 	test_must_fail git merge move-to-b &&
 	: >expect &&
@@ -314,6 +317,7 @@ test_expect_success 'mergetool produces no errors when keepBackup is used' '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
 	test_when_finished "git reset --hard HEAD" &&
+	git checkout -b test$test_count &&
 	test_config mergetool.keepTemporaries false &&
 	test_must_fail git merge move-to-b &&
 	echo d | git mergetool a/a/file.txt &&
@@ -323,6 +327,7 @@ test_expect_success 'mergetool honors tempfile config for deleted files' '
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
 	test_when_finished "git reset --hard HEAD" &&
 	test_when_finished "git clean -fdx" &&
+	git checkout -b test$test_count &&
 	test_config mergetool.keepTemporaries true &&
 	test_must_fail git merge move-to-b &&
 	! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -640,7 +645,7 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
 
 test_expect_success 'diff.orderFile configuration is honored' '
 	test_when_finished "git reset --hard >/dev/null" &&
-	git checkout order-file-side2 &&
+	git checkout -b test$test_count order-file-side2 &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
@@ -658,6 +663,7 @@ test_expect_success 'diff.orderFile configuration is honored' '
 '
 test_expect_success 'mergetool -Oorder-file is honored' '
 	test_when_finished "git reset --hard >/dev/null 2>&1" &&
+	git checkout -b test$test_count &&
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 07/14] t7610: run 'git reset --hard' after each test to clean up
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]

Use test_when_finished to run 'git reset --hard' after each test so
that the repository is left in a saner state for the next test.

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 2d92a2646..55587504e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -127,6 +127,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	test_must_fail git merge master >/dev/null 2>&1 &&
@@ -170,6 +171,7 @@ test_expect_success 'mergetool crlf' '
 '
 
 test_expect_success 'mergetool in subdir' '
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	(
@@ -181,6 +183,7 @@ test_expect_success 'mergetool in subdir' '
 '
 
 test_expect_success 'mergetool on file in parent dir' '
+	test_when_finished "git reset --hard" &&
 	git reset --hard &&
 	git submodule update -N &&
 	(
@@ -214,6 +217,7 @@ test_expect_success 'mergetool skips autoresolved' '
 '
 
 test_expect_success 'mergetool merges all from subdir' '
+	test_when_finished "git reset --hard" &&
 	test_config rerere.enabled false &&
 	(
 		cd subdir &&
@@ -244,6 +248,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
 '
 
 test_expect_success 'conflicted stash sets up rerere'  '
+	test_when_finished "git reset --hard" &&
 	test_config rerere.enabled true &&
 	git checkout stash1 &&
 	echo "Conflicting stash content" >file11 &&
@@ -403,6 +408,7 @@ test_expect_success 'deleted vs modified submodule' '
 '
 
 test_expect_success 'file vs modified submodule' '
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	mv submod submod-movedaside &&
@@ -474,6 +480,7 @@ test_expect_success 'file vs modified submodule' '
 '
 
 test_expect_success 'submodule in subdirectory' '
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	(
@@ -535,6 +542,7 @@ test_expect_success 'submodule in subdirectory' '
 '
 
 test_expect_success 'directory vs modified submodule' '
+	test_when_finished "git reset --hard" &&
 	git checkout -b test$test_count branch1 &&
 	mv submod submod-movedaside &&
 	git rm --cached submod &&
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 06/14] t7610: don't rely on state from previous test
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

If the repository must be in a particular state (beyond what is
already done by the 'setup' test case) before the test can run, make
the necessary repository changes in the test script even if it means
duplicating some lines of code from the previous test case.

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index f62ceffdc..2d92a2646 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -181,8 +181,12 @@ test_expect_success 'mergetool in subdir' '
 '
 
 test_expect_success 'mergetool on file in parent dir' '
+	git reset --hard &&
+	git submodule update -N &&
 	(
 		cd subdir &&
+		test_must_fail git merge master >/dev/null 2>&1 &&
+		( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
 		( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
 		( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) &&
 		( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
@@ -651,6 +655,8 @@ test_expect_success 'mergetool -Oorder-file is honored' '
 	test_config diff.orderFile order-file &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
+	echo b >order-file &&
+	echo a >>order-file &&
 	test_must_fail git merge order-file-side1 &&
 	cat >expect <<-\EOF &&
 		Merging:
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* [PATCH v4 03/14] t7610: update branch names to match test number
From: Richard Hansen @ 2017-01-09 23:29 UTC (permalink / raw)
  To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 11801 bytes --]

Rename the testNN branches so that NN matches the test number.  This
should make it easier to troubleshoot test issues.  Use $test_count to
keep this future-proof.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 t/t7610-mergetool.sh | 82 ++++++++++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6d9f21511..14090739f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,7 +94,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
-	git checkout -b test1 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	test_must_fail git merge master >/dev/null 2>&1 &&
 	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -113,7 +113,7 @@ test_expect_success 'custom mergetool' '
 
 test_expect_success 'mergetool crlf' '
 	test_config core.autocrlf true &&
-	git checkout -b test2 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	test_must_fail git merge master >/dev/null 2>&1 &&
 	( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
@@ -134,7 +134,7 @@ test_expect_success 'mergetool crlf' '
 '
 
 test_expect_success 'mergetool in subdir' '
-	git checkout -b test3 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	(
 		cd subdir &&
@@ -161,7 +161,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
-	git checkout -b test4 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
@@ -192,7 +192,7 @@ test_expect_success 'mergetool merges all from subdir' '
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
 	test_config rerere.enabled true &&
 	rm -rf .git/rr-cache &&
-	git checkout -b test5 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	test_must_fail git merge master >/dev/null 2>&1 &&
 	( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
@@ -233,7 +233,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
 test_expect_success 'mergetool takes partial path' '
 	git reset --hard &&
 	test_config rerere.enabled false &&
-	git checkout -b test12 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	test_must_fail git merge master &&
 
@@ -308,12 +308,12 @@ test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
 '
 
 test_expect_success 'deleted vs modified submodule' '
-	git checkout -b test6 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	mv submod submod-movedaside &&
 	git rm --cached submod &&
 	git commit -m "Submodule deleted from branch" &&
-	git checkout -b test6.a test6 &&
+	git checkout -b test$test_count.a test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
 	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -329,7 +329,7 @@ test_expect_success 'deleted vs modified submodule' '
 	git commit -m "Merge resolved by keeping module" &&
 
 	mv submod submod-movedaside &&
-	git checkout -b test6.b test6 &&
+	git checkout -b test$test_count.b test$test_count &&
 	git submodule update -N &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
@@ -343,9 +343,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git commit -m "Merge resolved by deleting module" &&
 
 	mv submod-movedaside submod &&
-	git checkout -b test6.c master &&
+	git checkout -b test$test_count.c master &&
 	git submodule update -N &&
-	test_must_fail git merge test6 &&
+	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
 	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -359,9 +359,9 @@ test_expect_success 'deleted vs modified submodule' '
 	git commit -m "Merge resolved by deleting module" &&
 	mv submod.orig submod &&
 
-	git checkout -b test6.d master &&
+	git checkout -b test$test_count.d master &&
 	git submodule update -N &&
-	test_must_fail git merge test6 &&
+	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
 	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -377,14 +377,14 @@ test_expect_success 'deleted vs modified submodule' '
 '
 
 test_expect_success 'file vs modified submodule' '
-	git checkout -b test7 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	mv submod submod-movedaside &&
 	git rm --cached submod &&
 	echo not a submodule >submod &&
 	git add submod &&
 	git commit -m "Submodule path becomes file" &&
-	git checkout -b test7.a branch1 &&
+	git checkout -b test$test_count.a branch1 &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
 	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -400,7 +400,7 @@ test_expect_success 'file vs modified submodule' '
 	git commit -m "Merge resolved by keeping module" &&
 
 	mv submod submod-movedaside &&
-	git checkout -b test7.b test7 &&
+	git checkout -b test$test_count.b test$test_count &&
 	test_must_fail git merge master &&
 	test -n "$(git ls-files -u)" &&
 	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
@@ -413,11 +413,11 @@ test_expect_success 'file vs modified submodule' '
 	test "$output" = "No files need merging" &&
 	git commit -m "Merge resolved by keeping file" &&
 
-	git checkout -b test7.c master &&
+	git checkout -b test$test_count.c master &&
 	rmdir submod && mv submod-movedaside submod &&
 	test ! -e submod.orig &&
 	git submodule update -N &&
-	test_must_fail git merge test7 &&
+	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
 	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -430,10 +430,10 @@ test_expect_success 'file vs modified submodule' '
 	test "$output" = "No files need merging" &&
 	git commit -m "Merge resolved by keeping file" &&
 
-	git checkout -b test7.d master &&
+	git checkout -b test$test_count.d master &&
 	rmdir submod && mv submod.orig submod &&
 	git submodule update -N &&
-	test_must_fail git merge test7 &&
+	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
 	( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool both>/dev/null 2>&1 ) &&
@@ -448,7 +448,7 @@ test_expect_success 'file vs modified submodule' '
 '
 
 test_expect_success 'submodule in subdirectory' '
-	git checkout -b test10 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	git submodule update -N &&
 	(
 		cd subdir &&
@@ -464,52 +464,52 @@ test_expect_success 'submodule in subdirectory' '
 	git add subdir/subdir_module &&
 	git commit -m "add submodule in subdirectory" &&
 
-	git checkout -b test10.a test10 &&
+	git checkout -b test$test_count.a test$test_count &&
 	git submodule update -N &&
 	(
 	cd subdir/subdir_module &&
 		git checkout -b super10.a &&
-		echo test10.a >file15 &&
+		echo test$test_count.a >file15 &&
 		git add file15 &&
 		git commit -m "on branch 10.a"
 	) &&
 	git add subdir/subdir_module &&
-	git commit -m "change submodule in subdirectory on test10.a" &&
+	git commit -m "change submodule in subdirectory on test$test_count.a" &&
 
-	git checkout -b test10.b test10 &&
+	git checkout -b test$test_count.b test$test_count &&
 	git submodule update -N &&
 	(
 		cd subdir/subdir_module &&
 		git checkout -b super10.b &&
-		echo test10.b >file15 &&
+		echo test$test_count.b >file15 &&
 		git add file15 &&
 		git commit -m "on branch 10.b"
 	) &&
 	git add subdir/subdir_module &&
-	git commit -m "change submodule in subdirectory on test10.b" &&
+	git commit -m "change submodule in subdirectory on test$test_count.b" &&
 
-	test_must_fail git merge test10.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
 	(
 		cd subdir &&
 		( yes "l" | git mergetool subdir_module )
 	) &&
-	test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
 	git submodule update -N &&
-	test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
 	git reset --hard &&
 	git submodule update -N &&
 
-	test_must_fail git merge test10.a >/dev/null 2>&1 &&
+	test_must_fail git merge test$test_count.a >/dev/null 2>&1 &&
 	( yes "r" | git mergetool subdir/subdir_module ) &&
-	test "$(cat subdir/subdir_module/file15)" = "test10.b" &&
+	test "$(cat subdir/subdir_module/file15)" = "test$test_count.b" &&
 	git submodule update -N &&
-	test "$(cat subdir/subdir_module/file15)" = "test10.a" &&
+	test "$(cat subdir/subdir_module/file15)" = "test$test_count.a" &&
 	git commit -m "branch1 resolved with mergetool" &&
 	rm -rf subdir/subdir_module
 '
 
 test_expect_success 'directory vs modified submodule' '
-	git checkout -b test11 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	mv submod submod-movedaside &&
 	git rm --cached submod &&
 	mkdir submod &&
@@ -537,9 +537,9 @@ test_expect_success 'directory vs modified submodule' '
 	test "$(cat submod/bar)" = "master submodule" &&
 	git reset --hard >/dev/null 2>&1 && rm -rf submod-movedaside &&
 
-	git checkout -b test11.c master &&
+	git checkout -b test$test_count.c master &&
 	git submodule update -N &&
-	test_must_fail git merge test11 &&
+	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
 	( yes "l" | git mergetool submod ) &&
 	git submodule update -N &&
@@ -547,7 +547,7 @@ test_expect_success 'directory vs modified submodule' '
 
 	git reset --hard >/dev/null 2>&1 &&
 	git submodule update -N &&
-	test_must_fail git merge test11 &&
+	test_must_fail git merge test$test_count &&
 	test -n "$(git ls-files -u)" &&
 	test ! -e submod.orig &&
 	( yes "r" | git mergetool submod ) &&
@@ -559,7 +559,7 @@ test_expect_success 'directory vs modified submodule' '
 '
 
 test_expect_success 'file with no base' '
-	git checkout -b test13 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	test_must_fail git merge master &&
 	git mergetool --no-prompt --tool mybase -- both &&
 	>expected &&
@@ -568,7 +568,7 @@ test_expect_success 'file with no base' '
 '
 
 test_expect_success 'custom commands override built-ins' '
-	git checkout -b test14 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
 	test_config mergetool.defaults.trustExitCode true &&
 	test_must_fail git merge master &&
@@ -579,7 +579,7 @@ test_expect_success 'custom commands override built-ins' '
 '
 
 test_expect_success 'filenames seen by tools start with ./' '
-	git checkout -b test15 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	test_config mergetool.writeToTemp false &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
@@ -595,7 +595,7 @@ test_lazy_prereq MKTEMP '
 '
 
 test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToTemp' '
-	git checkout -b test16 branch1 &&
+	git checkout -b test$test_count branch1 &&
 	test_config mergetool.writeToTemp true &&
 	test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
 	test_config mergetool.myecho.trustExitCode true &&
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related

* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Junio C Hamano @ 2017-01-09 23:32 UTC (permalink / raw)
  To: Richard Hansen; +Cc: git, davvid, j6t, sbeller, simon
In-Reply-To: <xmqq37gru819.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

> By the way, I didn't think this through, but how is the orderfile
> that comes from the configuration file handled when it is not an
> absolute path?  I think it is _wrong_ to take it as relative to
> where the user started the program.

Answering my own question: the program does not handle configured
orderfile at all, so there is no gotcha ;-)

^ permalink raw reply

* Re: [PATCH v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-09 23:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, davvid, j6t, sbeller, simon
In-Reply-To: <xmqq37gru819.fsf@gitster.mtv.corp.google.com>

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On 2017-01-09 18:29, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> I wonder if it makes more sense to always move to toplevel upfront
>>> and consistently use path from the toplevel, perhaps like the patch
>>
>> s/the patch/the attached patch/ I meant.
>>
>>> does.  The first hunk is what you wrote but only inside MERGE_RR
>>> block, and the second hunk deals with converting end-user supplied
>>> paths that are relative to the original relative to the top-level.
>>>
>>> The tweaking of $orderfile you have in the first hunk may have to be
>>> tightened mimicking the way how "eval ... --sq ... ; shift" is used
>>> in the second hunk to avoid confusion in case orderfile specified by
>>> the end user happens to be the same as a valid revname
>>> (e.g. "master").
>>
>> And here is a squash-able patch to illustrate what I mean.
>
> By the way, I didn't think this through, but how is the orderfile
> that comes from the configuration file handled when it is not an
> absolute path?

Good question; it does whatever 'git diff' does, and that case isn't 
documented.  It's also unclear if the globs are like the .gitignore 
patterns.

I would expect it to act like $GIT_DIR/info/exclude, but I haven't checked.

> I think it is _wrong_ to take it as relative to
> where the user started the program.

Agreed.

> The -O<file> parameter from the
> command line, when <file> is not absolute, should be taken as
> relative to where the user _thinks_ s/he is,

Agreed.

> but when it comes from
> the diff.orderfile configuration and it is not absolute, it should
> be taken as relative to the top of the working tree.

Agreed.

> As we always
> cd_to_top with the suggested SQUASH, it means that the orderfile
> that came from the configuration does not have to be touched, while
> the orderfile given via -O<file> on the command line needs
> prefixing.

Yes.  By unconditionally running cd_to_top we should get the behavior we 
want even if 'git diff' uses the current working directory rather than 
the top-level directory.

I'll poke at 'git diff' to see what it does.

-Richard

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply

* [PATCH 0/2] minor diff orderfile documentation improvements
From: Richard Hansen @ 2017-01-10  0:40 UTC (permalink / raw)
  To: git; +Cc: Richard Hansen

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Richard Hansen (2):
  diff: document behavior of relative diff.orderFile
  diff: document the pattern format for diff.orderFile

 Documentation/diff-config.txt  | 5 ++++-
 Documentation/diff-options.txt | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply

* [PATCH 1/2] diff: document behavior of relative diff.orderFile
From: Richard Hansen @ 2017-01-10  0:40 UTC (permalink / raw)
  To: git; +Cc: Richard Hansen
In-Reply-To: <20170110004031.57985-1-hansenr@google.com>

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

Document that a relative pathname for diff.orderFile is interpreted as
relative to the top-level work directory.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/diff-config.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6af..875212045 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -101,6 +101,8 @@ diff.noprefix::
 diff.orderFile::
 	File indicating how to order files within a diff, using
 	one shell glob pattern per line.
+	If `diff.orderFile` is a relative pathname, it is treated as
+	relative to the top of the work tree.
 	Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox