* Re: [PATCH v4 4/4] t7800: run both builtin and scripted difftool, for now
From: Junio C Hamano @ 2017-01-09 9:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <alpine.DEB.2.20.1701090850340.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> And the most important point is that we do all of this only during a
> hopefully brief period in time that is mostly spent on reviewing the code
> and finding serious bugs and fixing them.
You seem to misunderstand the purpose of the code review.
We indeed spot bugs while reviewing patches, especially ones from
less experienced folks, but that is the least important part of the
review. In general, we review for:
- Design. Is the feature make sense? Is it too narrow? Are there
better ways? Does it fit well with the rest of the system?
- Explanation. Is the purpose of the change in the bigger picture
explained well enough to allow future people to answer this
question: "We now have an additional requirement to the feature.
If the original author knew about that when this was first
introduced, would s/he consider that our design for this
additional thing consistent with the original design? Should we
design our enhancement in a different way?"
- Maintainability. Does the implementation avoid reinventing data
structures and helper functions that already exist to interact
with elements in the system? Would a future change to some
elements in the system that are touched by the implementation
require changes to both existing code _and_ reinvented ones the
patch introduced?
- Correctness. Does the implementation actually reflect the design
and the way the design was explained?
For the "difftool in C" topic, the first two are mostly irrelevant
as the goal of the topic is to first replicate what already exists
faithfully (even in a bug-to-bug compatible way). The issues in
correctness is something your daily use before submission would
have caught, use of 'next' users as testers will help, and also
caught by running test suite (again, before submission).
I honestly do not expect glaring errors in the code from experienced
contributors remaining when their patches are polished enough to be
submit to the list.
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Junio C Hamano @ 2017-01-09 9:28 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Segev Finer
In-Reply-To: <alpine.DEB.2.20.1701090825510.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> If you feel strongly about your contrived examples possibly being
> affected by this patch, we could easily make this conditional on
>
> 1) no '&&' or '||' being found on the command-line, and
> 2) argv[0] not containing an '='
>
> Another approach would be to verify that argv[i] starts with '-' for
> non-zero i.
>
> But do we really need to do that?
No. An explicit way to override an incorrect guess is sufficient
and necessary. The above two-item list you gave will be just part
of the machinery to make an incorrect guess. The auto-detection in
the posted patch should cover many users' use case and I do not
think it needs to be extended further to make it more brittle, as by
definition its guess cannot be perfect. Just keep it simple and
give a separate escape hatch.
> That means that the user has to specify something like
>
> HAHAHA_IT_IS_NOT=/plink.exe ssh
>
> as GIT_SSH_COMMAND.
My second message was to clarify that "VAR1=VAL2 command" is NOT a
contrived example, and this response indicates that I somehow failed
to convey that to you. The "if tortoiseplink exists (and the end
user can override the location with an environment), use that, and
if PuTTY plink exists (ditto), use that instead" in a "myssh"
script, and use it as core.sshcommand with the environment to
override my custom installation location to these two programs,
would be what I would do when I get two Windows machines, with these
variants of SSH on each. So take the second message as a bug report
against the version of Git for Windows you ship with the patch in
question.
The auto-detection may work for many people and that is a great
thing. I failed to say that in my message, as I thought that was
obvious. But it is important to plan to cope with the case where it
does not work. The usual practice around here is to say "the it may
not necessarily work for everybody, so lets be prepared to add an
explicit override if it turns out to be necessary".
The second message, which you are responding to, was meant to be a
bug report from the future, telling us that an override is needed,
showing that we do not have to wait for a bug report to act on.
^ permalink raw reply
* Re: [PATCH v4 2/2] t9813: avoid using pipes
From: Luke Diamand @ 2017-01-09 9:11 UTC (permalink / raw)
To: Pranit Bauva; +Cc: Git Users
In-Reply-To: <010201597f0179fb-fc4c0240-5ec7-466b-96b9-59f4840954d7-000000@eu-west-1.amazonses.com>
On 8 January 2017 at 16:55, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it. By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.
Looks good to me, thanks!
Ack.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> t/t9813-git-p4-preserve-users.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
> index 76004a5..bda222a 100755
> --- a/t/t9813-git-p4-preserve-users.sh
> +++ b/t/t9813-git-p4-preserve-users.sh
> @@ -118,12 +118,12 @@ test_expect_success 'not preserving user with mixed authorship' '
> make_change_by_user usernamefile3 Derek derek@example.com &&
> P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
> export P4EDITOR P4USER P4PASSWD &&
> - git p4 commit |\
> - grep "git author derek@example.com does not match" &&
> + git p4 commit >actual &&
> + grep "git author derek@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 Charlie charlie@example.com &&
> - git p4 commit |\
> - grep "git author charlie@example.com does not match" &&
> + git p4 commit >actual &&
> + grep "git author charlie@example.com does not match" actual &&
>
> make_change_by_user usernamefile3 alice alice@example.com &&
> git p4 commit >actual &&
>
> --
> https://github.com/git/git/pull/314
^ permalink raw reply
* [PATCH 2/2] completion: add bash completion for 'filter-branch'
From: Denton Liu @ 2017-01-09 8:56 UTC (permalink / raw)
To: git; +Cc: spearce
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 51832108e..b4cbea5c7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1297,6 +1297,23 @@ _git_fetch ()
__git_complete_remote_or_refspec
}
+__git_filter_branch_options="
+ --env-filter --tree-filter --index-filter --parent-filter --msg-filter
+ --commit-filter --tag-name-filter --subdirectory-filter --prune-empty
+ --original --force
+"
+_git_filter_branch ()
+{
+ __git_has_doubledash && __git_complete_rev_list_command && return
+
+ case "$cur" in
+ --*)
+ __gitcomp "$__git_filter_branch_options"
+ return
+ ;;
+ esac
+}
+
__git_format_patch_options="
--stdout --attach --no-attach --thread --thread= --no-thread
--numbered --start-number --numbered-files --keep-subject --signoff
--
2.11.0
^ permalink raw reply related
* [PATCH 1/2] completion: add bash completion for 'git rev-list'
From: Denton Liu @ 2017-01-09 8:56 UTC (permalink / raw)
To: git; +Cc: spearce
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
contrib/completion/git-completion.bash | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 21016bf8d..51832108e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2430,6 +2430,36 @@ _git_revert ()
__gitcomp_nl "$(__git_refs)"
}
+__git_rev_list_options="
+ --max-count= --skip= --max-age= --min-age= --sparse --merges --no-merges
+ --min-parents= --no-min-parents --max-parents= --no-max-parents --first-parent
+ --remove-empty --full-history --not --all --branches= --tags= --remotes=
+ --glob= --ignore-missing --stdin --quiet --topo-order --parents --timestamp
+ --left-right --left-only --right-only --cherry-mark --cherry-pick --encoding=
+ --author= --committer= --grep= --regexp-ignore-case --extended-regexp
+ --fixed-strings --date= --objects --objects-edge --objects-edge-aggressive
+ --unpacked --pretty --header --bisect --bisect-vars --bisect-all --merge
+ --reverse --walk-reflogs --no-walk --do-walk --count --use-bitmap-index
+"
+
+__git_complete_rev_list_command ()
+{
+ case "$cur" in
+ --*)
+ __gitcomp "$__git_rev_list_options"
+ return 0
+ ;;
+ esac
+ return 1
+}
+
+_git_rev_list ()
+{
+ __git_has_doubledash && return
+
+ __git_complete_rev_list_command || __gitcomp_nl "$(__git_refs)"
+}
+
_git_rm ()
{
case "$cur" in
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] Makefile: POSIX windres
From: Johannes Schindelin @ 2017-01-09 8:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Steven Penny, Johannes Sixt, git
In-Reply-To: <xmqqtw99x70u.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Sun, 8 Jan 2017, Junio C Hamano wrote:
> Steven Penny <svnpenn@gmail.com> writes:
>
> > When environment variable POSIXLY_CORRECT is set, the "input -o
> > output" syntax is not supported.
> >
> > http://cygwin.com/ml/cygwin/2017-01/msg00036.html
> >
> > Signed-off-by: Steven Penny <svnpenn@gmail.com>
> > ---
>
> Who other than cygwin build uses this target? Git for Windows?
Yes, Git for Windows uses this target, as did msysGit (and I suspect
Hannes' setup).
The resources are built correctly in Git for Windows SDK with this patch,
and I just verified that the windres shipped with the last msysGit (AKA
Git for Windows 1.x' SDK) handles the -i flag correctly, too. That is, at
least windres.exe included in binutils-2.19.1-mingw32-bin.tar.gz (which
was current at the time I updated msysGit on Feb 19 2009) can handle it.
So: ACK
Ciao,
Dscho
P.S.: I applied this patch to Git for Windows' `master`:
https://github.com/git-for-windows/git/commit/744120c602
^ permalink raw reply
* Re: [PATCH v4 4/4] t7800: run both builtin and scripted difftool, for now
From: Johannes Schindelin @ 2017-01-09 7:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqtw99ypvq.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Sun, 8 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > This is uglier than a simple
> >
> > touch "$GIT_EXEC_PATH/use-builtin-difftool"
> >
> > of course. But oh well.
>
> That is totally irrelevant.
>
> The more important point is that we do the same set of tests so
> instead of running just one, we run BOTH.
And the most important point is that we do all of this only during a
hopefully brief period in time that is mostly spent on reviewing the code
and finding serious bugs and fixing them.
During that period (which I would expect to be spent completely inside
`pu`), the cross-validation does not have to be beautiful, but correct.
And even more importantly: it has to come at a minimal integration cost,
in case David decides to add another test case to t7800.
And after that period, we retire the Perl script and switch to the builtin
difftool and simply drop this patch to cross-validate both difftools'
outputs with one another.
At which point all of this safe-guarding and indenting and all of those
changes that are just meant to cross-validate the output during that
hopefully brief period of time will become moot and all the work spent on
those changes will be worthless.
Oh, at that point also all review that went into *this* patch will have
been spent in vain.
In short: can we please move on to reviewing the *actual* builtin difftool
and spend our effort on making sure that it is correct?
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Johannes Schindelin @ 2017-01-09 7:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, git@vger.kernel.org, David Aguilar,
Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqy3ylyqhf.fsf@gitster.mtv.corp.google.com>
Hi,
On Sun, 8 Jan 2017, Junio C Hamano wrote:
> How about explaining it like this then?
>
> (only the log message has been corrected; diff is from the original).
>
> commit c9bb5d101ca657fa466afa8c4368c43ea7b7aca8
> Author: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Mon Jan 2 17:22:33 2017 +0100
>
> git_exec_path: avoid Coverity warning about unfree()d result
>
> Technically, it is correct that git_exec_path() returns a possibly
> malloc()ed string returned from system_path(), and it is sometimes
> not allocated. Cache the result in a static variable and make sure
> that we call system_path() only once, which plugs a potential leak.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sounds good to me.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
From: Johannes Schindelin @ 2017-01-09 7:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Segev Finer
In-Reply-To: <xmqq4m1911mf.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Sun, 8 Jan 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I suspect that this will break when GIT_SSH_COMMAND, which is meant
> > to be processed by the shell, hence the user can write anything,
> > begins with a one-shot environment variable assignment, e.g.
> >
> > [core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink
> >
> > One possible unintended side effect of this patch is when VAL1 ends
> > with /plink (or /tortoiseplink) and the command is not either of
> > these, in which case the "tortoiseplink" and "putty" variables will
> > tweak the command line for an SSH implementation that does not want
> > such a tweak to be made. There may be other unintended fallouts.
>
> Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND)
> interface takes any shell-interpretable commands, the first "token"
> you see is not limited to a one-shot environment assignment. It
> could even be "cmd1 && cmd2 && ..." or even:
>
> if test -f ${TPLINK:=//path/to/tortoiseplink}
> then
> exec "$TPLINK" "$@"
> elif test -f ${PLINK:=//path/to/plink}
> exec "$PLINK" "$@"
> else
> echo >&2 no usable ssh on this host
> fi
Sure, it could be all of that. It could even blast through the environment
limits in some environments on some platforms, but not on others. It could
automatically transfer bitcoins whenever a connection to github.com is
attempted. It could start the camera and build a time-lapse of "every time
I pushed or fetched a repository", as an art project. It could shut down
the computer.
It could do all of that.
In practice, however, what users realistically do is to use
GIT_SSH_COMMAND whenever they need to override the ssh command with
options.
This is the common case, and that is what we must make less cumbersome to
use.
If you feel strongly about your contrived examples possibly being affected
by this patch, we could easily make this conditional on
1) no '&&' or '||' being found on the command-line, and
2) argv[0] not containing an '='
Another approach would be to verify that argv[i] starts with '-' for
non-zero i.
But do we really need to do that?
Let's have a very brief look at the amount of work required to come up
with a false positive (I am not so much concerned about any power user
writing elaborate shell expressions that may call plink.exe: those power
users will simply have to continue to use their own -P => -p and -batch
hacks):
The logic kicks in if the split command-line's first component has a
basename `plink` or `tortoiseplink` (possibly with `.exe` suffix). The
only way this logic can kick in by mistake is if the first component is
*not* the command to call *and* if the first component *still* has a
basename of `plink` or `tortoiseplink`.
That means that the user has to specify something like
HAHAHA_IT_IS_NOT=/plink.exe ssh
as GIT_SSH_COMMAND.
Now, let's take a *very* brief look at the question how likely it is to
have a basename of `plink` or `tortoiseplink`. Remember, there are two
ways that the basename can be a specific term: either the original string
is already equal to that term, or it ends in a slash followed by the term.
That is, either the first component refers to plink.exe/tortoiseplink.exe, i.e.
it would be a *true* positive. Or the first component would end in
"/plink" or "/tortoiseplink" (possibly with the `.exe` suffix) *and not be
a command*. But how likely is it to specify a non-command (such as a
per-process variable assignment) that specifically mentions plink or
tortoiseplink?
Is it even realistic to expect users to specify values for GIT_SSH_COMMAND
that contain "plink" as a substring when they do *not* want to call plink
at all?
After thinking a bit longer than I had originally planned about it, my
answer is: no. No, I do not think it is realistic. I fully expect *no*
user to have a GIT_SSH_COMMAND containing even a substring "plink"
*anywhere*, unless they do, in fact, want to call plink or tortoiseplink.
My main aim with Git for Windows is to improve the user experience, and
the patch in question does do that. Of course, I also try to avoid
breaking existing setups while improving the user experience, and
I believe that it is safe to assume that the patch in question in all
likelihood does not break any existing setup.
Ciao,
Dscho
^ permalink raw reply
* Re: Preserve/Prune Old Pack Files
From: Mike Hommey @ 2017-01-09 7:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170109062137.zghmurndlbts5x44@sigill.intra.peff.net>
On Mon, Jan 09, 2017 at 01:21:37AM -0500, Jeff King wrote:
> On Wed, Jan 04, 2017 at 09:11:55AM -0700, Martin Fick wrote:
>
> > I am replying to this email across lists because I wanted to
> > highlight to the git community this jgit change to repacking
> > that we have up for review
> >
> > https://git.eclipse.org/r/#/c/87969/
> >
> > This change introduces a new convention for how to preserve
> > old pack files in a staging area
> > (.git/objects/packs/preserved) before deleting them. I
> > wanted to ensure that the new proposed convention would be
> > done in a way that would be satisfactory to the git
> > community as a whole so that it would be more easy to
> > provide the same behavior in git eventually. The preserved
> > pack files (and accompanying index and bitmap files), are not
> > only moved, but they are also renamed so that they no longer
> > will match recursive finds looking for pack files.
>
> It looks like objects/pack/pack-123.pack becomes
> objects/pack/preserved/pack-123.old-pack, and so forth.
> Which seems reasonable, and I'm happy that:
>
> find objects/pack -name '*.pack'
>
> would not find it. :)
>
> I suspect the name-change will break a few tools that you might want to
> use to look at a preserved pack (like verify-pack). I know that's not
> your primary use case, but it seems plausible that somebody may one day
> want to use a preserved pack to try to recover from corruption. I think
> "git index-pack --stdin <objects/packs/preserved/pack-123.old-pack"
> could always be a last-resort for re-admitting the objects to the
> repository.
>
> I notice this doesn't do anything for loose objects. I think they
> technically suffer the same issue, though the race window is much
> shorter (we mmap them and zlib inflate immediately, whereas packfiles
> may stay mapped across many object requests).
>
> I have one other thought that's tangentially related.
>
> I've wondered if we could make object pruning more atomic by
> speculatively moving items to be deleted into some kind of "outgoing"
> object area. Right now you can have a case like:
>
> 0. We have a pack that has commit X, which is reachable, and commit Y,
> which is not.
>
> 1. Process A is repacking. It walks the object graph and finds that X
> is reachable. It begins creating a new pack with X and its
> dependent objects.
>
> 2. Meanwhile, process B pushes up a merge of X and Y, and updates a
> ref to point to it.
>
> 3. Process A finishes writing the new pack, and deletes the old one,
> removing Y. The repository is now corrupt.
>
> I don't have a solution here. I don't think we want to solve it by
> locking the repository for updates during a repack. I have a vague sense
> that a solution could be crafted around moving the old pack into a
> holding area instead of deleting (during which time nobody else would
> see the objects, and thus not reference them), while the repacking
> process checks to see if the actual deletion would break any references
> (and rolls back the deletion if it would).
>
> That's _way_ more complicated than your problem, and as I said, I do not
> have a finished solution. But it seems like they touch on a similar
> concept (a post-delete holding area for objects). So I thought I'd
> mention it in case if spurs any brilliance.
Something that is kind-of in the same family of problems is the
"loosening" or objects on repacks, before they can be pruned.
When you have a large repository and do large rewrite operations
(extreme case, a filter-branch on a multi-hundred-thousands commits),
and you gc for the first time, git will possibly create a *lot* of loose
objects, each of which will consume an inode and a file system block. In
the extreme case, you can end up with git gc filling up multiple extra
gigabytes on your disk.
Mike
^ permalink raw reply
* Re: Preserve/Prune Old Pack Files
From: Jeff King @ 2017-01-09 6:21 UTC (permalink / raw)
To: Martin Fick; +Cc: repo-discuss, jmelvin, jgit-dev, git
In-Reply-To: <5172470.bsscxDU4yv@mfick1-lnx>
On Wed, Jan 04, 2017 at 09:11:55AM -0700, Martin Fick wrote:
> I am replying to this email across lists because I wanted to
> highlight to the git community this jgit change to repacking
> that we have up for review
>
> https://git.eclipse.org/r/#/c/87969/
>
> This change introduces a new convention for how to preserve
> old pack files in a staging area
> (.git/objects/packs/preserved) before deleting them. I
> wanted to ensure that the new proposed convention would be
> done in a way that would be satisfactory to the git
> community as a whole so that it would be more easy to
> provide the same behavior in git eventually. The preserved
> pack files (and accompanying index and bitmap files), are not
> only moved, but they are also renamed so that they no longer
> will match recursive finds looking for pack files.
It looks like objects/pack/pack-123.pack becomes
objects/pack/preserved/pack-123.old-pack, and so forth.
Which seems reasonable, and I'm happy that:
find objects/pack -name '*.pack'
would not find it. :)
I suspect the name-change will break a few tools that you might want to
use to look at a preserved pack (like verify-pack). I know that's not
your primary use case, but it seems plausible that somebody may one day
want to use a preserved pack to try to recover from corruption. I think
"git index-pack --stdin <objects/packs/preserved/pack-123.old-pack"
could always be a last-resort for re-admitting the objects to the
repository.
I notice this doesn't do anything for loose objects. I think they
technically suffer the same issue, though the race window is much
shorter (we mmap them and zlib inflate immediately, whereas packfiles
may stay mapped across many object requests).
I have one other thought that's tangentially related.
I've wondered if we could make object pruning more atomic by
speculatively moving items to be deleted into some kind of "outgoing"
object area. Right now you can have a case like:
0. We have a pack that has commit X, which is reachable, and commit Y,
which is not.
1. Process A is repacking. It walks the object graph and finds that X
is reachable. It begins creating a new pack with X and its
dependent objects.
2. Meanwhile, process B pushes up a merge of X and Y, and updates a
ref to point to it.
3. Process A finishes writing the new pack, and deletes the old one,
removing Y. The repository is now corrupt.
I don't have a solution here. I don't think we want to solve it by
locking the repository for updates during a repack. I have a vague sense
that a solution could be crafted around moving the old pack into a
holding area instead of deleting (during which time nobody else would
see the objects, and thus not reference them), while the repacking
process checks to see if the actual deletion would break any references
(and rolls back the deletion if it would).
That's _way_ more complicated than your problem, and as I said, I do not
have a finished solution. But it seems like they touch on a similar
concept (a post-delete holding area for objects). So I thought I'd
mention it in case if spurs any brilliance.
-Peff
^ permalink raw reply
* Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Jeff King @ 2017-01-09 6:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Johannes Schindelin, git@vger.kernel.org,
David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqy3ylyqhf.fsf@gitster.mtv.corp.google.com>
On Sun, Jan 08, 2017 at 05:25:00PM -0800, Junio C Hamano wrote:
> > If this patch is only to appease coverity (as the commit message eludes
> > to) I think this may be a bad idea for upstream. If this patch fixes an
> > actual problem, then the commit message needs to spell that out.
>
> That is true, and I see Peff pointed out another possible issue
> around getenv(), but I think from the "one step at a time" point of
> view, it is an improvement to call system_path() just once and cache
> it in "static char *".
Yep, I don't think it's a big deal to do it on top, like this:
-- >8 --
Subject: git_exec_path: do not return the result of getenv()
The result of getenv() is not guaranteed by POSIX to last
beyond another call to getenv(), or setenv(), etc. We
should duplicate the string before returning to the caller
to avoid any surprises.
We already keep a cached pointer to avoid repeatedly leaking
the result of system_path(). We can use the same pointer
here to avoid allocating and leaking for each call.
Signed-off-by: Jeff King <peff@peff.net>
---
To be honest, I do not know how big a problem this is. I looked at the
code paths that call git_exec_path(), and the most likely problem case
is calling a second getenv() is via the strbuf functions, which call
xmalloc(), which checks $GIT_ALLOC_LIMIT. We do cache that value, but
it would be a potential problem if this is the first xmalloc call in
the program.
But we are not really solving that here, as xstrdup() would have the
same problem. This _is_ safer, in that we've better contained the
length of time that we expect the result to be valid.
I have no idea what platforms, if any, use a single static buffer for
the getenv() return. I don't know that we've ever gotten a bug report
about it (I only knew about it because somebody pointed it out in one
of my patches a few years ago, so I have it in the back of my mind as
a potential problem).
So I don't mind if this is dropped as "too esoteric" until somebody
actually reports a bug about it.
exec_cmd.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 587bd7eb4..fb94aeba9 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -64,20 +64,19 @@ void git_set_argv_exec_path(const char *exec_path)
/* Returns the highest-priority, location to look for git programs. */
const char *git_exec_path(void)
{
- const char *env;
- static char *system_exec_path;
+ static char *cached_exec_path;
if (argv_exec_path)
return argv_exec_path;
- env = getenv(EXEC_PATH_ENVIRONMENT);
- if (env && *env) {
- return env;
+ if (!cached_exec_path) {
+ const char *env = getenv(EXEC_PATH_ENVIRONMENT);
+ if (env && *env)
+ cached_exec_path = xstrdup(env);
+ else
+ cached_exec_path = system_path(GIT_EXEC_PATH);
}
-
- if (!system_exec_path)
- system_exec_path = system_path(GIT_EXEC_PATH);
- return system_exec_path;
+ return cached_exec_path;
}
static void add_path(struct strbuf *out, const char *path)
--
2.11.0.531.ge85397315
^ permalink raw reply related
* [PATCH v3 02/13] t7610: update branch names to match test number
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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
* [PATCH v3 10/13] t7610: spell 'git reset --hard' consistently
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 11/13] t7610: add test case for rerere+mergetool+subdir bug
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 12/13] mergetool: take the "-O" out of $orderfile
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 09/13] t7610: don't assume the checked-out commit
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 13/13] mergetool: fix running in subdir when rerere enabled
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-1-hansenr@google.com>
[-- Attachment #1: Type: text/plain, Size: 3555 bytes --]
If rerere is enabled and no pathnames are given, run cd_to_toplevel
before running 'git diff --name-only' so that 'git diff --name-only'
sees the files named by 'git rerere remaining', which outputs
pathnames relative to the top-level directory.
The cd_to_toplevel command could be run after 'git rerere remaining',
but it is run before just in case 'git rerere remaining' is ever
changed to print pathnames relative to the current working directory
rather than relative to the top-level directory.
An alternative approach would be to unconditionally convert all
relative pathnames (including the orderfile pathname) to be relative
to the top-level directory and then run cd_to_toplevel before 'git
diff --name-only', but unfortunately 'git rev-parse --prefix' requires
valid pathnames, which would break some valid use cases.
This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).
Signed-off-by: Richard Hansen <hansenr@google.com>
---
git-mergetool.sh | 32 ++++++++++++++++++++++++++++++++
t/t7610-mergetool.sh | 2 +-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index b506896dc..22f56c25a 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -456,6 +456,28 @@ main () {
if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
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
+
set -- $(git rerere remaining)
if test $# -eq 0
then
@@ -463,6 +485,16 @@ main () {
fi
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"} -- "$@")
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index b36fde1c0..a55bf67e1 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 &&
--
2.11.0.390.gc69c2f50cf-goog
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]
^ permalink raw reply related
* [PATCH v3 08/13] t7610: always work on a test-specific branch
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 07/13] t7610: delete some now-unnecessary 'git reset --hard' lines
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 04/13] t7610: use test_when_finished for cleanup tasks
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 03/13] t7610: Move setup code to the 'setup' test case.
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 06/13] t7610: run 'git reset --hard' after each test to clean up
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 05/13] t7610: don't rely on state from previous test
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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 v3 01/13] .mailmap: Use my personal email address as my canonical
From: Richard Hansen @ 2017-01-09 5:42 UTC (permalink / raw)
To: git; +Cc: davvid, j6t, hansenr, sbeller, simon
In-Reply-To: <20170109054238.42599-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
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