* Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-10 17:09 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, davvid, sbeller, simon, gitster
In-Reply-To: <3a09e318-f14b-5f14-a992-3bd045f0a4c6@kdbg.org>
[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]
On 2017-01-10 01:17, Johannes Sixt wrote:
> Am 10.01.2017 um 00:29 schrieb Richard Hansen:
>> 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)
>
> Is the purpose of this complication only to detect errors of the git
> invocation?
Yes. I've been burned so many times by the shell not stopping when
there's an error that I always like to do things one step at a time with
error checking, even if it is less efficient.
> IMHO, we could dispense with that, but others might
> disagree. I am arguing because this adds yet another process; but it is
> only paid when -O is used, so...
>
>> + 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"
>
> BTW, the --sq and eval business is not required here. At this point,
> $IFS = $'\n',
Whoa, really? That's surprising and feels wrong.
(BTW, I just noticed that guess_merge_tool sets IFS to a space without
resetting it afterward. That function is always run in a subshell so
there's no problem at the moment, but it's still a bit risky.)
> so
>
> set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")
>
> will do. (Except that it would not detect errors.)
I think I would prefer to leave it as-is in case IFS is ever changed
back to the default.
>
>> + shift
>> fi
>
> As I don't see anything wrong with what you have written, these comments
> alone do not warrant another re-roll.
I'll reroll if I hear other votes in favor of changing. Thanks for
reviewing!
-Richard
>
> -- Hannes
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile
From: Richard Hansen @ 2017-01-10 17:27 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170110065816.pu325sxajbyuqpj6@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]
On 2017-01-10 01:58, Jeff King wrote:
> On Mon, Jan 09, 2017 at 07:40:30PM -0500, Richard Hansen wrote:
>
>> 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].
>
> What happens in a bare repository?
I didn't know which is why this patch is silent on that topic. :)
>
> I'm guessing it's relative to the top-level of the repository,
I just tried it out and it's relative to $PWD.
> but we should probably spell it out.
Do we want it to be relative to $PWD? I think relative to $GIT_DIR is
more useful. If we want it to be relative to $GIT_DIR, then I think we
should stay silent regarding bare repositories so that the behavior
remains unspecified until we update the logic.
>
> The other case is --no-index when we are not in a repository at all, but
> that should just be relative to the current directory,
It is.
> which isn't really worth mentioning.
Agreed.
I'll post a re-roll if people prefer the current behavior over relative
to $GIT_DIR.
Thanks for reviewing,
Richard
>
> -Peff
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply
* Re: [PATCHv8] pathspec: give better message for submodule related pathspec error
From: Junio C Hamano @ 2017-01-10 18:13 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, peff
In-Reply-To: <20170109231650.9043-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> 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
Thanks, queued.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Localise error headers
From: Stefan Beller @ 2017-01-10 18:28 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git@vger.kernel.org
In-Reply-To: <20170110090418.4egk4oflblshmjon@sigill.intra.peff.net>
On Tue, Jan 10, 2017 at 1:04 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Jan 09, 2017 at 01:43:15PM +0100, Michael J Gruber wrote:
>
>> > I can't say I'm excited about having matching "_" variants for each
>> > function. Are we sure that they are necessary? I.e., would it be
>> > acceptable to just translate them always?
>>
>> We would still need to mark the strings, e.g.
>>
>> die(N_("oopsie"));
>>
>> and would not be able to opt out of translating in the code (only in the
>> po file, by not providing a translation).
>
> I meant more along the lines of: would it be OK to just always translate
> the prefix, even if the message itself is not translated? I.e.,
>
> diff --git a/usage.c b/usage.c
> index 82ff13163..8e5400f57 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -32,7 +32,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
>
> static NORETURN void die_builtin(const char *err, va_list params)
> {
> - vreportf("fatal: ", err, params);
> + vreportf(_("fatal: "), err, params);
> exit(128);
> }
>
>> In any case, the question is whether we want to tell the user
>>
>> A: B
>>
>> where A is in English and B is localised, or rather localise both A and
>> B (for A in "error", "fatal", "warning"...).
>>
>> For localising A and B, we'd need this series or something similar. For
>> keeping the mix, we don't need to do anything ;)
>
> What I wrote above would keep the mix, but switch it in the other
> direction.
>
> And then presumably that mix would gradually move to 100% consistency as
> more messages are translated. But the implicit question is: are there
> die() messages that should never be translated? I'm not sure.
I would assume any plumbing command is not localizing?
Because in plumbing land, (easily scriptable) you may find
a grep on the output/stderr for a certain condition?
To find a good example, "git grep die" giving me some food of though:
die_errno(..) should always take a string marked up for translation,
because the errno string is translated?
(-> we'd have to fix up any occurrence of git grep "die_errno(\"")
apply.c: die(_("internal error"));
That is funny, too. I think we should substitute that with
die("BUG: untranslated, but what went wrong instead")
>
> -Peff
^ permalink raw reply
* git-mergetool to be used for rebases as well?
From: Stefan Beller @ 2017-01-10 18:54 UTC (permalink / raw)
To: git; +Cc: Stefan Beller
An internal user report running on origin/next:
$ git pull
From .
* branch ... -> FETCH_HEAD
First, rewinding head to replay your work on top of it...
Applying: ...
Applying: ...
Using index info to reconstruct a base tree...
CONFLICT (content): ....
error: Failed to merge in the changes.
Patch failed at 0002...
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
$ git status
rebase in progress; onto ...
You are currently rebasing branch '...' on '...'.
Changes to be committed:
modified: ...
Unmerged paths:
both modified: ...
$ git mergetool
No files need merging
$ git diff <file name>
diff --cc <file name>
index ...
--- a/file
+++ b/file
@@@ ...
content
++<<<<<<< HEAD
+ content
++=======
+ content
++>>>>>>> other commit
content
The mergetool used to work apparently, but stopped for rebases.
I noticed in neither t7610-mergetool.sh nor any rebase test the combination of
rebase and mergetool is tested.
Stefan
^ permalink raw reply
* Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled
From: Junio C Hamano @ 2017-01-10 19:25 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Richard Hansen, git, davvid, sbeller, simon
In-Reply-To: <3a09e318-f14b-5f14-a992-3bd045f0a4c6@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> + 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)
>
> Is the purpose of this complication only to detect errors of the git
> invocation? IMHO, we could dispense with that, but others might
> disagree. I am arguing because this adds yet another process; but it
> is only paid when -O is used, so...
I do not terribly mind an added process, but this change makes it
harder to read and also forces the readers to wonder if the quoting
around printf is correct.
>> @@ -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"
>
> BTW, the --sq and eval business is not required here. At this point,
> $IFS = $'\n', so
>
> set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")
>
> will do. (Except that it would not detect errors.)
I thought you are suggesting not to use --sq but it is still there.
I think the original is written in such a way that any letter in
each of "$@" is preserved, even an LF in the filename.
Such a pathname probably won't correctly be given from the "rerere
remaining" side (i.e. when you are lazy and run mergetool without
pathname), so it may appear not to matter, but being able to give an
explicit pathname from the command line is a workaround for that, so
in that sense, it has value to prepare this side of the codepath to
be able to cope with such a pathname.
Unrelated, but I notice that in this:
eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
shift
"set" will get one "--" from its direct command line argument,
followed by "--" that comes out of rev-parse, and then the first
pathname (i.e. "$prefix/$1", if "$1" is relative). Then the first
"--" is discarded and the second "--" that came out of rev-parse
becomes "$1", and the first pathname becomes "$2", etc. We use
"shift" to get rid of "--" and move everything down by one.
It is my fault but it is a roundabout way to say:
eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
I would think, and we should probably want to write this like so.
[PATCH v4 02/14] would probably want to be updated as well.
I can locally update them if everybody agrees it is a good idea.
^ permalink raw reply
* Re: [PATCHv2 2/5] unpack-trees: remove unneeded continue
From: Junio C Hamano @ 2017-01-10 19:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, l.s.r, git
In-Reply-To: <20170109194621.17013-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> The continue is the last statement in the loop, so not needed.
> This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
> read-tree -u remove index entries outside sparse area) when statements
> after the continue were removed.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
Thanks for digging the history to find the culprit.
This is an unrelated tangent, but I do not think of a way other than
"log -L" to find it; conceptually, "blame" ought to be usable for
it, but it is not useful for finding deleted lines.
> unpack-trees.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8e6768f283..d443decb23 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)
> display_progress(progress, ++cnt);
> if (o->update && !o->dry_run)
> unlink_entry(ce);
> - continue;
> }
> }
> remove_marked_cache_entries(index);
^ permalink raw reply
* Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates
From: Junio C Hamano @ 2017-01-10 20:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: peff, l.s.r, git
In-Reply-To: <20170109194621.17013-5-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> This makes check_updates shorter and easier to understand.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
I agree that 3/5 made it easier to understand by ejecting a block
that is not essential to the functionality of the function out of
it, making the remainder of the fuction about "removing gone files
and then write out the modified files".
The ejecting of the first half of these two operations, both are
what this function is about, done by this step feels backwards. If
anything, the "only do the actual working tree manipulation when not
doing a dry-run and told to update" logic that must be in both are
spread in two helper functions after step 5/5, and with the added
boilerplate for these two helpers, the end result becomes _longer_
to understand what is really going on when check_updates() is
called.
Is the original after step 3/5 too long and hard to understand?
> unpack-trees.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b564024472..ac59510251 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -218,6 +218,26 @@ static void unlink_entry(const struct cache_entry *ce)
> schedule_dir_for_removal(ce->name, ce_namelen(ce));
> }
>
> +static unsigned remove_workingtree_files(struct unpack_trees_options *o,
> + struct progress *progress)
> +{
> + int i;
> + unsigned cnt = 0;
> + struct index_state *index = &o->result;
> +
> + for (i = 0; i < index->cache_nr; i++) {
> + const struct cache_entry *ce = index->cache[i];
> +
> + if (ce->ce_flags & CE_WT_REMOVE) {
> + display_progress(progress, ++cnt);
> + if (o->update && !o->dry_run)
> + unlink_entry(ce);
> + }
> + }
> +
> + return cnt;
> +}
> +
> static struct progress *get_progress(struct unpack_trees_options *o)
> {
> unsigned cnt = 0, total = 0;
> @@ -254,15 +274,8 @@ static int check_updates(struct unpack_trees_options *o)
>
> if (o->update)
> git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
> - for (i = 0; i < index->cache_nr; i++) {
> - const struct cache_entry *ce = index->cache[i];
>
> - if (ce->ce_flags & CE_WT_REMOVE) {
> - display_progress(progress, ++cnt);
> - if (o->update && !o->dry_run)
> - unlink_entry(ce);
> - }
> - }
> + cnt = remove_workingtree_files(o, progress);
> remove_marked_cache_entries(index);
> remove_scheduled_dirs();
^ permalink raw reply
* [PATCH] index: improve constness for reading blob data
From: Brandon Williams @ 2017-01-10 20:06 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
Improve constness of the index_state parameter to the
'read_blob_data_from_index' function.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
cache.h | 2 +-
read-cache.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index a50a61a19..363953c1a 100644
--- a/cache.h
+++ b/cache.h
@@ -599,7 +599,7 @@ extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char
extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);
extern int index_name_is_other(const struct index_state *, const char *, int);
-extern void *read_blob_data_from_index(struct index_state *, const char *, unsigned long *);
+extern void *read_blob_data_from_index(const struct index_state *, const char *, unsigned long *);
/* do stat comparison even if CE_VALID is true */
#define CE_MATCH_IGNORE_VALID 01
diff --git a/read-cache.c b/read-cache.c
index f92a912dc..6ee044442 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2292,7 +2292,8 @@ int index_name_is_other(const struct index_state *istate, const char *name,
return 1;
}
-void *read_blob_data_from_index(struct index_state *istate, const char *path, unsigned long *size)
+void *read_blob_data_from_index(const struct index_state *istate,
+ const char *path, unsigned long *size)
{
int pos, len;
unsigned long sz;
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply related
* Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile
From: Junio C Hamano @ 2017-01-10 20:14 UTC (permalink / raw)
To: Richard Hansen; +Cc: git
In-Reply-To: <20170110004031.57985-3-hansenr@google.com>
Richard Hansen <hansenr@google.com> writes:
> Document the format of the patterns used for the diff.orderFile
> setting and diff's '-O' option by referring the reader to the
> gitignore[5] page.
>
> Signed-off-by: Richard Hansen <hansenr@google.com>
> ---
> Documentation/diff-config.txt | 3 ++-
> Documentation/diff-options.txt | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 875212045..a35ecdd6b 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -100,7 +100,8 @@ diff.noprefix::
>
> diff.orderFile::
> File indicating how to order files within a diff, using
> - one shell glob pattern per line.
> + one glob pattern per line.
> + See linkgit:gitignore[5] for the pattern format.
I do not think it is wise to suggest referring to gitignore, as the
logic of matching is quite different, other than the fact that they
both use wildmatch() internally. Also, unlike gitignore, orderfile
does not allow any negative matching i.e. "!<pattern>".
> 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 --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c372..dc6b1af71 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -467,7 +467,8 @@ endif::git-format-patch[]
>
> -O<orderfile>::
> Output the patch in the order specified in the
> - <orderfile>, which has one shell glob pattern per line.
> + <orderfile>, which has one glob pattern per line.
> + See linkgit:gitignore[5] for the pattern format.
> This overrides the `diff.orderFile` configuration variable
> (see linkgit:git-config[1]). To cancel `diff.orderFile`,
> use `-O/dev/null`.
^ permalink raw reply
* Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile
From: Junio C Hamano @ 2017-01-10 20:19 UTC (permalink / raw)
To: Richard Hansen; +Cc: Jeff King, git
In-Reply-To: <e100d30a-5ee8-8485-5012-f9b1c6961ffa@google.com>
Richard Hansen <hansenr@google.com> writes:
> On 2017-01-10 01:58, Jeff King wrote:
> ...
>> What happens in a bare repository?
>>
>> I'm guessing it's relative to the top-level of the repository,
>
> I just tried it out and it's relative to $PWD.
That is understandable. When the user says
$ cmd -O $file
with any option -O that takes a filename, it is most natural if we
used $PWD/$file when $file is not absolute path.
^ permalink raw reply
* Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile
From: Junio C Hamano @ 2017-01-10 20:23 UTC (permalink / raw)
To: Richard Hansen; +Cc: Jeff King, git
In-Reply-To: <xmqq4m16sm5v.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Richard Hansen <hansenr@google.com> writes:
>
>> On 2017-01-10 01:58, Jeff King wrote:
>> ...
>>> What happens in a bare repository?
>>>
>>> I'm guessing it's relative to the top-level of the repository,
>>
>> I just tried it out and it's relative to $PWD.
>
> That is understandable. When the user says
>
> $ cmd -O $file
>
> with any option -O that takes a filename, it is most natural if we
> used $PWD/$file when $file is not absolute path.
Ahh, ignore me in the above. The discussion is about the
configuration variable, and I agree that being relative to GIT_DIR
would have made more sense. In fact taking it as relative to PWD
does not make any sense.
We should have been a lot more careful when we added 6d8940b562
("diff: add diff.orderfile configuration variable", 2013-12-18), but
it is too late to complain now.
A related tangent.
I wonder if anything that uses git_config_pathname() should be
relative to GIT_DIR when it is not absolute.
^ permalink raw reply
* Re: git-mergetool to be used for rebases as well?
From: Junio C Hamano @ 2017-01-10 20:26 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170110185421.2638-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> $ git status
> rebase in progress; onto ...
> You are currently rebasing branch '...' on '...'.
>
> Changes to be committed:
> modified: ...
>
> Unmerged paths:
> both modified: ...
>
> $ git mergetool
> No files need merging
> $ git diff <file name>
> diff --cc <file name>
> index ...
> --- a/file
> +++ b/file
> @@@ ...
> content
> ++<<<<<<< HEAD
> + content
> ++=======
> + content
> ++>>>>>>> other commit
> content
>
>
> The mergetool used to work apparently, but stopped for rebases.
> I noticed in neither t7610-mergetool.sh nor any rebase test the combination of
> rebase and mergetool is tested.
The above redacts too much to be useful for guessing, but are you by
any chance being hit by a recent regression, i.e. have rerere enabled
and running mergetool from a subdirectory?
See <20170109232941.43637-15-hansenr@google.com> (and previous
rounds).
^ permalink raw reply
* Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled
From: Johannes Sixt @ 2017-01-10 20:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Richard Hansen, git, davvid, sbeller, simon
In-Reply-To: <xmqqmveyson3.fsf@gitster.mtv.corp.google.com>
Am 10.01.2017 um 20:25 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>> BTW, the --sq and eval business is not required here. At this point,
>> $IFS = $'\n', so
>>
>> set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")
>>
>> will do. (Except that it would not detect errors.)
>
> I thought you are suggesting not to use --sq but it is still there.
A copy-paste-p. Of course, I want to suggest not to use --sq.
> Unrelated, but I notice that in this:
>
> eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> shift
>
> It is my fault but it is a roundabout way to say:
>
> eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
Clever! But I fear that half a year down the road we will appreciate a
comment like
# rev-parse provides the -- needed for `set`
and then we are back at two lines, so I dunno...
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/4] t1000: modernize style
From: Junio C Hamano @ 2017-01-10 20:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, novalis, git
In-Reply-To: <20170110014542.19352-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> The preferred style in tests seems to be
s/seems to be/is/;
>
> test_expect_success 'short description, ended by 2 single quotes' '
> here comes the test &&
> and chains over many lines &&
> ended by a quote
> '
Thanks. This is way overdue. During the time the script has been
dormant for more than two years, we should have done this.
^ permalink raw reply
* Re: [PATCH 1/4] read-tree: use OPT_BOOL instead of OPT_SET_INT
From: Junio C Hamano @ 2017-01-10 20:41 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, novalis, git
In-Reply-To: <20170110014542.19352-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> All occurrences of OPT_SET_INT were setting the value to 1;
> internally OPT_BOOL is just that.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> builtin/read-tree.c | 36 ++++++++++++++++++------------------
> 1 file changed, 18 insertions(+), 18 deletions(-)
The result is much easier to read, partly because the "1" (true) is
at the very end of the line when OPT_SET_INT() is used for the
reader to notice that this is merely a boolean.
More importantly, as OPT_SET_INT() can be used multiple times on the
same variable (or field) to set it to different values, it is easier
to read if OPT_BOOL() is used when OPT_SET_INT() to 1 is not used
for that purpose.
Thanks.
>> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index fa6edb35b2..8ba64bc785 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -109,34 +109,34 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
> { OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
> N_("write resulting index to <file>"),
> PARSE_OPT_NONEG, index_output_cb },
> - OPT_SET_INT(0, "empty", &read_empty,
> - N_("only empty the index"), 1),
> + OPT_BOOL(0, "empty", &read_empty,
> + N_("only empty the index")),
> OPT__VERBOSE(&opts.verbose_update, N_("be verbose")),
> OPT_GROUP(N_("Merging")),
> - OPT_SET_INT('m', NULL, &opts.merge,
> - N_("perform a merge in addition to a read"), 1),
> - OPT_SET_INT(0, "trivial", &opts.trivial_merges_only,
> - N_("3-way merge if no file level merging required"), 1),
> - OPT_SET_INT(0, "aggressive", &opts.aggressive,
> - N_("3-way merge in presence of adds and removes"), 1),
> - OPT_SET_INT(0, "reset", &opts.reset,
> - N_("same as -m, but discard unmerged entries"), 1),
> + OPT_BOOL('m', NULL, &opts.merge,
> + N_("perform a merge in addition to a read")),
> + OPT_BOOL(0, "trivial", &opts.trivial_merges_only,
> + N_("3-way merge if no file level merging required")),
> + OPT_BOOL(0, "aggressive", &opts.aggressive,
> + N_("3-way merge in presence of adds and removes")),
> + OPT_BOOL(0, "reset", &opts.reset,
> + N_("same as -m, but discard unmerged entries")),
> { OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
> N_("read the tree into the index under <subdirectory>/"),
> PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
> - OPT_SET_INT('u', NULL, &opts.update,
> - N_("update working tree with merge result"), 1),
> + OPT_BOOL('u', NULL, &opts.update,
> + N_("update working tree with merge result")),
> { OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
> N_("gitignore"),
> N_("allow explicitly ignored files to be overwritten"),
> PARSE_OPT_NONEG, exclude_per_directory_cb },
> - OPT_SET_INT('i', NULL, &opts.index_only,
> - N_("don't check the working tree after merging"), 1),
> + OPT_BOOL('i', NULL, &opts.index_only,
> + N_("don't check the working tree after merging")),
> OPT__DRY_RUN(&opts.dry_run, N_("don't update the index or the work tree")),
> - OPT_SET_INT(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
> - N_("skip applying sparse checkout filter"), 1),
> - OPT_SET_INT(0, "debug-unpack", &opts.debug_unpack,
> - N_("debug unpack-trees"), 1),
> + OPT_BOOL(0, "no-sparse-checkout", &opts.skip_sparse_checkout,
> + N_("skip applying sparse checkout filter")),
> + OPT_BOOL(0, "debug-unpack", &opts.debug_unpack,
> + N_("debug unpack-trees")),
> OPT_END()
> };
^ permalink raw reply
* [PATCH v5 01/14] .mailmap: Use my personal email address as my canonical
From: Richard Hansen @ 2017-01-10 20:41 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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
^ permalink raw reply related
* [PATCH v5 00/14] fix mergetool+rerere+subdir regression
From: Richard Hansen @ 2017-01-10 20:41 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170109232941.43637-1-hansenr@google.com>
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 v4:
* Revert to Junio's original proposal for 14/14. (Junio: Please
feel free to take author credit on 14/4; it's now 100% your code.)
* Do:
# rev-parse provides the -- needed for 'set'
eval "set $(git rev-parse --sq -- "$@")"
instead of:
eval "set -- $(git rev-parse --sq -- "$@")"
shift
for both 02/14 and 14/14.
Richard Hansen (14):
.mailmap: Use my personal email address as my canonical
rev-parse doc: pass "--" to rev-parse 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 | 21 ++-
t/t7610-mergetool.sh | 281 ++++++++++++++++++++++++----------------
4 files changed, 187 insertions(+), 120 deletions(-)
--
2.11.0.390.gc69c2f50cf-goog
^ permalink raw reply
* [PATCH v5 09/14] t7610: always work on a test-specific branch
From: Richard Hansen @ 2017-01-10 20:41 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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
^ permalink raw reply related
* [PATCH v5 11/14] t7610: spell 'git reset --hard' consistently
From: Richard Hansen @ 2017-01-10 20:41 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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
^ permalink raw reply related
* [PATCH v5 10/14] t7610: don't assume the checked-out commit
From: Richard Hansen @ 2017-01-10 20:41 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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
^ permalink raw reply related
* [PATCH v5 12/14] t7610: add test case for rerere+mergetool+subdir bug
From: Richard Hansen @ 2017-01-10 20:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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
^ permalink raw reply related
* [PATCH v5 13/14] mergetool: take the "-O" out of $orderfile
From: Richard Hansen @ 2017-01-10 20:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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
^ permalink raw reply related
* [PATCH v5 14/14] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-10 20:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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 | 17 +++++++++++++++--
t/t7610-mergetool.sh | 7 ++++++-
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index b506896dc..c062e3de3 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -454,6 +454,17 @@ 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" |
+ sed -e 1d
+ )
+ fi
+
if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
set -- $(git rerere remaining)
@@ -461,14 +472,16 @@ main () {
then
print_noop_and_exit
fi
+ elif test $# -ge 0
+ then
+ # rev-parse provides the -- needed for 'set'
+ eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
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
^ permalink raw reply related
* [PATCH v5 08/14] t7610: delete some now-unnecessary 'git reset --hard' lines
From: Richard Hansen @ 2017-01-10 20:41 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon, gitster
In-Reply-To: <20170110204202.21779-1-hansenr@google.com>
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
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox