* [PATCH v2 1/2] don't use test_must_fail with grep
From: Pranit Bauva @ 2017-01-02 18:45 UTC (permalink / raw)
To: git; +Cc: sbeller, luke, j6t, Pranit Bauva
In-Reply-To: <20161231114412.23439-1-pranit.bauva@gmail.com>
test_must_fail should only be used for testing git commands. To test the
failure of other commands use `!`.
Reported-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
t/t3510-cherry-pick-sequence.sh | 6 +++---
t/t5504-fetch-receive-strict.sh | 2 +-
t/t5516-fetch-push.sh | 2 +-
t/t5601-clone.sh | 2 +-
t/t6030-bisect-porcelain.sh | 2 +-
t/t7610-mergetool.sh | 2 +-
t/t9001-send-email.sh | 2 +-
t/t9117-git-svn-init-clone.sh | 12 ++++++------
t/t9813-git-p4-preserve-users.sh | 8 ++++----
t/t9814-git-p4-rename.sh | 6 +++---
10 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 372307c21..0acf4b146 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -385,7 +385,7 @@ test_expect_success '--continue respects opts' '
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "cherry picked from" initial_msg &&
+ ! grep "cherry picked from" initial_msg &&
grep "cherry picked from" unrelatedpick_msg &&
grep "cherry picked from" picked_msg &&
grep "cherry picked from" anotherpick_msg
@@ -426,9 +426,9 @@ test_expect_failure '--signoff is automatically propagated to resolved conflict'
git cat-file commit HEAD~1 >picked_msg &&
git cat-file commit HEAD~2 >unrelatedpick_msg &&
git cat-file commit HEAD~3 >initial_msg &&
- test_must_fail grep "Signed-off-by:" initial_msg &&
+ ! grep "Signed-off-by:" initial_msg &&
grep "Signed-off-by:" unrelatedpick_msg &&
- test_must_fail grep "Signed-off-by:" picked_msg &&
+ ! grep "Signed-off-by:" picked_msg &&
grep "Signed-off-by:" anotherpick_msg
'
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 9b19cff72..49d3621a9 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -152,7 +152,7 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
git --git-dir=dst/.git config --add \
receive.fsck.badDate warn &&
git push --porcelain dst bogus >act 2>&1 &&
- test_must_fail grep "missingEmail" act
+ ! grep "missingEmail" act
'
test_expect_success \
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2cafc4..0fc5a7c59 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1004,7 +1004,7 @@ test_expect_success 'push --porcelain' '
test_expect_success 'push --porcelain bad url' '
mk_empty testrepo &&
test_must_fail git push >.git/bar --porcelain asdfasdfasd refs/heads/master:refs/remotes/origin/master &&
- test_must_fail grep -q Done .git/bar
+ ! grep -q Done .git/bar
'
test_expect_success 'push --porcelain rejected' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a43339420..4241ea5b3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -151,7 +151,7 @@ test_expect_success 'clone --mirror does not repeat tags' '
git clone --mirror src mirror2 &&
(cd mirror2 &&
git show-ref 2> clone.err > clone.out) &&
- test_must_fail grep Duplicate mirror2/clone.err &&
+ ! grep Duplicate mirror2/clone.err &&
grep some-tag mirror2/clone.out
'
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 5e5370feb..8c2c6eaef 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -407,7 +407,7 @@ test_expect_success 'good merge base when good and bad are siblings' '
test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
grep $HASH4 my_bisect_log.txt &&
git bisect good > my_bisect_log.txt &&
- test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
+ ! grep "merge base must be tested" my_bisect_log.txt &&
grep $HASH6 my_bisect_log.txt &&
git bisect reset
'
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 63d36fb28..0fe7e58cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -602,7 +602,7 @@ test_expect_success MKTEMP 'temporary filenames are used with mergetool.writeToT
test_config mergetool.myecho.trustExitCode true &&
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 &&
grep /both_LOCAL_ actual >/dev/null &&
git reset --hard master >/dev/null 2>&1
'
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 3dc4a3454..0f398dd16 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -50,7 +50,7 @@ test_no_confirm () {
--smtp-server="$(pwd)/fake.sendmail" \
$@ \
$patches >stdout &&
- test_must_fail grep "Send this email" stdout &&
+ ! grep "Send this email" stdout &&
>no_confirm_okay
}
diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 69a675052..044f65e91 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -55,7 +55,7 @@ test_expect_success 'clone to target directory with --stdlayout' '
test_expect_success 'init without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn init "$svnrepo"/project/trunk trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -63,7 +63,7 @@ test_expect_success 'init without -s/-T/-b/-t does not warn' '
test_expect_success 'clone without -s/-T/-b/-t does not warn' '
test ! -d trunk &&
git svn clone "$svnrepo"/project/trunk 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
rm -rf trunk &&
rm -f warning
'
@@ -86,7 +86,7 @@ EOF
test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn init -s "$svnrepo"/project project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -95,7 +95,7 @@ test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test ! -d project &&
git svn clone -s "$svnrepo"/project 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "origin/" &&
rm -rf project &&
rm -f warning
@@ -104,7 +104,7 @@ test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn init -s "$svnrepo"/project project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
@@ -113,7 +113,7 @@ test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
test ! -d project &&
git svn clone -s "$svnrepo"/project --prefix "" 2>warning &&
- test_must_fail grep -q prefix warning &&
+ ! grep -q prefix warning &&
test_svn_configured_prefix "" &&
rm -rf project &&
rm -f warning
diff --git a/t/t9813-git-p4-preserve-users.sh b/t/t9813-git-p4-preserve-users.sh
index 0fe231280..798bf2b67 100755
--- a/t/t9813-git-p4-preserve-users.sh
+++ b/t/t9813-git-p4-preserve-users.sh
@@ -126,13 +126,13 @@ test_expect_success 'not preserving user with mixed authorship' '
grep "git author charlie@example.com does not match" &&
make_change_by_user usernamefile3 alice alice@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ git p4 commit >actual 2>&1 &&
+ ! grep "git author.*does not match" actual &&
git config git-p4.skipUserNameCheck true &&
make_change_by_user usernamefile3 Charlie charlie@example.com &&
- git p4 commit |\
- test_must_fail grep "git author.*does not match" &&
+ git p4 commit >actual 2>&1 &&
+ ! grep "git author.*does not match" actual &&
p4_check_commit_author usernamefile3 alice
)
diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index c89992cf9..e7e0268e9 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -141,7 +141,7 @@ test_expect_success 'detect copies' '
git diff-tree -r -C HEAD &&
git p4 submit &&
p4 filelog //depot/file8 &&
- p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file8 | grep -q "branch from" &&
echo "file9" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -154,7 +154,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies true &&
git p4 submit &&
p4 filelog //depot/file9 &&
- p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file9 | grep -q "branch from" &&
echo "file10" >>file2 &&
git commit -a -m "Differentiate file2" &&
@@ -202,7 +202,7 @@ test_expect_success 'detect copies' '
git config git-p4.detectCopies $(($level + 2)) &&
git p4 submit &&
p4 filelog //depot/file12 &&
- p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
+ ! p4 filelog //depot/file12 | grep -q "branch from" &&
echo "file13" >>file2 &&
git commit -a -m "Differentiate file2" &&
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes
From: Jeff King @ 2017-01-02 18:26 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu>
On Mon, Jan 02, 2017 at 07:06:32PM +0100, Michael Haggerty wrote:
> My assumption was that only valid reference names should ever be allowed
> to be inserted into a `ref_transaction` entry. But Peff is right that
> sometimes the reference name is checked by `refname_is_safe()` rather
> than `check_refname_format()`. Let's audit this more carefully...
>
> * `ref_transaction_add_update()` relies on its caller doing the check
> (this fact is documented). Its callers are:
> * `ref_transaction_update()` (the usual codepath), which checks the
> reference itself using either check_refname_format() or
> refname_is_safe() depending on what kind of update it is.
> * `split_head_update()` passes the literal string "HEAD".
> * `split_symref_update()` picks apart reference updates that go
> through existing symbolic references. As such I don't think they
> are an attack surface. It doesn't do any checking itself (here
> we're talking about its `referent` argument). It has only one
> caller:
> * `lock_ref_for_update()`, which gets `referent` from:
> * `files_read_raw_ref()`, which gets the value either:
> * by reading a filesystem-level symlink's contents and
> checking it with `check_refname_format()`, or
> * reading a symref from the filesystem. In this case, *the
> value is not checked*.
>
> Obviously this chain of custody is disconcertingly long and complicated.
> And the gap for symrefs should probably be fixed, even though they are
> hopefully trustworthy.
Thanks as always for a careful analysis. I agree it seems like a bug
that symlinks are checked but symrefs are not.
> I think the best thing to do is to drop this patch from the patch
> series, and address these checks in a separate series. (There are more
> known problems in this area, for example that the checks in
> `check_refname_format()` are not a strict superset of the checks in
> `refname_is_safe()`.)
Sounds like a good plan. I'd be very happy if the "superset" mismatch is
fixed. It seems like it has come up in our discussions more than once.
-Peff
^ permalink raw reply
* Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function
From: Jeff King @ 2017-01-02 18:22 UTC (permalink / raw)
To: Michael Haggerty; +Cc: brian m. carlson, git
In-Reply-To: <eb8a3dad-9ca7-e0e6-3c31-9cd2e02e0bf9@alum.mit.edu>
On Mon, Jan 02, 2017 at 07:18:58PM +0100, Michael Haggerty wrote:
> > Given that this patch only converts one caller, I'd be more inclined to
> > just have the caller do its own hashcpy. Like:
> >
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 1173071859..16345688b5 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
> >
> > for (i = 0; i < p->num_objects; i++) {
> > const unsigned char *sha1 = nth_packed_object_sha1(p, i);
> > + struct object_id oid;
> >
> > if (!sha1)
> > return error("unable to get sha1 of object %u in %s",
> > i, p->pack_name);
> >
> > - r = cb(sha1, p, i, data);
> > + hashcpy(oid.hash, sha1);
> > + r = cb(&oid, p, i, data);
> > if (r)
> > break;
> > }
> >
> > That punts on the issue for all the other callers, but like I said, I'm
> > not quite sure if, when, and how it would make sense to convert them to
> > using a "struct oid".
>
> Your change is not safe if any of the callback functions ("cb") tuck
> away a copy of the pointer that they are passed, expecting it to contain
> the same object id later.
I think it's generally a given that callback functions should not assume
the lifetime of parameters extend beyond the end of the callback. That
said, I didn't audit the callers (although I'm pretty sure I wrote all
of them myself in this particular case).
-Peff
^ permalink raw reply
* Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function
From: Michael Haggerty @ 2017-01-02 18:18 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, git
In-Reply-To: <20170102170902.6g6bxvaanewxzm2v@sigill.intra.peff.net>
On 01/02/2017 06:09 PM, Jeff King wrote:
> [...]
> But I think the thing that gives me the most pause is that the oid
> version of the function feels bolted-on. The nth_packed_object_sha1()
> function is there specifically to give access to the mmap'd pack-index
> data. And at least for now, that only stores sha1s, not any kind of
> struct. If and when it does learn about other hashes, I'm not sure if
> we're going to want a generic nth_packed_object_oid() function, or if
> the callers would need to handle the various cases specially.
>
> Given that this patch only converts one caller, I'd be more inclined to
> just have the caller do its own hashcpy. Like:
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 1173071859..16345688b5 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
>
> for (i = 0; i < p->num_objects; i++) {
> const unsigned char *sha1 = nth_packed_object_sha1(p, i);
> + struct object_id oid;
>
> if (!sha1)
> return error("unable to get sha1 of object %u in %s",
> i, p->pack_name);
>
> - r = cb(sha1, p, i, data);
> + hashcpy(oid.hash, sha1);
> + r = cb(&oid, p, i, data);
> if (r)
> break;
> }
>
> That punts on the issue for all the other callers, but like I said, I'm
> not quite sure if, when, and how it would make sense to convert them to
> using a "struct oid".
Your change is not safe if any of the callback functions ("cb") tuck
away a copy of the pointer that they are passed, expecting it to contain
the same object id later.
Michael
^ permalink raw reply
* Re: [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Michael Haggerty @ 2017-01-02 18:14 UTC (permalink / raw)
To: Jeff King, Jacob Keller
Cc: Philip Oakley, Junio C Hamano, Git mailing list, David Turner
In-Reply-To: <20170102041947.5jzx6og5fcpv7oso@sigill.intra.peff.net>
On 01/02/2017 05:19 AM, Jeff King wrote:
> On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:
>
>> But how likely is it to end up with differing binaries running on the
>> exact same repository concurrently? Basically, I am trying to see
>> whether or not we could accidentally end up causing problems by trying
>> to race with other git processes that haven't yet been made safe
>> against race? Is the worst case only that some git operation would
>> fail and you would have to retry?
>
> Yes, I think that is the worst case.
>
> A more likely scenario might be something like a server accepting pushes
> or other ref updates from both JGit and regular git (or maybe libgit2
> and regular git).
>
> IMHO it's not really worth worrying about too much. Certain esoteric
> setups might have a slightly higher chance of a pretty obscure race
> condition happening on a very busy repository. I hate to say "eh, ship
> it, we'll see if anybody complains". But I'd be surprised to get a
> single report about this.
I agree. I think these races would mostly affect busy hosting sites and
result in failed updates rather than corruption. And the races can
already occur whenever the user runs `git pack-refs`. By contrast, the
failure to delete empty directories is more likely to affect normal users.
That being said, if Junio wants to merge all but the last two patches in
one release, then merge the last two a release or two later, I have no
objections.
Michael
^ permalink raw reply
* Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes
From: Michael Haggerty @ 2017-01-02 18:06 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git, David Turner
In-Reply-To: <20170101055947.7b5jxih3wlprqcil@sigill.intra.peff.net>
On 01/01/2017 06:59 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 06:30:01PM -0800, Junio C Hamano wrote:
>
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> "refname" has already been checked by check_refname_format(), so it
>>> cannot have consecutive slashes.
>>
>> In the endgame state, this has two callers. Both use what came in
>> the transaction->updates[] array. Presumably "has already been
>> checked by check_refname_format()" says that whoever created entries
>> in that array must have called the function, but it would be helpful
>> to be more explicit here.
>
> Hmm, yeah. This is called when we are deleting a ref, and I thought we
> explicitly _didn't_ do check_refname_format() when deleting, so that
> funny-named refs could be deleted. It's only is_refname_safe() that we
> must pass.
>
> So I have no idea if that's a problem in the code or not, but it is at
> least not immediately obvious who is responsible for calling
> check_refname_format() here.
My assumption was that only valid reference names should ever be allowed
to be inserted into a `ref_transaction` entry. But Peff is right that
sometimes the reference name is checked by `refname_is_safe()` rather
than `check_refname_format()`. Let's audit this more carefully...
* `ref_transaction_add_update()` relies on its caller doing the check
(this fact is documented). Its callers are:
* `ref_transaction_update()` (the usual codepath), which checks the
reference itself using either check_refname_format() or
refname_is_safe() depending on what kind of update it is.
* `split_head_update()` passes the literal string "HEAD".
* `split_symref_update()` picks apart reference updates that go
through existing symbolic references. As such I don't think they
are an attack surface. It doesn't do any checking itself (here
we're talking about its `referent` argument). It has only one
caller:
* `lock_ref_for_update()`, which gets `referent` from:
* `files_read_raw_ref()`, which gets the value either:
* by reading a filesystem-level symlink's contents and
checking it with `check_refname_format()`, or
* reading a symref from the filesystem. In this case, *the
value is not checked*.
Obviously this chain of custody is disconcertingly long and complicated.
And the gap for symrefs should probably be fixed, even though they are
hopefully trustworthy.
`refname_is_safe()` checks that its argument is either UPPER_CASE or
looks like a plausible filename under "refs/". Contrary to its
docstring, it *does not* accept strings that contain successive slashes
or "." or ".." components. It was made stricter in
e40f355 "refname_is_safe(): insist that the refname already be
normalized", 2016-04-27
, but the docstring wasn't updated at that time. I'll fix it.
I think the best thing to do is to drop this patch from the patch
series, and address these checks in a separate series. (There are more
known problems in this area, for example that the checks in
`check_refname_format()` are not a strict superset of the checks in
`refname_is_safe()`.)
Michael
^ permalink raw reply
* Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents
From: Jeff King @ 2017-01-02 17:10 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <9215973c-8db1-8f5a-2dc7-3a0137dd5c62@alum.mit.edu>
On Mon, Jan 02, 2017 at 05:27:59PM +0100, Michael Haggerty wrote:
> > or something. But I doubt the single allocation is breaking the bank,
> > and it has the nice side effect that callers can pass in a const string
> > (I didn't check yet whether that enables further cleanups).
>
> The last patch in the series passes ref_update::refname to this
> function, which is `const char *`. With your suggested change, either
> that member would have to be made non-const, or it would have to be cast
> to const at the `try_remove_empty_parents()` callsite.
>
> Making the member non-const would be easy, though it loses a tiny bit of
> documentation and safety against misuse. Also, scribbling even
> temporarily over that member would mean that this code is not
> thread-safe (though it seems unlikely that we would ever bother making
> it multithreaded).
>
> I think I prefer the current version because it loosens the coupling
> between this function and its callers. But I don't mind either way if
> somebody feels strongly about it.
OK, let's take what you have here, then.
> As an aside, I wonder whether we would be discussing this at all if we
> had stack-allocated strbufs that could be used without allocating heap
> memory in the usual case.
I'm not sure. We still pay the memcpy(), though I don't know how
substantial that is compared to an allocation. For these small strings,
probably not very.
-Peff
^ permalink raw reply
* Re: [PATCH 14/17] sha1_file: introduce an nth_packed_object_oid function
From: Jeff King @ 2017-01-02 17:09 UTC (permalink / raw)
To: Michael Haggerty; +Cc: brian m. carlson, git
In-Reply-To: <8c205558-928d-42ac-d401-e73e19c96030@alum.mit.edu>
On Mon, Jan 02, 2017 at 04:30:21PM +0100, Michael Haggerty wrote:
> On 01/01/2017 08:18 PM, brian m. carlson wrote:
> > There are places in the code where we would like to provide a struct
> > object_id *, yet read the hash directly from the pack. Provide an
> > nth_packed_object_oid function that mirrors the nth_packed_object_sha1
> > function.
> >
> > The required cast is questionable, but should be safe on all known
> > platforms. The alternative of allocating an object as an intermediate
> > would be too inefficient and cause memory leaks. If necessary, an
> > intermediate union could be used; this practice is allowed by GCC and
> > explicitly sanctioned by C11. However, such a change will likely not be
> > necessary, and can be made if and when it is.
>
> I have the feeling that this design decision has been discussed on the
> mailing list. If so, could you include a URL here?
I don't recall an explicit discussion, but I think we already do this
for similar reasons in decode_tree_entry(), where we point to the sha1s
embedded in the tree buffer (of course, you may argue that the cast
there is gross, too :) ).
I'm actually not sure how bad this cast is by the C standard. Conversion
between a struct and its first member is pretty common, and the
alignment is guaranteed by the standard. I think it probably breaks
strict aliasing rules, but then so does "struct object". This is
slightly more exotic in that it's not a struct-to-struct cast, and I
could believe that compilers treat those specially.
> The obvious alternative to allocating a new object to return to the
> caller would be to have the caller of `nth_packed_object_oid()` pass a
> `struct object_id *` argument to be filled in (by copying the hash into
> it). Aside from being legal C, this would also be a more robust step
> towards a post-SHA-1 world, where the suggested hack wouldn't work.
>
> Of course, the question is what the callers want to do with the
> `object_id`. Are the return values of `nth_packed_object_sha1()` stored
> to other longer-lived structures that rely on the lifetime of the
> packfile mmap to keep the value valid? If so, then keeping track of the
> lifetime of the `struct object_id` could be a big chore, not to mention
> that needing to keep a 20-byte `struct object_id` around rather than a
> 8- or 4-byte pointer would also cost more RAM.
I agree that in general, copying the values out is a safer and saner
interface. There are definitely lifetime and memory-use issues to
consider (e.g., the loop in verify_packfile()). And I'd have a slight
worry that the performance impact might be noticeable, just because this
is really quite a low-level function that gets called a lot (but of
course measuring trumps everything there).
But I think the thing that gives me the most pause is that the oid
version of the function feels bolted-on. The nth_packed_object_sha1()
function is there specifically to give access to the mmap'd pack-index
data. And at least for now, that only stores sha1s, not any kind of
struct. If and when it does learn about other hashes, I'm not sure if
we're going to want a generic nth_packed_object_oid() function, or if
the callers would need to handle the various cases specially.
Given that this patch only converts one caller, I'd be more inclined to
just have the caller do its own hashcpy. Like:
diff --git a/sha1_file.c b/sha1_file.c
index 1173071859..16345688b5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3769,12 +3769,14 @@ static int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn c
for (i = 0; i < p->num_objects; i++) {
const unsigned char *sha1 = nth_packed_object_sha1(p, i);
+ struct object_id oid;
if (!sha1)
return error("unable to get sha1 of object %u in %s",
i, p->pack_name);
- r = cb(sha1, p, i, data);
+ hashcpy(oid.hash, sha1);
+ r = cb(&oid, p, i, data);
if (r)
break;
}
That punts on the issue for all the other callers, but like I said, I'm
not quite sure if, when, and how it would make sense to convert them to
using a "struct oid".
-Peff
^ permalink raw reply related
* Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents
From: Michael Haggerty @ 2017-01-02 16:27 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, David Turner
In-Reply-To: <20161231064053.prvlw6x6qprzkmw7@sigill.intra.peff.net>
On 12/31/2016 07:40 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote:
>
>> It's bad manners and surprising and therefore error-prone.
>
> Agreed.
>
> I wondered while reading your earlier patch whether the
> safe_create_leading_directories() function had the same problem, but it
> carefully replaces the NUL it inserts.
>
>> -static void try_remove_empty_parents(char *refname)
>> +static void try_remove_empty_parents(const char *refname)
>> {
>> + struct strbuf buf = STRBUF_INIT;
>> char *p, *q;
>> int i;
>> - p = refname;
>> +
>> + strbuf_addstr(&buf, refname);
>
> I see here you just make a copy. I think it would be enough to do:
>
>> @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)
>> q--;
>> if (q == p)
>> break;
>> - *q = '\0';
>> - if (rmdir(git_path("%s", refname)))
>> + strbuf_setlen(&buf, q - buf.buf);
>> + if (rmdir(git_path("%s", buf.buf)))
>> break;
>
> *q = '\0';
> r = rmdir(git_path("%s", refname));
> *q = '/';
>
> if (r)
> break;
>
> or something. But I doubt the single allocation is breaking the bank,
> and it has the nice side effect that callers can pass in a const string
> (I didn't check yet whether that enables further cleanups).
The last patch in the series passes ref_update::refname to this
function, which is `const char *`. With your suggested change, either
that member would have to be made non-const, or it would have to be cast
to const at the `try_remove_empty_parents()` callsite.
Making the member non-const would be easy, though it loses a tiny bit of
documentation and safety against misuse. Also, scribbling even
temporarily over that member would mean that this code is not
thread-safe (though it seems unlikely that we would ever bother making
it multithreaded).
I think I prefer the current version because it loosens the coupling
between this function and its callers. But I don't mind either way if
somebody feels strongly about it.
As an aside, I wonder whether we would be discussing this at all if we
had stack-allocated strbufs that could be used without allocating heap
memory in the usual case.
Michael
^ permalink raw reply
* [PATCH v4 4/4] t7800: run both builtin and scripted difftool, for now
From: Johannes Schindelin @ 2017-01-02 16:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
This is uglier than a simple
touch "$GIT_EXEC_PATH/use-builtin-difftool"
of course. But oh well.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
This patch implements the good ole' cross-validation technique
(also known as "GitHub Scientist") that I already used for my
rebase--helper work.
I am not sure whether we want to have that patch in `master`,
ever. But at least for the transition time, it may make sense.
t/t7800-difftool.sh | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..273ab55723 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,20 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
+for use_builtin_difftool in false true
+do
+
+test_expect_success 'verify we are running the correct difftool' '
+ if test true = '$use_builtin_difftool'
+ then
+ test_must_fail ok=129 git difftool -h >help &&
+ grep "g, --gui" help
+ else
+ git difftool -h >help &&
+ grep "g|--gui" help
+ fi
+'
+
# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
# Create a file on master and change it on branch
@@ -606,4 +620,17 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
)
'
+test true != $use_builtin_difftool || break
+
+test_expect_success 'tear down for re-run' '
+ rm -rf * .[a-z]* &&
+ git init
+'
+
+# run as builtin difftool now
+GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
+export GIT_CONFIG_PARAMETERS
+
+done
+
test_done
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
From: Johannes Schindelin @ 2017-01-02 16:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
Technically, it is correct that git_exec_path() returns a possibly
malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
let's just use a static variable to make it a singleton. That'll shut
Coverity up, hopefully.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
exec_cmd.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 19ac2146d0..587bd7eb48 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,6 +65,7 @@ void git_set_argv_exec_path(const char *exec_path)
const char *git_exec_path(void)
{
const char *env;
+ static char *system_exec_path;
if (argv_exec_path)
return argv_exec_path;
@@ -74,7 +75,9 @@ const char *git_exec_path(void)
return env;
}
- return system_path(GIT_EXEC_PATH);
+ if (!system_exec_path)
+ system_exec_path = system_path(GIT_EXEC_PATH);
+ return system_exec_path;
}
static void add_path(struct strbuf *out, const char *path)
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v4 3/4] difftool: implement the functionality in the builtin
From: Johannes Schindelin @ 2017-01-02 16:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
This patch gives life to the skeleton added in the previous patch.
The motivation for converting the difftool is that Perl scripts are not at
all native on Windows, and that `git difftool` therefore is pretty slow on
that platform, when there is no good reason for it to be slow.
In addition, Perl does not really have access to Git's internals. That
means that any script will always have to jump through unnecessary
hoops.
The current version of the builtin difftool does not, however, make full
use of the internals but instead chooses to spawn a couple of Git
processes, still, to make for an easier conversion. There remains a lot
of room for improvement, left for a later date.
Note: to play it safe, the original difftool is still called unless the
config setting difftool.useBuiltin is set to true.
The reason: this new, experimental, builtin difftool will be shipped as
part of Git for Windows v2.11.0, to allow for easier large-scale
testing, but of course as an opt-in feature.
Sadly, the speedup is more noticable on Linux than on Windows: a quick
test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s)
(real/user/sys) in a Linux VM, down from (6.529s/3.112s/0.644s), while
on Windows, it is (36.064s/2.730s/7.194s), down from
(47.637s/2.407s/6.863s). The culprit is most likely the overhead
incurred from *still* having to shell out to mergetool-lib.sh and
difftool--helper.sh.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/difftool.c | 672 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 671 insertions(+), 1 deletion(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 53870bbaf7..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -11,9 +11,610 @@
*
* Copyright (C) 2016 Johannes Schindelin
*/
+#include "cache.h"
#include "builtin.h"
#include "run-command.h"
#include "exec_cmd.h"
+#include "parse-options.h"
+#include "argv-array.h"
+#include "strbuf.h"
+#include "lockfile.h"
+#include "dir.h"
+
+static char *diff_gui_tool;
+static int trust_exit_code;
+
+static const char *const builtin_difftool_usage[] = {
+ N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
+ NULL
+};
+
+static int difftool_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "diff.guitool")) {
+ diff_gui_tool = xstrdup(value);
+ return 0;
+ }
+
+ if (!strcmp(var, "difftool.trustexitcode")) {
+ trust_exit_code = git_config_bool(var, value);
+ return 0;
+ }
+
+ return git_default_config(var, value, cb);
+}
+
+static int print_tool_help(void)
+{
+ const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
+ return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int parse_index_info(char *p, int *mode1, int *mode2,
+ struct object_id *oid1, struct object_id *oid2,
+ char *status)
+{
+ if (*p != ':')
+ return error("expected ':', got '%c'", *p);
+ *mode1 = (int)strtol(p + 1, &p, 8);
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ *mode2 = (int)strtol(p + 1, &p, 8);
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ if (get_oid_hex(++p, oid1))
+ return error("expected object ID, got '%s'", p + 1);
+ p += GIT_SHA1_HEXSZ;
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ if (get_oid_hex(++p, oid2))
+ return error("expected object ID, got '%s'", p + 1);
+ p += GIT_SHA1_HEXSZ;
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ *status = *++p;
+ if (!*status)
+ return error("missing status");
+ if (p[1] && !isdigit(p[1]))
+ return error("unexpected trailer: '%s'", p + 1);
+ return 0;
+}
+
+/*
+ * Remove any trailing slash from $workdir
+ * before starting to avoid double slashes in symlink targets.
+ */
+static void add_path(struct strbuf *buf, size_t base_len, const char *path)
+{
+ strbuf_setlen(buf, base_len);
+ if (buf->len && buf->buf[buf->len - 1] != '/')
+ strbuf_addch(buf, '/');
+ strbuf_addstr(buf, path);
+}
+
+/*
+ * Determine whether we can simply reuse the file in the worktree.
+ */
+static int use_wt_file(const char *workdir, const char *name,
+ struct object_id *oid)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct stat st;
+ int use = 0;
+
+ strbuf_addstr(&buf, workdir);
+ add_path(&buf, buf.len, name);
+
+ if (!lstat(buf.buf, &st) && !S_ISLNK(st.st_mode)) {
+ struct object_id wt_oid;
+ int fd = open(buf.buf, O_RDONLY);
+
+ if (fd >= 0 &&
+ !index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
+ if (is_null_oid(oid)) {
+ oidcpy(oid, &wt_oid);
+ use = 1;
+ } else if (!oidcmp(oid, &wt_oid))
+ use = 1;
+ }
+ }
+
+ strbuf_release(&buf);
+
+ return use;
+}
+
+struct working_tree_entry {
+ struct hashmap_entry entry;
+ char path[FLEX_ARRAY];
+};
+
+static int working_tree_entry_cmp(struct working_tree_entry *a,
+ struct working_tree_entry *b, void *keydata)
+{
+ return strcmp(a->path, b->path);
+}
+
+/*
+ * The `left` and `right` entries hold paths for the symlinks hashmap,
+ * and a SHA-1 surrounded by brief text for submodules.
+ */
+struct pair_entry {
+ struct hashmap_entry entry;
+ char left[PATH_MAX], right[PATH_MAX];
+ const char path[FLEX_ARRAY];
+};
+
+static int pair_cmp(struct pair_entry *a, struct pair_entry *b, void *keydata)
+{
+ return strcmp(a->path, b->path);
+}
+
+static void add_left_or_right(struct hashmap *map, const char *path,
+ const char *content, int is_right)
+{
+ struct pair_entry *e, *existing;
+
+ FLEX_ALLOC_STR(e, path, path);
+ hashmap_entry_init(e, strhash(path));
+ existing = hashmap_get(map, e, NULL);
+ if (existing) {
+ free(e);
+ e = existing;
+ } else {
+ e->left[0] = e->right[0] = '\0';
+ hashmap_add(map, e);
+ }
+ strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
+}
+
+struct path_entry {
+ struct hashmap_entry entry;
+ char path[FLEX_ARRAY];
+};
+
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
+{
+ return strcmp(a->path, key ? key : b->path);
+}
+
+static void changed_files(struct hashmap *result, const char *index_path,
+ const char *workdir)
+{
+ struct child_process update_index = CHILD_PROCESS_INIT;
+ struct child_process diff_files = CHILD_PROCESS_INIT;
+ struct strbuf index_env = STRBUF_INIT, buf = STRBUF_INIT;
+ const char *git_dir = absolute_path(get_git_dir()), *env[] = {
+ NULL, NULL
+ };
+ FILE *fp;
+
+ strbuf_addf(&index_env, "GIT_INDEX_FILE=%s", index_path);
+ env[0] = index_env.buf;
+
+ argv_array_pushl(&update_index.args,
+ "--git-dir", git_dir, "--work-tree", workdir,
+ "update-index", "--really-refresh", "-q",
+ "--unmerged", NULL);
+ update_index.no_stdin = 1;
+ update_index.no_stdout = 1;
+ update_index.no_stderr = 1;
+ update_index.git_cmd = 1;
+ update_index.use_shell = 0;
+ update_index.clean_on_exit = 1;
+ update_index.dir = workdir;
+ update_index.env = env;
+ /* Ignore any errors of update-index */
+ run_command(&update_index);
+
+ argv_array_pushl(&diff_files.args,
+ "--git-dir", git_dir, "--work-tree", workdir,
+ "diff-files", "--name-only", "-z", NULL);
+ diff_files.no_stdin = 1;
+ diff_files.git_cmd = 1;
+ diff_files.use_shell = 0;
+ diff_files.clean_on_exit = 1;
+ diff_files.out = -1;
+ diff_files.dir = workdir;
+ diff_files.env = env;
+ if (start_command(&diff_files))
+ die("could not obtain raw diff");
+ fp = xfdopen(diff_files.out, "r");
+ while (!strbuf_getline_nul(&buf, fp)) {
+ struct path_entry *entry;
+ FLEX_ALLOC_STR(entry, path, buf.buf);
+ hashmap_entry_init(entry, strhash(buf.buf));
+ hashmap_add(result, entry);
+ }
+ if (finish_command(&diff_files))
+ die("diff-files did not exit properly");
+ strbuf_release(&index_env);
+ strbuf_release(&buf);
+}
+
+static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addstr(&buf, tmpdir);
+ remove_dir_recursively(&buf, 0);
+ if (exit_code)
+ warning(_("failed: %d"), exit_code);
+ exit(exit_code);
+}
+
+static int ensure_leading_directories(char *path)
+{
+ switch (safe_create_leading_directories(path)) {
+ case SCLD_OK:
+ case SCLD_EXISTS:
+ return 0;
+ default:
+ return error(_("could not create leading directories "
+ "of '%s'"), path);
+ }
+}
+
+static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
+ int argc, const char **argv)
+{
+ char tmpdir[PATH_MAX];
+ struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
+ struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
+ struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
+ struct strbuf wtdir = STRBUF_INIT;
+ size_t ldir_len, rdir_len, wtdir_len;
+ struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
+ const char *workdir, *tmp;
+ int ret = 0, i;
+ FILE *fp;
+ struct hashmap working_tree_dups, submodules, symlinks2;
+ struct hashmap_iter iter;
+ struct pair_entry *entry;
+ enum object_type type;
+ unsigned long size;
+ struct index_state wtindex;
+ struct checkout lstate, rstate;
+ int rc, flags = RUN_GIT_CMD, err = 0;
+ struct child_process child = CHILD_PROCESS_INIT;
+ const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+ struct hashmap wt_modified, tmp_modified;
+ int indices_loaded = 0;
+
+ workdir = get_git_work_tree();
+
+ /* Setup temp directories */
+ tmp = getenv("TMPDIR");
+ xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
+ if (!mkdtemp(tmpdir))
+ return error("could not create '%s'", tmpdir);
+ strbuf_addf(&ldir, "%s/left/", tmpdir);
+ strbuf_addf(&rdir, "%s/right/", tmpdir);
+ strbuf_addstr(&wtdir, workdir);
+ if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
+ strbuf_addch(&wtdir, '/');
+ mkdir(ldir.buf, 0700);
+ mkdir(rdir.buf, 0700);
+
+ memset(&wtindex, 0, sizeof(wtindex));
+
+ memset(&lstate, 0, sizeof(lstate));
+ lstate.base_dir = ldir.buf;
+ lstate.base_dir_len = ldir.len;
+ lstate.force = 1;
+ memset(&rstate, 0, sizeof(rstate));
+ rstate.base_dir = rdir.buf;
+ rstate.base_dir_len = rdir.len;
+ rstate.force = 1;
+
+ ldir_len = ldir.len;
+ rdir_len = rdir.len;
+ wtdir_len = wtdir.len;
+
+ hashmap_init(&working_tree_dups,
+ (hashmap_cmp_fn)working_tree_entry_cmp, 0);
+ hashmap_init(&submodules, (hashmap_cmp_fn)pair_cmp, 0);
+ hashmap_init(&symlinks2, (hashmap_cmp_fn)pair_cmp, 0);
+
+ child.no_stdin = 1;
+ child.git_cmd = 1;
+ child.use_shell = 0;
+ child.clean_on_exit = 1;
+ child.dir = prefix;
+ child.out = -1;
+ argv_array_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z",
+ NULL);
+ for (i = 0; i < argc; i++)
+ argv_array_push(&child.args, argv[i]);
+ if (start_command(&child))
+ die("could not obtain raw diff");
+ fp = xfdopen(child.out, "r");
+
+ /* Build index info for left and right sides of the diff */
+ i = 0;
+ while (!strbuf_getline_nul(&info, fp)) {
+ int lmode, rmode;
+ struct object_id loid, roid;
+ char status;
+ const char *src_path, *dst_path;
+ size_t src_path_len, dst_path_len;
+
+ if (starts_with(info.buf, "::"))
+ die(N_("combined diff formats('-c' and '--cc') are "
+ "not supported in\n"
+ "directory diff mode('-d' and '--dir-diff')."));
+
+ if (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid,
+ &status))
+ break;
+ if (strbuf_getline_nul(&lpath, fp))
+ break;
+ src_path = lpath.buf;
+ src_path_len = lpath.len;
+
+ i++;
+ if (status != 'C' && status != 'R') {
+ dst_path = src_path;
+ dst_path_len = src_path_len;
+ } else {
+ if (strbuf_getline_nul(&rpath, fp))
+ break;
+ dst_path = rpath.buf;
+ dst_path_len = rpath.len;
+ }
+
+ if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "Subproject commit %s",
+ oid_to_hex(&loid));
+ add_left_or_right(&submodules, src_path, buf.buf, 0);
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "Subproject commit %s",
+ oid_to_hex(&roid));
+ if (!oidcmp(&loid, &roid))
+ strbuf_addstr(&buf, "-dirty");
+ add_left_or_right(&submodules, dst_path, buf.buf, 1);
+ continue;
+ }
+
+ if (S_ISLNK(lmode)) {
+ char *content = read_sha1_file(loid.hash, &type, &size);
+ add_left_or_right(&symlinks2, src_path, content, 0);
+ free(content);
+ }
+
+ if (S_ISLNK(rmode)) {
+ char *content = read_sha1_file(roid.hash, &type, &size);
+ add_left_or_right(&symlinks2, dst_path, content, 1);
+ free(content);
+ }
+
+ if (lmode && status != 'C') {
+ ce->ce_mode = lmode;
+ oidcpy(&ce->oid, &loid);
+ strcpy(ce->name, src_path);
+ ce->ce_namelen = src_path_len;
+ if (checkout_entry(ce, &lstate, NULL))
+ return error("could not write '%s'", src_path);
+ }
+
+ if (rmode) {
+ struct working_tree_entry *entry;
+
+ /* Avoid duplicate working_tree entries */
+ FLEX_ALLOC_STR(entry, path, dst_path);
+ hashmap_entry_init(entry, strhash(dst_path));
+ if (hashmap_get(&working_tree_dups, entry, NULL)) {
+ free(entry);
+ continue;
+ }
+ hashmap_add(&working_tree_dups, entry);
+
+ if (!use_wt_file(workdir, dst_path, &roid)) {
+ ce->ce_mode = rmode;
+ oidcpy(&ce->oid, &roid);
+ strcpy(ce->name, dst_path);
+ ce->ce_namelen = dst_path_len;
+ if (checkout_entry(ce, &rstate, NULL))
+ return error("could not write '%s'",
+ dst_path);
+ } else if (!is_null_oid(&roid)) {
+ /*
+ * Changes in the working tree need special
+ * treatment since they are not part of the
+ * index.
+ */
+ struct cache_entry *ce2 =
+ make_cache_entry(rmode, roid.hash,
+ dst_path, 0, 0);
+
+ add_index_entry(&wtindex, ce2,
+ ADD_CACHE_JUST_APPEND);
+
+ add_path(&rdir, rdir_len, dst_path);
+ if (ensure_leading_directories(rdir.buf))
+ return error("could not create "
+ "directory for '%s'",
+ dst_path);
+ add_path(&wtdir, wtdir_len, dst_path);
+ if (symlinks) {
+ if (symlink(wtdir.buf, rdir.buf)) {
+ ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
+ goto finish;
+ }
+ } else {
+ struct stat st;
+ if (stat(wtdir.buf, &st))
+ st.st_mode = 0644;
+ if (copy_file(rdir.buf, wtdir.buf,
+ st.st_mode)) {
+ ret = error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf);
+ goto finish;
+ }
+ }
+ }
+ }
+ }
+
+ if (finish_command(&child)) {
+ ret = error("error occurred running diff --raw");
+ goto finish;
+ }
+
+ if (!i)
+ return 0;
+
+ /*
+ * Changes to submodules require special treatment.This loop writes a
+ * temporary file to both the left and right directories to show the
+ * change in the recorded SHA1 for the submodule.
+ */
+ hashmap_iter_init(&submodules, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ if (*entry->left) {
+ add_path(&ldir, ldir_len, entry->path);
+ ensure_leading_directories(ldir.buf);
+ write_file(ldir.buf, "%s", entry->left);
+ }
+ if (*entry->right) {
+ add_path(&rdir, rdir_len, entry->path);
+ ensure_leading_directories(rdir.buf);
+ write_file(rdir.buf, "%s", entry->right);
+ }
+ }
+
+ /*
+ * Symbolic links require special treatment.The standard "git diff"
+ * shows only the link itself, not the contents of the link target.
+ * This loop replicates that behavior.
+ */
+ hashmap_iter_init(&symlinks2, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ if (*entry->left) {
+ add_path(&ldir, ldir_len, entry->path);
+ ensure_leading_directories(ldir.buf);
+ write_file(ldir.buf, "%s", entry->left);
+ }
+ if (*entry->right) {
+ add_path(&rdir, rdir_len, entry->path);
+ ensure_leading_directories(rdir.buf);
+ write_file(rdir.buf, "%s", entry->right);
+ }
+ }
+
+ strbuf_release(&buf);
+
+ strbuf_setlen(&ldir, ldir_len);
+ helper_argv[1] = ldir.buf;
+ strbuf_setlen(&rdir, rdir_len);
+ helper_argv[2] = rdir.buf;
+
+ if (extcmd) {
+ helper_argv[0] = extcmd;
+ flags = 0;
+ } else
+ setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
+ rc = run_command_v_opt(helper_argv, flags);
+
+ /*
+ * If the diff includes working copy files and those
+ * files were modified during the diff, then the changes
+ * should be copied back to the working tree.
+ * Do not copy back files when symlinks are used and the
+ * external tool did not replace the original link with a file.
+ *
+ * These hashes are loaded lazily since they aren't needed
+ * in the common case of --symlinks and the difftool updating
+ * files through the symlink.
+ */
+ hashmap_init(&wt_modified, (hashmap_cmp_fn)path_entry_cmp,
+ wtindex.cache_nr);
+ hashmap_init(&tmp_modified, (hashmap_cmp_fn)path_entry_cmp,
+ wtindex.cache_nr);
+
+ for (i = 0; i < wtindex.cache_nr; i++) {
+ struct hashmap_entry dummy;
+ const char *name = wtindex.cache[i]->name;
+ struct stat st;
+
+ add_path(&rdir, rdir_len, name);
+ if (lstat(rdir.buf, &st))
+ continue;
+
+ if ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))
+ continue;
+
+ if (!indices_loaded) {
+ static struct lock_file lock;
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "%s/wtindex", tmpdir);
+ if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
+ write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
+ ret = error("could not write %s", buf.buf);
+ rollback_lock_file(&lock);
+ goto finish;
+ }
+ changed_files(&wt_modified, buf.buf, workdir);
+ strbuf_setlen(&rdir, rdir_len);
+ changed_files(&tmp_modified, buf.buf, rdir.buf);
+ add_path(&rdir, rdir_len, name);
+ indices_loaded = 1;
+ }
+
+ hashmap_entry_init(&dummy, strhash(name));
+ if (hashmap_get(&tmp_modified, &dummy, name)) {
+ add_path(&wtdir, wtdir_len, name);
+ if (hashmap_get(&wt_modified, &dummy, name)) {
+ warning(_("both files modified: '%s' and '%s'."),
+ wtdir.buf, rdir.buf);
+ warning(_("working tree file has been left."));
+ warning("");
+ err = 1;
+ } else if (unlink(wtdir.buf) ||
+ copy_file(wtdir.buf, rdir.buf, st.st_mode))
+ warning_errno(_("could not copy '%s' to '%s'"),
+ rdir.buf, wtdir.buf);
+ }
+ }
+
+ if (err) {
+ warning(_("temporary files exist in '%s'."), tmpdir);
+ warning(_("you may want to cleanup or recover these."));
+ exit(1);
+ } else
+ exit_cleanup(tmpdir, rc);
+
+finish:
+ free(ce);
+ strbuf_release(&ldir);
+ strbuf_release(&rdir);
+ strbuf_release(&wtdir);
+ strbuf_release(&buf);
+
+ return ret;
+}
+
+static int run_file_diff(int prompt, const char *prefix,
+ int argc, const char **argv)
+{
+ struct argv_array args = ARGV_ARRAY_INIT;
+ const char *env[] = {
+ "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL,
+ NULL
+ };
+ int ret = 0, i;
+
+ if (prompt > 0)
+ env[2] = "GIT_DIFFTOOL_PROMPT=true";
+ else if (!prompt)
+ env[2] = "GIT_DIFFTOOL_NO_PROMPT=true";
+
+
+ argv_array_push(&args, "diff");
+ for (i = 0; i < argc; i++)
+ argv_array_push(&args, argv[i]);
+ ret = run_command_v_opt_cd_env(args.argv, RUN_GIT_CMD, prefix, env);
+ exit(ret);
+}
/*
* NEEDSWORK: this function can go once the legacy-difftool Perl script is
@@ -41,6 +642,35 @@ static int use_builtin_difftool(void) {
int cmd_difftool(int argc, const char **argv, const char *prefix)
{
+ int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
+ tool_help = 0;
+ static char *difftool_cmd = NULL, *extcmd = NULL;
+ struct option builtin_difftool_options[] = {
+ OPT_BOOL('g', "gui", &use_gui_tool,
+ N_("use `diff.guitool` instead of `diff.tool`")),
+ OPT_BOOL('d', "dir-diff", &dir_diff,
+ N_("perform a full-directory diff")),
+ { OPTION_SET_INT, 'y', "no-prompt", &prompt, NULL,
+ N_("do not prompt before launching a diff tool"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
+ { OPTION_SET_INT, 0, "prompt", &prompt, NULL, NULL,
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
+ NULL, 1 },
+ OPT_BOOL(0, "symlinks", &symlinks,
+ N_("use symlinks in dir-diff mode")),
+ OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
+ N_("use the specified diff tool")),
+ OPT_BOOL(0, "tool-help", &tool_help,
+ N_("print a list of diff tools that may be used with "
+ "`--tool`")),
+ OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
+ N_("make 'git-difftool' exit when an invoked diff "
+ "tool returns a non - zero exit code")),
+ OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
+ N_("specify a custom command for viewing diffs")),
+ OPT_END()
+ };
+
/*
* NEEDSWORK: Once the builtin difftool has been tested enough
* and git-legacy-difftool.perl is retired to contrib/, this preamble
@@ -58,6 +688,46 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
prefix = setup_git_directory();
trace_repo_setup(prefix);
setup_work_tree();
+ /* NEEDSWORK: once we no longer spawn anything, remove this */
+ setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+ setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+
+ git_config(difftool_config, NULL);
+ symlinks = has_symlinks;
+
+ argc = parse_options(argc, argv, prefix, builtin_difftool_options,
+ builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
+ PARSE_OPT_KEEP_DASHDASH);
- die("TODO");
+ if (tool_help)
+ return print_tool_help();
+
+ if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
+ setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+ else if (difftool_cmd) {
+ if (*difftool_cmd)
+ setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
+ else
+ die(_("no <tool> given for --tool=<tool>"));
+ }
+
+ if (extcmd) {
+ if (*extcmd)
+ setenv("GIT_DIFFTOOL_EXTCMD", extcmd, 1);
+ else
+ die(_("no <cmd> given for --extcmd=<cmd>"));
+ }
+
+ setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
+ trust_exit_code ? "true" : "false", 1);
+
+ /*
+ * In directory diff mode, 'git-difftool--helper' is called once
+ * to compare the a / b directories. In file diff mode, 'git diff'
+ * will invoke a separate instance of 'git-difftool--helper' for
+ * each file that changed.
+ */
+ if (dir_diff)
+ return run_dir_diff(extcmd, symlinks, prefix, argc, argv);
+ return run_file_diff(prompt, prefix, argc, argv);
}
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v4 2/4] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2017-01-02 16:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
This adds a builtin difftool that still falls back to the legacy Perl
version, which has been renamed to `legacy-difftool`.
The idea is that the new, experimental, builtin difftool immediately hands
off to the legacy difftool for now, unless the config variable
difftool.useBuiltin is set to true.
This feature flag will be used in the upcoming Git for Windows v2.11.0
release, to allow early testers to opt-in to use the builtin difftool and
flesh out any bugs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.gitignore | 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/difftool.c | 63 +++++++++++++++++++++++++++
git-difftool.perl => git-legacy-difftool.perl | 0
git.c | 6 +++
t/t7800-difftool.sh | 2 +
7 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 builtin/difftool.c
rename git-difftool.perl => git-legacy-difftool.perl (100%)
diff --git a/.gitignore b/.gitignore
index 6722f78f9a..5555ae025b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
/git-init-db
/git-interpret-trailers
/git-instaweb
+/git-legacy-difftool
/git-log
/git-ls-files
/git-ls-remote
diff --git a/Makefile b/Makefile
index d861bd9985..8cf5bef034 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,7 @@ SCRIPT_LIB += git-sh-setup
SCRIPT_LIB += git-sh-i18n
SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-difftool.perl
+SCRIPT_PERL += git-legacy-difftool.perl
SCRIPT_PERL += git-archimport.perl
SCRIPT_PERL += git-cvsexportcommit.perl
SCRIPT_PERL += git-cvsimport.perl
@@ -883,6 +883,7 @@ BUILTIN_OBJS += builtin/diff-files.o
BUILTIN_OBJS += builtin/diff-index.o
BUILTIN_OBJS += builtin/diff-tree.o
BUILTIN_OBJS += builtin/diff.o
+BUILTIN_OBJS += builtin/difftool.o
BUILTIN_OBJS += builtin/fast-export.o
BUILTIN_OBJS += builtin/fetch-pack.o
BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index b9122bc5f4..67f80519da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,6 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const char *prefix);
extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
extern int cmd_diff(int argc, const char **argv, const char *prefix);
extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_difftool(int argc, const char **argv, const char *prefix);
extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
extern int cmd_fetch(int argc, const char **argv, const char *prefix);
extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/difftool.c b/builtin/difftool.c
new file mode 100644
index 0000000000..53870bbaf7
--- /dev/null
+++ b/builtin/difftool.c
@@ -0,0 +1,63 @@
+/*
+ * "git difftool" builtin command
+ *
+ * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
+ * git-difftool--helper script.
+ *
+ * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+ * The GIT_DIFF* variables are exported for use by git-difftool--helper.
+ *
+ * Any arguments that are unknown to this script are forwarded to 'git diff'.
+ *
+ * Copyright (C) 2016 Johannes Schindelin
+ */
+#include "builtin.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+
+/*
+ * NEEDSWORK: this function can go once the legacy-difftool Perl script is
+ * retired.
+ *
+ * We intentionally avoid reading the config directly here, to avoid messing up
+ * the GIT_* environment variables when we need to fall back to exec()ing the
+ * Perl script.
+ */
+static int use_builtin_difftool(void) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ int ret;
+
+ argv_array_pushl(&cp.args,
+ "config", "--bool", "difftool.usebuiltin", NULL);
+ cp.git_cmd = 1;
+ if (capture_command(&cp, &out, 6))
+ return 0;
+ strbuf_trim(&out);
+ ret = !strcmp("true", out.buf);
+ strbuf_release(&out);
+ return ret;
+}
+
+int cmd_difftool(int argc, const char **argv, const char *prefix)
+{
+ /*
+ * NEEDSWORK: Once the builtin difftool has been tested enough
+ * and git-legacy-difftool.perl is retired to contrib/, this preamble
+ * can be removed.
+ */
+ if (!use_builtin_difftool()) {
+ const char *path = mkpath("%s/git-legacy-difftool",
+ git_exec_path());
+
+ if (sane_execvp(path, (char **)argv) < 0)
+ die_errno("could not exec %s", path);
+
+ return 0;
+ }
+ prefix = setup_git_directory();
+ trace_repo_setup(prefix);
+ setup_work_tree();
+
+ die("TODO");
+}
diff --git a/git-difftool.perl b/git-legacy-difftool.perl
similarity index 100%
rename from git-difftool.perl
rename to git-legacy-difftool.perl
diff --git a/git.c b/git.c
index dce529fcbf..044958a780 100644
--- a/git.c
+++ b/git.c
@@ -424,6 +424,12 @@ static struct cmd_struct commands[] = {
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
{ "diff-index", cmd_diff_index, RUN_SETUP },
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
+ /*
+ * NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
+ * builtin/difftool.c has been removed, this entry should be changed to
+ * RUN_SETUP | NEED_WORK_TREE
+ */
+ { "difftool", cmd_difftool },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 99d4123461..e94910c563 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,8 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
+# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
+
# Create a file on master and change it on branch
test_expect_success PERL 'setup' '
echo master >file &&
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v4 0/4] Show Git Mailing List: a builtin difftool
From: Johannes Schindelin @ 2017-01-02 16:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1480019834.git.johannes.schindelin@gmx.de>
I have been working on the builtin difftool for a while now, for two
reasons:
1. Perl is really not native on Windows. Not only is there a performance
penalty to be paid just for running Perl scripts, we also have to deal
with the fact that users may have different Perl installations, with
different options, and some other Perl installation may decide to set
PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
have to use because almost all other Perl distributions lack the
Subversion bindings we need for `git svn`).
2. Perl makes for a rather large reason that Git for Windows' installer
weighs in with >30MB. While one Perl script less does not relieve us
of that burden, it is one step in the right direction.
Changes since v3:
- made path_entry_cmp static
- fixed a few bugs identified by Coverity
- fixed overzealous status parsing that did not expect any number after
C or R
Johannes Schindelin (4):
Avoid Coverity warning about unfree()d git_exec_path()
difftool: add a skeleton for the upcoming builtin
difftool: implement the functionality in the builtin
t7800: run both builtin and scripted difftool, for now
.gitignore | 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/difftool.c | 733 ++++++++++++++++++++++++++
exec_cmd.c | 5 +-
git-difftool.perl => git-legacy-difftool.perl | 0
git.c | 6 +
t/t7800-difftool.sh | 29 +
8 files changed, 776 insertions(+), 2 deletions(-)
create mode 100644 builtin/difftool.c
rename git-difftool.perl => git-legacy-difftool.perl (100%)
base-commit: e05806da9ec4aff8adfed142ab2a2b3b02e33c8c
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v4
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v4
Interdiff vs v3:
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 3480920fea..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -73,8 +73,10 @@ static int parse_index_info(char *p, int *mode1, int *mode2,
if (*p != ' ')
return error("expected ' ', got '%c'", *p);
*status = *++p;
- if (!status || p[1])
- return error("unexpected trailer: '%s'", p);
+ if (!*status)
+ return error("missing status");
+ if (p[1] && !isdigit(p[1]))
+ return error("unexpected trailer: '%s'", p + 1);
return 0;
}
@@ -107,7 +109,8 @@ static int use_wt_file(const char *workdir, const char *name,
struct object_id wt_oid;
int fd = open(buf.buf, O_RDONLY);
- if (!index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
+ if (fd >= 0 &&
+ !index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
if (is_null_oid(oid)) {
oidcpy(oid, &wt_oid);
use = 1;
@@ -162,7 +165,7 @@ static void add_left_or_right(struct hashmap *map, const char *path,
e->left[0] = e->right[0] = '\0';
hashmap_add(map, e);
}
- strcpy(is_right ? e->right : e->left, content);
+ strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
}
struct path_entry {
@@ -170,7 +173,7 @@ struct path_entry {
char path[FLEX_ARRAY];
};
-int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
{
return strcmp(a->path, key ? key : b->path);
}
@@ -423,17 +426,16 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
struct cache_entry *ce2 =
make_cache_entry(rmode, roid.hash,
dst_path, 0, 0);
- ce_mode_from_stat(ce2, rmode);
add_index_entry(&wtindex, ce2,
ADD_CACHE_JUST_APPEND);
- add_path(&wtdir, wtdir_len, dst_path);
add_path(&rdir, rdir_len, dst_path);
if (ensure_leading_directories(rdir.buf))
return error("could not create "
"directory for '%s'",
dst_path);
+ add_path(&wtdir, wtdir_len, dst_path);
if (symlinks) {
if (symlink(wtdir.buf, rdir.buf)) {
ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
diff --git a/exec_cmd.c b/exec_cmd.c
index 19ac2146d0..587bd7eb48 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,6 +65,7 @@ void git_set_argv_exec_path(const char *exec_path)
const char *git_exec_path(void)
{
const char *env;
+ static char *system_exec_path;
if (argv_exec_path)
return argv_exec_path;
@@ -74,7 +75,9 @@ const char *git_exec_path(void)
return env;
}
- return system_path(GIT_EXEC_PATH);
+ if (!system_exec_path)
+ system_exec_path = system_path(GIT_EXEC_PATH);
+ return system_exec_path;
}
static void add_path(struct strbuf *out, const char *path)
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..273ab55723 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,20 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
+for use_builtin_difftool in false true
+do
+
+test_expect_success 'verify we are running the correct difftool' '
+ if test true = '$use_builtin_difftool'
+ then
+ test_must_fail ok=129 git difftool -h >help &&
+ grep "g, --gui" help
+ else
+ git difftool -h >help &&
+ grep "g|--gui" help
+ fi
+'
+
# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
# Create a file on master and change it on branch
@@ -606,4 +620,17 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
)
'
+test true != $use_builtin_difftool || break
+
+test_expect_success 'tear down for re-run' '
+ rm -rf * .[a-z]* &&
+ git init
+'
+
+# run as builtin difftool now
+GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
+export GIT_CONFIG_PARAMETERS
+
+done
+
test_done
--
2.11.0.rc3.windows.1
^ permalink raw reply
* Re: builtin difftool parsing issue
From: Johannes Schindelin @ 2017-01-02 16:12 UTC (permalink / raw)
To: Paul Sbarra; +Cc: davvid, dennis, git, gitster
In-Reply-To: <CAGf+dShpkPvsC8wQN6mWmYeMZ3=i-ZOzDNSM1aa0rinKW6+-+g@mail.gmail.com>
Hi Paul,
On Wed, 21 Dec 2016, Paul Sbarra wrote:
> Sadly, I haven't been able to figure out how to get the mbox file from
> this tread into gmail, but wanted to report a parsing issue I've found
> with the builtin difftool.
>
> Original Patch:
> https://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schindelin@gmx.de/#t
>
> > + *status = *++p;
> > + if (!status || p[1])
> > + return error("unexpected trailer: '%s'", p);
> > + return 0;
>
> The p[1] null check assumes the status is only one character long, but
> git-diff's raw output format shows that a numeric value can follow in
> the copy-edit and rename-edit cases.
Thank you for the report! I fixed it locally.
> I'm looking forward to seeing the builtin difftool land. I came across it
> while investigating adding --submodule=diff (expanding on diff's
> recent addition) support and this looks more promising then the perl
> script. Hopefully I will make some progress. Any tips/pointers would
> be greatly appreciated.
I would have expected `git difftool --submodule=diff ...` to work... What
are the problems?
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 21/21] Documentation/git-update-index: explain splitIndex.*
From: Christian Couder @ 2017-01-02 16:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqq8tr19ocs.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 27, 2016 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> --split-index::
>> --no-split-index::
>> - Enable or disable split index mode. If enabled, the index is
>> - split into two files, $GIT_DIR/index and $GIT_DIR/sharedindex.<SHA-1>.
>> - Changes are accumulated in $GIT_DIR/index while the shared
>> - index file contains all index entries stays unchanged. If
>> - split-index mode is already enabled and `--split-index` is
>> - given again, all changes in $GIT_DIR/index are pushed back to
>> - the shared index file. This mode is designed for very large
>> - indexes that take a significant amount of time to read or write.
>> + Enable or disable split index mode. If split-index mode is
>> + already enabled and `--split-index` is given again, all
>> + changes in $GIT_DIR/index are pushed back to the shared index
>> + file.
>
> In the world after this series that introduced the percentage-based
> auto consolidation, it smells strange, or even illogical, that index
> is un-split after doing this:
>
> $ git update-index --split-index
> $ git update-index --split-index
>
> Before this series, it may have been a quick and dirty way to force
> consolidating the split index without totally disabling the feature,
> i.e. it would have looked more like
>
> $ git update-index --split-index
> ... work work work to accumulate more modifications
> ... consolidate into one --- there was no other way than
> ... disabling it temporarily
> $ git update-index --no-split-index
> ... but the user likes the feature so re-enable it.
> $ git update-index --split-index
>
> which I guess is where this strange behaviour comes from.
>
> It may be something we need to fix to unconfuse the end-users after
> this series lands. Even though "first disable and then re-enable"
> takes two commands (as opposed to one), it is far more logical. And
> the percentage-based auto consolidation feature is meant to reduce
> the need for manual consolidation, so it probably makes sense to
> remove this illogical feature.
Yeah, I tend to agree that this feature could be removed later. Though
before removing it, I'd like to hear Duy's opinion on this as he
created the feature in the first place.
>> @@ -394,6 +390,31 @@ Although this bit looks similar to assume-unchanged bit, its goal is
>> different from assume-unchanged bit's. Skip-worktree also takes
>> precedence over assume-unchanged bit when both are set.
>>
>> +Split index
>> +-----------
>> +
>> +This mode is designed for very large indexes that take a significant
>> +amount of time to read or write.
>
> This is not a new problem, but it probably is incorrect to say "to
> read or write". It saves time by not rewriting the whole thing but
> instead write out only the updated bits. You'd still read the whole
> thing while populating the in-core index from the disk, and if
> anything, you'd probably spend _more_ cycles because you'd essentially
> be reading the base and then reading the delta to apply on top.
Ok, then what about:
+This mode is designed for repositories with very large indexes, and aims
+at reducing the time it takes to repeatedly write these indexes.
>> +To avoid deleting a shared index file that is still used, its mtime is
>> +updated to the current time everytime a new split index based on the
>> +shared index file is either created or read from.
>
> The same comment on the mention of "mtime" in another patch applies
> here as well.
Ok, I will use "modification time" instead of "mtime".
Thanks.
^ permalink raw reply
* [PATCH 2/2] giteveryday: unbreak rendering with AsciiDoctor
From: Johannes Schindelin @ 2017-01-02 16:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1483373021.git.johannes.schindelin@gmx.de>
The "giteveryday" document has a callout list that contains a code
block. This is not a problem for AsciiDoc, but AsciiDoctor sadly was
explicitly designed *not* to render this correctly [*1*]. The symptom is
an unhelpful
line 322: callout list item index: expected 1 got 12
line 325: no callouts refer to list item 1
line 325: callout list item index: expected 2 got 13
line 327: no callouts refer to list item 2
In Git for Windows, we rely on the speed improvement of AsciiDoctor (on
this developer's machine, `make -j15 html` takes roughly 30 seconds with
AsciiDoctor, 70 seconds with AsciiDoc), therefore we need a way to
render this correctly.
The easiest way out is to simplify the callout list, as suggested by
AsciiDoctor's author, even while one may very well disagree with him
that a code block hath no place in a callout list.
*1*: https://github.com/asciidoctor/asciidoctor/issues/1478
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/giteveryday.txt | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/Documentation/giteveryday.txt b/Documentation/giteveryday.txt
index 35473ad02f..10c8ff93c0 100644
--- a/Documentation/giteveryday.txt
+++ b/Documentation/giteveryday.txt
@@ -307,9 +307,16 @@ master or exposed as a part of a stable branch.
<9> backport a critical fix.
<10> create a signed tag.
<11> make sure master was not accidentally rewound beyond that
-already pushed out. `ko` shorthand points at the Git maintainer's
+already pushed out.
+<12> In the output from `git show-branch`, `master` should have
+everything `ko/master` has, and `next` should have
+everything `ko/next` has, etc.
+<13> push out the bleeding edge, together with new tags that point
+into the pushed history.
+
+In this example, the `ko` shorthand points at the Git maintainer's
repository at kernel.org, and looks like this:
-+
+
------------
(in .git/config)
[remote "ko"]
@@ -320,12 +327,6 @@ repository at kernel.org, and looks like this:
push = +refs/heads/pu
push = refs/heads/maint
------------
-+
-<12> In the output from `git show-branch`, `master` should have
-everything `ko/master` has, and `next` should have
-everything `ko/next` has, etc.
-<13> push out the bleeding edge, together with new tags that point
-into the pushed history.
Repository Administration[[ADMINISTRATION]]
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`
From: Johannes Schindelin @ 2017-01-02 16:03 UTC (permalink / raw)
To: git; +Cc: 마누엘, Junio C Hamano
In-Reply-To: <cover.1483373021.git.johannes.schindelin@gmx.de>
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= <nalla@hamal.uberspace.de>
The `user-manual.txt` is designed as a `book` but the `Makefile` wants
to build it as an `article`. This seems to be a problem when building
the documentation with `asciidoctor`. Furthermore the parts *Git
Glossary* and *Appendix B* had no subsections which is not allowed when
building with `asciidoctor`. So lets add a *dummy* section.
Signed-off-by: 마누엘 <nalla@hamal.uberspace.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/Makefile | 2 +-
Documentation/user-manual.txt | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index b43d66eae6..a9fb497b83 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -337,7 +337,7 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
user-manual.xml: user-manual.txt user-manual.conf
$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \
- $(TXT_TO_XML) -d article -o $@+ $< && \
+ $(TXT_TO_XML) -d book -o $@+ $< && \
mv $@+ $@
technical/api-index.txt: technical/api-index-skel.txt \
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 5e07454572..bc29298678 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -4395,6 +4395,10 @@ itself!
Git Glossary
============
+[[git-explained]]
+Git explained
+-------------
+
include::glossary-content.txt[]
[[git-quick-start]]
@@ -4636,6 +4640,10 @@ $ git gc
Appendix B: Notes and todo list for this manual
===============================================
+[[todo-list]]
+Todo list
+---------
+
This is a work in progress.
The basic requirements:
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH 0/2] Fix the documentation to build with asciidoctor
From: Johannes Schindelin @ 2017-01-02 16:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]
The first patch in this series has been with Git for Windows for well
over a year already, the second one is a replacement for such an old
patch.
The reason why we use asciidoctor in Git for Windows is easy to see:
when building new Git for Windows packages, rendering the documentation
dominates the time by far. It takes roughly 75 seconds to compile all
the executables from scratch on my main work machine, but it takes
roughly 125 seconds to build the documentation from scratch.
Out of those 125 seconds, 45 are taken by the HTML documentation.
In the Git for Windows project, we realized a long time ago that we
could reduce the time dramatically by using asciidoctor: it takes only
15 seconds to build the HTML documentation.
Those savings come at a cost, though: asciidoctor was designed to be
much less flexible than asciidoc, in favor for maximum speed and minimal
size of the code base. And to accomodate those shortcomings, it is
unfortunately necessary to change Git's documentation sources.
So what is the big deal with those timings? It's only half a minute!
This is on a very beefy machine. Internally, I use quite a bit of
Continuous Integration to build intermediate Git for Windows versions
for testing, and those builds are backed by reasonably-sized VMs. That
makes it worth shaving that extra time off.
Side note for the curious who wonder why asciidoctor is *so much* faster
than asciidoc: my gut feeling is that the primary reason for this is,
once again, the POSIX emulation layer: asciidoc runs as a Python script,
using a Python interpreter that uses the MSYS2 runtime for path
translation and fork emulation (among other things), while asciidoctor
runs as a Ruby script, using a pure Windows Ruby interpreter (for those
remembering the nomenclature: the Python interpreter is an MSYS2 program
while the Ruby interpreter is a MINGW program). But even after that I
suspect that asciidoc was just not designed for speed, on a very
fundamental level.
Now back to the patch series: I *hope* the patches are not too
disruptive. I am very open to changing them however needed, I only care
about one result in the end: that the documentation builds correctly
(and fast) with asciidoctor.
Johannes Schindelin (1):
giteveryday: unbreak rendering with AsciiDoctor
마누엘 (1):
asciidoctor: fix user-manual to be built by `asciidoctor`
Documentation/Makefile | 2 +-
Documentation/giteveryday.txt | 17 +++++++++--------
Documentation/user-manual.txt | 8 ++++++++
3 files changed, 18 insertions(+), 9 deletions(-)
base-commit: e05806da9ec4aff8adfed142ab2a2b3b02e33c8c
Published-As: https://github.com/dscho/git/releases/tag/asciidoctor-fixes-v1
Fetch-It-Via: git fetch https://github.com/dscho/git asciidoctor-fixes-v1
--
2.11.0.rc3.windows.1
^ permalink raw reply
* Re: [PATCH 00/17] object_id part 6
From: Michael Haggerty @ 2017-01-02 15:37 UTC (permalink / raw)
To: brian m. carlson, git; +Cc: Jeff King
In-Reply-To: <20170101191847.564741-1-sandals@crustytoothpaste.net>
On 01/01/2017 08:18 PM, brian m. carlson wrote:
> This is another series in the continuing conversion to struct object_id.
>
> This series converts more of the builtin directory and some of the
> refs code to use struct object_id. Additionally, it implements an
> nth_packed_object_oid function which provides a struct object_id
> version of the nth_packed_object function.
>
> There is a small known conflict with next, but it can easily be fixed up.
I read through all of the patches, and sent a few comments. I didn't
notice any other problems.
Michael
^ permalink raw reply
* [PATCH v3 38/38] sequencer (rebase -i): write out the final message
From: Johannes Schindelin @ 2017-01-02 15:36 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer,
Jeff King
In-Reply-To: <cover.1483370556.git.johannes.schindelin@gmx.de>
The shell script version of the interactive rebase has a very specific
final message. Teach the sequencer to print the same.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index b39cd21e03..0e7d2ca5c8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2127,6 +2127,9 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
}
apply_autostash(opts);
+ fprintf(stderr, "Successfully rebased and updated %s.\n",
+ head_ref.buf);
+
strbuf_release(&buf);
strbuf_release(&head_ref);
}
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v3 37/38] sequencer (rebase -i): write the progress into files
From: Johannes Schindelin @ 2017-01-02 15:36 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer,
Jeff King
In-Reply-To: <cover.1483370556.git.johannes.schindelin@gmx.de>
For the benefit of e.g. the shell prompt, the interactive rebase not
only displays the progress for the user to see, but also writes it into
the msgnum/end files in the state directory.
Teach the sequencer this new trick.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 2c9c555ab6..b39cd21e03 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,6 +47,16 @@ static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
*/
static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
/*
+ * The file to keep track of how many commands were already processed (e.g.
+ * for the prompt).
+ */
+static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
+/*
+ * The file to keep track of how many commands are to be processed in total
+ * (e.g. for the prompt).
+ */
+static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
+/*
* The commit message that is planned to be used for any changes that
* need to be committed following a user interaction.
*/
@@ -1353,6 +1363,7 @@ static int read_populate_todo(struct todo_list *todo_list,
if (is_rebase_i(opts)) {
struct todo_list done = TODO_LIST_INIT;
+ FILE *f = fopen(rebase_path_msgtotal(), "w");
if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
!parse_insn_buffer(done.buf.buf, &done))
@@ -1362,8 +1373,12 @@ static int read_populate_todo(struct todo_list *todo_list,
todo_list->total_nr = todo_list->done_nr
+ count_commands(todo_list);
-
todo_list_release(&done);
+
+ if (f) {
+ fprintf(f, "%d\n", todo_list->total_nr);
+ fclose(f);
+ }
}
return 0;
@@ -1947,11 +1962,20 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
if (save_todo(todo_list, opts))
return -1;
if (is_rebase_i(opts)) {
- if (item->command != TODO_COMMENT)
+ if (item->command != TODO_COMMENT) {
+ FILE *f = fopen(rebase_path_msgnum(), "w");
+
+ todo_list->done_nr++;
+
+ if (f) {
+ fprintf(f, "%d\n", todo_list->done_nr);
+ fclose(f);
+ }
fprintf(stderr, "Rebasing (%d/%d)%s",
- ++(todo_list->done_nr),
+ todo_list->done_nr,
todo_list->total_nr,
opts->verbose ? "\n" : "\r");
+ }
unlink(rebase_path_message());
unlink(rebase_path_author_script());
unlink(rebase_path_stopped_sha());
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v3 36/38] sequencer (rebase -i): show the progress
From: Johannes Schindelin @ 2017-01-02 15:35 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer,
Jeff King
In-Reply-To: <cover.1483370556.git.johannes.schindelin@gmx.de>
The interactive rebase keeps the user informed about its progress.
If the sequencer wants to do the grunt work of the interactive
rebase, it also needs to show that progress.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 4792a3de3b..2c9c555ab6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1181,6 +1181,7 @@ struct todo_list {
struct strbuf buf;
struct todo_item *items;
int nr, alloc, current;
+ int done_nr, total_nr;
};
#define TODO_LIST_INIT { STRBUF_INIT }
@@ -1297,6 +1298,17 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
return res;
}
+static int count_commands(struct todo_list *todo_list)
+{
+ int count = 0, i;
+
+ for (i = 0; i < todo_list->nr; i++)
+ if (todo_list->items[i].command != TODO_COMMENT)
+ count++;
+
+ return count;
+}
+
static int read_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
{
@@ -1339,6 +1351,21 @@ static int read_populate_todo(struct todo_list *todo_list,
return error(_("cannot revert during a cherry-pick."));
}
+ if (is_rebase_i(opts)) {
+ struct todo_list done = TODO_LIST_INIT;
+
+ if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
+ !parse_insn_buffer(done.buf.buf, &done))
+ todo_list->done_nr = count_commands(&done);
+ else
+ todo_list->done_nr = 0;
+
+ todo_list->total_nr = todo_list->done_nr
+ + count_commands(todo_list);
+
+ todo_list_release(&done);
+ }
+
return 0;
}
@@ -1920,6 +1947,11 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
if (save_todo(todo_list, opts))
return -1;
if (is_rebase_i(opts)) {
+ if (item->command != TODO_COMMENT)
+ fprintf(stderr, "Rebasing (%d/%d)%s",
+ ++(todo_list->done_nr),
+ todo_list->total_nr,
+ opts->verbose ? "\n" : "\r");
unlink(rebase_path_message());
unlink(rebase_path_author_script());
unlink(rebase_path_stopped_sha());
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v3 35/38] sequencer (rebase -i): suggest --edit-todo upon unknown command
From: Johannes Schindelin @ 2017-01-02 15:35 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer,
Jeff King
In-Reply-To: <cover.1483370556.git.johannes.schindelin@gmx.de>
This is the same behavior as known from `git rebase -i`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 4f37ba8d33..4792a3de3b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1314,8 +1314,12 @@ static int read_populate_todo(struct todo_list *todo_list,
close(fd);
res = parse_insn_buffer(todo_list->buf.buf, todo_list);
- if (res)
+ if (res) {
+ if (is_rebase_i(opts))
+ return error(_("please fix this using "
+ "'git rebase --edit-todo'."));
return error(_("unusable instruction sheet: '%s'"), todo_file);
+ }
if (!todo_list->nr &&
(!is_rebase_i(opts) || !file_exists(rebase_path_done())))
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH v3 34/38] sequencer (rebase -i): show only failed cherry-picks' output
From: Johannes Schindelin @ 2017-01-02 15:35 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer,
Jeff King
In-Reply-To: <cover.1483370556.git.johannes.schindelin@gmx.de>
This is the behavior of the shell script version of the interactive
rebase, by using the `output` function defined in `git-rebase.sh`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index a501dfce38..4f37ba8d33 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -433,6 +433,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
o.ancestor = base ? base_label : "(empty tree)";
o.branch1 = "HEAD";
o.branch2 = next ? next_label : "(empty tree)";
+ if (is_rebase_i(opts))
+ o.buffer_output = 2;
head_tree = parse_tree_indirect(head);
next_tree = next ? next->tree : empty_tree();
@@ -444,6 +446,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
clean = merge_trees(&o,
head_tree,
next_tree, base_tree, &result);
+ if (is_rebase_i(opts) && clean <= 0)
+ fputs(o.obuf.buf, stdout);
strbuf_release(&o.obuf);
if (clean < 0)
return clean;
--
2.11.0.rc3.windows.1
^ 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