* Re: [PATCH 6/9] rebase -i: check for missing commits in the rebase--helper
From: Dennis Kaarsemaker @ 2016-09-02 20:59 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano
In-Reply-To: <6d28acb59d7c217f1f8d6d364e5a52c8bde35085.1472833365.git.johannes.schindelin@gmx.de>
On vr, 2016-09-02 at 18:23 +0200, Johannes Schindelin wrote:
> In particular on Windows, where shell scripts are even more expensive
> than on MacOSX or Linux, it makes sense to move a loop that forks
> Git at least once for every line in the todo list into a builtin.
Heh, this was the one thing that made me hesitate sending the
suggestion about rebase-helper --edit-todo, but with this bit already
moved, I think rebase-helper --edit-todo makes even more sense to do.
D.
^ permalink raw reply
* Re: [PATCH 4/9] rebase -i: also expand/collapse the SHA-1s via the rebase--helper
From: Dennis Kaarsemaker @ 2016-09-02 20:56 UTC (permalink / raw)
To: Johannes Schindelin, git; +Cc: Junio C Hamano
In-Reply-To: <fe53c109c757ede9cd1b0d2944d2b5e16575c49b.1472833365.git.johannes.schindelin@gmx.de>
On vr, 2016-09-02 at 18:23 +0200, Johannes Schindelin wrote:
> This is crucial to improve performance on Windows, as the speed is now
> mostly dominated by the SHA-1 transformation (because it spawns a new
> rev-parse process for *every* line, and spawning processes is pretty
> slow from Git for Windows' MSYS2 Bash).
I see these functions only used as part of an shorten-edit-expand
sequence. Why not do a git rebase-helper --edit-todo instead? Saves
another few process spawnings.
Something for yet another later followup patch?
D.
^ permalink raw reply
* Re: [PATCH 2/2] connect: know that zero-ID is not a ref
From: Jeff King @ 2016-09-02 20:13 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com>
On Fri, Sep 02, 2016 at 10:15:39AM -0700, Jonathan Tan wrote:
> connect.c, when processing packfiles, treats a zero ID (with
> `capabilities^{}` in place of the refname) as an actual ref instead of a
> placeholder for a capability declaration, contrary to the specification
> in Reference Discovery in Documentation/technical/pack-protocol.txt.
> This is an issue when interacting with Git implementations that follow
> this specification. For example, `ls-remote` (against a git://
> repository served by such a Git implementation) will report a ref when
> there are none, and `clone` (against something similar) will fail its
> checkout.
>
> Make connect.c follow the specification with respect to this, while
> maintaining compatibility with existing implementations that do not
> serve the zero ID when a repository has no refs.
Hmm. So since this is backwards-compatible, I'm not overly concerned
with changing the client. But I wonder if you considered that the
documentation is wrong, and that JGit should stop sending the extra
capabilities line?
In either case, there will still be breakage until _somebody_ upgrades
(with your patch, until clients upgrade; with a JGit patch, until the
server upgrades). So perhaps an interesting question would be: if we
were writing this now, what is the correct behavior?
For pushing, it is obviously useful to send capabilities even though
there are no refs (because the client is going to send _you_ something).
And that is why "capabilities^{}" exists; it is a receive-pack feature
(that is actually implemented!), and the documentation (which came after
the implementation) blindly mentioned it for upload-pack, as well.
Is it useful for upload-pack? If we have no refs, there's traditionally
been nothing to fetch. Perhaps that's something that could change,
though. For example, there could be a capability to allow fetching
arbitrary sha1s (we have allowTIPSH1InWant and allowReachableSHA1InWant,
which obviously both require some refs, but allowArbitrarySHA1 does not
seem outside the realm of possibility).
I'll review the rest assuming that this is the direction we want to
go...
> (git-daemon should probably also be changed to serve zero IDs, but such
> a change can be considered independently from this change; even if both
> the client and server changes were made in one commit, it is nearly
> impossible that all Git installations are updated at the same time - an
> updated client would still need to deal with unupdated servers and vice
> versa.)
I'm really not sure what you mean here. How does git-daemon enter into
this at all?
> + if (is_null_oid(&old_oid)) {
> + if (strcmp(name, "capabilities^{}"))
> + warning("zero object ID received that is not accompanied by a "
> + "capability declaration, ignoring and continuing anyway");
> + continue;
> + }
I agree with Shawn that this should be looking for "capabilities^{}" as
the name of the first entry (and the warning can go away; if we see any
other null sha1, it just gets handled in the usual error code paths).
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 819b9dd..c6f8b6f 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
> test_cmp expect actual
> '
>
> +test_lazy_prereq GIT_DAEMON '
> + test_have_prereq JGIT &&
> + test_tristate GIT_TEST_GIT_DAEMON &&
> + test "$GIT_TEST_GIT_DAEMON" != false
> +'
GIT_DAEMON depends on JGIT? Should this really be the JGIT_DAEMON
prerequisite?
> +# This test spawns a daemon, so run it only if the user would be OK with
> +# testing with git-daemon.
> +test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
> + JGIT_DAEMON_PID= &&
> + git init --bare empty.git &&
> + touch empty.git/git-daemon-export-ok &&
> + {
> + jgit daemon --port="$JGIT_DAEMON_PORT" . &
> + JGIT_DAEMON_PID=$!
> + } &&
> + test_when_finished kill "$JGIT_DAEMON_PID" &&
> + sleep 1 && # allow jgit daemon some time to set up
We definitely need something more robust than this "sleep". This test is
going to fail racily when the system is under load.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] connect: know that zero-ID is not a ref
From: Shawn Pearce @ 2016-09-02 20:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jonathan Tan, git
In-Reply-To: <CAGZ79kYWr09K2RXgSmmy4G=to65LbLfCi=GRkykhciq_vsyMZQ@mail.gmail.com>
On Fri, Sep 2, 2016 at 12:56 PM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce <spearce@spearce.org> wrote:
>> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>>
>>> + if (is_null_oid(&old_oid)) {
>>> + if (strcmp(name, "capabilities^{}"))
>>
>> Its not the zero ID that is special, its the "capabilities^{}" name
>> that is special when its the first entry in the stream. In the wire
>> protocol a "x^{}" line is a modifier to a prior "x" line to add a
>> peeled object to the prior line. But if we see "^{}" on the first line
>> that is non-sense, there is no prior line to modify with this
>> identifier.
>>
>> Further ^{} is used here because its invalid in a name. A server
>> really cannot have a reference that ends with the sequence ^{}. And a
>> server should not have a reference named "capabilities" without a
>> "refs/" prefix on it.
>>
>> So the entire "capabilities^{}" on the first line is a bunch of
>> contradictions that violate a number of things about the protocol,
>> which is why clients should ignore it.
>>
>> I think the test should be about:
>>
>> !*list && !strcmp(name, "capabilities^{}")
>>
>>> + warning("zero object ID received that is not accompanied by a "
>>> + "capability declaration, ignoring and continuing anyway");
>>
>> Annoyingly a zero object ID is sort of possible; with a probability of
>> 1/2^160 or something. Its just a very very unlikely value. Slightly
>> stronger to test against the known invalid name.
>
> That ship has sailed long ago I would think?
> There are tests for null sha1 in the refs code, e.g. for
> deletion/creation of a branch
> we consider the null sha1 as the counterpart.
>
> So while it may be possible to have SHA1 producing a "0"x40, you
> cannot e.g. push it?
Indeed, you are correct.
I'm just stating the JGit client behavior is to look at
"capabilities^{}" in the first line as special, not the SHA-1.
^ permalink raw reply
* [PATCH] sequencer: support folding in rfc2822 footer
From: Jonathan Tan @ 2016-09-02 19:58 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
When running `git cherry-pick -x`, a blank line would be inserted if a line in
the footer was broken into multiple lines (which is inconsistent with its
behavior if no lines were broken). For example, this command would produce:
---
Sample-field: no blank line below
(cherry picked from commit ...)
---
but would also produce:
---
Sample-field: multiple-line field body
that causes a blank line below
(cherry picked from commit ...)
---
This, among other things, trips up tools that look for the last paragraph
(including sequencer.c itself).
RFC 2822 allows field bodies to be split into multiple lines, especially (but
not only) to deal with the line-width limitations described in the
specification, referring to this as "folding".
Teach sequencer.c to treat split and unsplit field bodies in the same way (that
is, to not include the blank line).
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
sequencer.c | 19 +++++++++++++++++--
t/t3511-cherry-pick-x.sh | 19 +++++++++++++++++++
2 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 3804fa9..411dd50 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -26,10 +26,20 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
-static int is_rfc2822_line(const char *buf, int len)
+static int is_rfc2822_line(const char *buf, int len,
+ int allow_folding_continuation)
{
int i;
+ /*
+ * Section 2.2.3 of RFC 2822 allows field bodies to continue onto
+ * multiple lines, referred to as "folding". Such continuation lines
+ * start with whitespace.
+ */
+ if (allow_folding_continuation)
+ if (len && (buf[0] == ' ' || buf[0] == '\t'))
+ return 1;
+
for (i = 0; i < len; i++) {
int ch = buf[i];
if (ch == ':')
@@ -64,6 +74,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
int len = sb->len - ignore_footer;
const char *buf = sb->buf;
int found_sob = 0;
+ int allow_folding_continuation = 0;
/* footer must end with newline */
if (!len || buf[len - 1] != '\n')
@@ -92,7 +103,11 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
; /* do nothing */
k++;
- found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
+ found_rfc2822 = is_rfc2822_line(buf + i,
+ k - i - 1,
+ allow_folding_continuation);
+ allow_folding_continuation = 1;
+
if (found_rfc2822 && sob &&
!strncmp(buf + i, sob->buf, sob->len))
found_sob = k;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..1a50339 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -36,6 +36,11 @@ mesg_with_cherry_footer="$mesg_with_footer_sob
(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
Tested-by: C.U. Thor <cuthor@example.com>"
+mesg_with_folding_footer="$mesg_no_footer
+
+Field: This is a very long field body
+ that is continued onto another line"
+
mesg_unclean="$mesg_one_line
@@ -68,6 +73,8 @@ test_expect_success setup '
git reset --hard initial &&
test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
git reset --hard initial &&
+ test_commit "$mesg_with_folding_footer" foo b mesg-with-folding-footer &&
+ git reset --hard initial &&
test_config commit.cleanup verbatim &&
test_commit "$mesg_unclean" foo b mesg-unclean &&
test_unconfig commit.cleanup &&
@@ -234,6 +241,18 @@ test_expect_success 'cherry-pick -x -s treats "(cherry picked from..." line as p
test_cmp expect actual
'
+test_expect_success 'cherry-pick -x does not insert blank line when folding footer is found' '
+ pristine_detach initial &&
+ sha1=$(git rev-parse mesg-with-folding-footer^0) &&
+ git cherry-pick -x mesg-with-folding-footer &&
+ cat <<-EOF >expect &&
+ $mesg_with_folding_footer
+ (cherry picked from commit $sha1)
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'cherry-pick preserves commit message' '
pristine_detach initial &&
printf "$mesg_unclean" >expect &&
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH 2/2] connect: know that zero-ID is not a ref
From: Stefan Beller @ 2016-09-02 19:56 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Jonathan Tan, git
In-Reply-To: <CAJo=hJssMohNq5zDU=wdb1gP4nW59m8pUmxP7eY4jnjr502z0w@mail.gmail.com>
On Fri, Sep 2, 2016 at 12:39 PM, Shawn Pearce <spearce@spearce.org> wrote:
> On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>>
>> + if (is_null_oid(&old_oid)) {
>> + if (strcmp(name, "capabilities^{}"))
>
> Its not the zero ID that is special, its the "capabilities^{}" name
> that is special when its the first entry in the stream. In the wire
> protocol a "x^{}" line is a modifier to a prior "x" line to add a
> peeled object to the prior line. But if we see "^{}" on the first line
> that is non-sense, there is no prior line to modify with this
> identifier.
>
> Further ^{} is used here because its invalid in a name. A server
> really cannot have a reference that ends with the sequence ^{}. And a
> server should not have a reference named "capabilities" without a
> "refs/" prefix on it.
>
> So the entire "capabilities^{}" on the first line is a bunch of
> contradictions that violate a number of things about the protocol,
> which is why clients should ignore it.
>
> I think the test should be about:
>
> !*list && !strcmp(name, "capabilities^{}")
>
>> + warning("zero object ID received that is not accompanied by a "
>> + "capability declaration, ignoring and continuing anyway");
>
> Annoyingly a zero object ID is sort of possible; with a probability of
> 1/2^160 or something. Its just a very very unlikely value. Slightly
> stronger to test against the known invalid name.
That ship has sailed long ago I would think?
There are tests for null sha1 in the refs code, e.g. for
deletion/creation of a branch
we consider the null sha1 as the counterpart.
So while it may be possible to have SHA1 producing a "0"x40, you
cannot e.g. push it?
^ permalink raw reply
* Re: git add -p—splitting hunks, limit is too large
From: Christian Neukirchen @ 2016-09-02 19:49 UTC (permalink / raw)
To: git
In-Reply-To: <20160902191425.ki7nfhlqgnihoqpw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Splitting to single lines means you need to remember to add the matched
> pairs, which might be arbitrarily far apart. That's not really any
> different than dumping the hunk in your editor, but I find there that
> it's easy to rearrange and group things as appropriate.
My main use case for this would be to split a plain addition into
several small additions. Which would be much easier with the
menu-driven approach.
(Mostly I just use magit, but sometimes I want to do this on machines
without emacs set up.)
--
Christian Neukirchen <chneukirchen@gmail.com> http://chneukirchen.org
^ permalink raw reply
* Re: [PATCH 2/2] connect: know that zero-ID is not a ref
From: Shawn Pearce @ 2016-09-02 19:39 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com>
On Fri, Sep 2, 2016 at 10:15 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>
> + if (is_null_oid(&old_oid)) {
> + if (strcmp(name, "capabilities^{}"))
Its not the zero ID that is special, its the "capabilities^{}" name
that is special when its the first entry in the stream. In the wire
protocol a "x^{}" line is a modifier to a prior "x" line to add a
peeled object to the prior line. But if we see "^{}" on the first line
that is non-sense, there is no prior line to modify with this
identifier.
Further ^{} is used here because its invalid in a name. A server
really cannot have a reference that ends with the sequence ^{}. And a
server should not have a reference named "capabilities" without a
"refs/" prefix on it.
So the entire "capabilities^{}" on the first line is a bunch of
contradictions that violate a number of things about the protocol,
which is why clients should ignore it.
I think the test should be about:
!*list && !strcmp(name, "capabilities^{}")
> + warning("zero object ID received that is not accompanied by a "
> + "capability declaration, ignoring and continuing anyway");
Annoyingly a zero object ID is sort of possible; with a probability of
1/2^160 or something. Its just a very very unlikely value. Slightly
stronger to test against the known invalid name.
^ permalink raw reply
* Re: [PATCH 2/2] connect: know that zero-ID is not a ref
From: Jonathan Nieder @ 2016-09-02 19:37 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <2bea354c6218a33db3972e42baa75676fdcbc598.1472836026.git.jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
> connect.c, when processing packfiles, treats a zero ID (with
> `capabilities^{}` in place of the refname) as an actual ref instead of a
> placeholder for a capability declaration, contrary to the specification
> in Reference Discovery in Documentation/technical/pack-protocol.txt.
> This is an issue when interacting with Git implementations that follow
> this specification. For example, `ls-remote` (against a git://
> repository served by such a Git implementation) will report a ref when
> there are none, and `clone` (against something similar) will fail its
> checkout.
For example, this means I get the following confusing message when
cloning an empty repository served by "jgit daemon":
$ git clone https://googlers.googlesource.com/jrn/empty
Cloning into 'empty'...
Checking connectivity... done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.
$
It also means that standard "git daemon" is not able to advertise
capabilities on a fetch from a repository without advertising some
refs, so I cannot fetch-by-sha1 from a C git server unless some refs
are advertised.
This fix should solve the former problem and paves the way for the
latter to be solved once a year or two passes and people's clients are
up to date (or earlier if a server operator chooses).
> Make connect.c follow the specification with respect to this, while
> maintaining compatibility with existing implementations that do not
> serve the zero ID when a repository has no refs.
This description focuses on the code. When deciding whether to apply
the patch (or whether to revert it if it comes up while trying to
investigate another problem), I am likely to wonder "what effect will
the patch have on me?" instead of "what standards does it follow?"
Flipping the emphasis, we could say something like
connect: treat ref advertisement with capabilities^{} line as one with no refs
When cloning an empty repository served by standard git, "git clone"
produces the following reassuring message:
$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
warning: You appear to have cloned an empty repository.
Checking connectivity... done.
Meanwhile when cloning an empty repository served by JGit, the output
is more haphazard:
$ git clone git://localhost/tmp/empty
Cloning into 'empty'...
Checking connectivity... done.
warning: remote HEAD refers to nonexistent ref, unable to checkout.
This is a common command to run immediately after creating a remote
repository as preparation for adding content to populate it and pushing.
The warning is confusing and needlessly worrying.
The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
with no refs in upload service., 2013-08-08), JGit's ref advertisement
includes a ref named capabilities^{} to advertise its refs on, while git's
ref advertisement is empty in this case. This allows the client to learn
about the server's capabilities and is need, for example, for fetch-by-sha1
to work when no refs are advertised.
Git advertises the same capabilities^{} ref in its ref advertisement for
push but since it never remembered to do so for fetch, the client forgot
to handle this case. Handle it.
So now you can see the same friendly message whether the server runs C
git or JGit.
The pack-protocol.txt documentation gives some guidance about the expected
server behavior: what JGit does is currently required, since a list-of-refs
is required to be non-empty.
advertised-refs = (no-refs / list-of-refs)
*shallow
flush-pkt
no-refs = PKT-LINE(zero-id SP "capabilities^{}"
NUL capability-list)
list-of-refs = first-ref *other-ref
Because git client versions without this fix are expected to exist in the
wild for a while, we should not change the server to always send the
capabilities^{} line when there are no refs to advertise yet. A transition
will take multiple steps:
1. This patch, which updates the client
2. Update pack-protocol to clarify that both server behaviors must be
tolerated.
3. Add a configuration variable to allow git upload-pack to advertise
capabilities when there are no refs to advertise. Leave it disabled
by default since git clients can't be counted on to have this patch (1)
yet.
4. After a year or so, flip the default for that server configuration
variable to true.
[...]
> --- a/connect.c
> +++ b/connect.c
> @@ -165,6 +165,13 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> continue;
> }
>
> + if (is_null_oid(&old_oid)) {
> + if (strcmp(name, "capabilities^{}"))
> + warning("zero object ID received that is not accompanied by a "
> + "capability declaration, ignoring and continuing anyway");
> + continue;
> + }
I'd rather this be stricter. Though it's not your fault: the code
before it is very lax in letting each line specify capabilities again.
Could this behave more like the ".have" case? That is, just
if (!strcmp(name, "capabilities^{}"))
continue;
Or if we want to sanity-check the line further,
if (!strcmp(name, "capabilities^{}")) {
if (len == name_len + GIT_SHA1_HEX_SZ + 1)
die("protocol error: expected capabilities after nul, got none");
if (!is_null_oid(&old_oid))
die("protocol error: expected zero-id, got %s", oid_to_hex(&old_oid));
continue;
}
The protocol definition and other clients already handle capabilities^{} differently
from other cases of zero-id.
[...]
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
[...]
> +test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
> + JGIT_DAEMON_PID= &&
> + git init --bare empty.git &&
> + touch empty.git/git-daemon-export-ok &&
> + {
> + jgit daemon --port="$JGIT_DAEMON_PORT" . &
> + JGIT_DAEMON_PID=$!
> + } &&
> + test_when_finished kill "$JGIT_DAEMON_PID" &&
> + sleep 1 && # allow jgit daemon some time to set up
Can this sleep be avoided? E.g. start_git_daemon in lib-git-daemon.sh
uses output from the daemon to tell when it is serving.
Thanks for writing this. I'll be happy when this protocol blip is gone.
Sincerely,
Jonathan
^ permalink raw reply
* Fixup of a fixup not working right
From: Robert Dailey @ 2016-09-02 19:24 UTC (permalink / raw)
To: Git
Suppose I have a branch with 4 commits, in the following order (as you
might see during interactive rebase):
pick 123 Original Change
pick 789 fixup! Original Change
pick 456 Some Other Thing
pick abc fixup! fixup! Original Change
However, let's say the first commit is already pushed upstream on a
topic branch. Since there are multiple developers on this topic
branch, I do not want to rebase right now. Instead, I want to document
future fixups via fixup commits and then when we're ready to merge, do
a final rebase prior to the merge to master to clean things up after
we're all done collaborating.
For this specific situation, since the first commit is already pushed,
I want to perform a fixup on the 1st fixup commit. When I perform an
interactive rebase against upstream topic, I get the following:
pick 789 fixup! Original Change
pick 456 Some Other Thing
pick abc fixup! fixup! Original Change
The tip commit (abc in this case) is not marked as a fixup. What I
expect to see is:
pick 789 fixup! Original Change
fixup abc fixup! fixup! Original Change
pick 456 Some Other Thing
Is this by design, or a defect? I assumed that Git would only look at
the first occurrence of "fixup!" and treat everything else after as
the commit description to match. But it seems in this case that it
stops at the last occurrence of "fixup!", which would explain why it
isn't matching in the interactive rebase. I haven't looked at the
code, though.
Thoughts? Also I'm perfectly willing to accept feedback involving me
just using the feature wrong or as not intended. Thanks in advance.
^ permalink raw reply
* If a branch moves a submodule, "merge --ff[-only]" succeeds while "merge --no-ff" fails with conflicts
From: Dakota Hawkins @ 2016-09-02 19:22 UTC (permalink / raw)
To: Git Mailing List, mwitte
Below is a simple reproduction of the issue.
The _real_ problem is that this is how our pull request merges work,
they're not allowed to do fast-forward merges. To work around this we
are having to split this up into two pull requests/merges: One that
copies the submodules to the new location and includes any fixes
required to support the move, and a second that removes the old
locations.
## Setup steps
git clone https://github.com/dakotahawkins/submodule-move-merge-bug-main-repo.git
cd submodule-move-merge-bug-main-repo
## How it was initially constructed
# git submodule add ../submodule-move-merge-bug-submodule-repo.git
./submodule-location-1
# git commit -m "Added submodule in its initial location"
# git push
# git checkout -b move-submodule
# git mv ./submodule-location-1 ./submodule-location-2
# git commit -m "Moved submodule"
# git push --set-upstream origin move-submodule
git branch move-submodule origin/move-submodule
## Test fast-forward merge, this will work
git checkout -b merge-ff-test master # warning: unable to rmdir
submodule-location-2: Directory not empty
rm -rf ./submodule-location-2
git merge --ff-only move-submodule
## Test no-fast-forward merge, this will fail with conflicts:
git checkout -b merge-no-ff-test master
git merge --no-ff move-submodule
# Auto-merging submodule-location-2
# Adding as submodule-location-2~move-submodule instead
# Automatic merge failed; fix conflicts and then commit the result.
git status
# On branch merge-no-ff-test
# You have unmerged paths.
# (fix conflicts and run "git commit")
# (use "git merge --abort" to abort the merge)
#
# Changes to be committed:
#
# modified: .gitmodules
# deleted: submodule-location-1
#
# Unmerged paths:
# (use "git add <file>..." to mark resolution)
#
# added by us: submodule-location-2
#
# fatal: Not a git repository: 'submodule-location-1/.git'
# Submodule changes to be committed:
#
# * submodule-location-1 07fec24...0000000:
^ permalink raw reply
* Re: git add -p—splitting hunks, limit is too large
From: Jeff King @ 2016-09-02 19:14 UTC (permalink / raw)
To: Beau Martinez; +Cc: git
In-Reply-To: <CAEtDOuV+0CKRSu9mJa27+yQKJ-QRyNmwecrZKaaCh0St+VagZg@mail.gmail.com>
On Fri, Sep 02, 2016 at 03:36:58PM +0100, Beau Martinez wrote:
> I'd like to inquire as to why `git add -p` can only split hunks so
> much. The limit is too large; why can't you split until each hunk is
> only a line? I often have to run `edit` and split them manually
> myself.
There's some previous discussion in this thread:
http://public-inbox.org/git/200805232221.45406.trast@student.ethz.ch/t/#u
and further back, this message:
http://public-inbox.org/git/7vbq8v7cdx.fsf@gitster.siamese.dyndns.org/
I think one problem is that in a given contiguous hunk, not all of the
lines are independent, because edits are represented as a pair of -/+
lines. E.g., if the preimage is:
one
two
four
and the postimage is:
one
two modified
three
four
your diff will be:
one
-two
+two modified
+three
four
The ideal split is two groups:
-two
+two modified
+three
So you could possibly achieve that by specifying the exact line to split
at. But let's imagine "two" was the missing item, and we modified
"three". Then your diff is:
one
-three
+two
+three modified
four
Now the related lines are non-adjacent! I don't think there's a general
solution, and of course it can get arbitrarily complicated, with many
interleaved pairs. I don't think we can rely on figuring out which lines
form a pair. In this toy example it's obvious, but in real diffs the
lines might not bear any resemblance.
Splitting to single lines means you need to remember to add the matched
pairs, which might be arbitrarily far apart. That's not really any
different than dumping the hunk in your editor, but I find there that
it's easy to rearrange and group things as appropriate.
> I'd like to contribute a patch to change it, although my C is rusty.
> Are there resources that will help me to do this?
The good news (or maybe the bad) is that "add -p" is implemented
entirely in Perl. :) It's in git-add--interactive.perl.
-Peff
^ permalink raw reply
* [PATCH 1/2] tests: move test_lazy_prereq JGIT to test-lib.sh
From: Jonathan Tan @ 2016-09-02 17:15 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1472836026.git.jonathantanmy@google.com>
This enables JGIT to be used as a prereq in invocations of
test_expect_success (and other functions) in other test scripts.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5310-pack-bitmaps.sh | 4 ----
t/test-lib.sh | 4 ++++
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..1e376ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -158,10 +158,6 @@ test_expect_success 'pack with missing parent' '
git pack-objects --stdout --revs <revs >/dev/null
'
-test_lazy_prereq JGIT '
- type jgit
-'
-
test_expect_success JGIT 'we can read jgit bitmaps' '
git clone . compat-jgit &&
(
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..c9c1037 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1072,6 +1072,10 @@ test_lazy_prereq NOT_ROOT '
test "$uid" != 0
'
+test_lazy_prereq JGIT '
+ type jgit
+'
+
# SANITY is about "can you correctly predict what the filesystem would
# do by only looking at the permission bits of the files and
# directories?" A typical example of !SANITY is running the test
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 2/2] connect: know that zero-ID is not a ref
From: Jonathan Tan @ 2016-09-02 17:15 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1472836026.git.jonathantanmy@google.com>
connect.c, when processing packfiles, treats a zero ID (with
`capabilities^{}` in place of the refname) as an actual ref instead of a
placeholder for a capability declaration, contrary to the specification
in Reference Discovery in Documentation/technical/pack-protocol.txt.
This is an issue when interacting with Git implementations that follow
this specification. For example, `ls-remote` (against a git://
repository served by such a Git implementation) will report a ref when
there are none, and `clone` (against something similar) will fail its
checkout.
Make connect.c follow the specification with respect to this, while
maintaining compatibility with existing implementations that do not
serve the zero ID when a repository has no refs.
(git-daemon should probably also be changed to serve zero IDs, but such
a change can be considered independently from this change; even if both
the client and server changes were made in one commit, it is nearly
impossible that all Git installations are updated at the same time - an
updated client would still need to deal with unupdated servers and vice
versa.)
The test uses JGit's daemon feature, which is specification-compliant.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
connect.c | 7 +++++++
t/t5512-ls-remote.sh | 22 ++++++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/connect.c b/connect.c
index 722dc3f..d4a58de 100644
--- a/connect.c
+++ b/connect.c
@@ -165,6 +165,13 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
continue;
}
+ if (is_null_oid(&old_oid)) {
+ if (strcmp(name, "capabilities^{}"))
+ warning("zero object ID received that is not accompanied by a "
+ "capability declaration, ignoring and continuing anyway");
+ continue;
+ }
+
if (!check_ref(name, flags))
continue;
ref = alloc_ref(buffer + GIT_SHA1_HEXSZ + 1);
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 819b9dd..c6f8b6f 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -207,5 +207,27 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '
test_cmp expect actual
'
+test_lazy_prereq GIT_DAEMON '
+ test_have_prereq JGIT &&
+ test_tristate GIT_TEST_GIT_DAEMON &&
+ test "$GIT_TEST_GIT_DAEMON" != false
+'
+
+JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}}
+
+# This test spawns a daemon, so run it only if the user would be OK with
+# testing with git-daemon.
+test_expect_success JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
+ JGIT_DAEMON_PID= &&
+ git init --bare empty.git &&
+ touch empty.git/git-daemon-export-ok &&
+ {
+ jgit daemon --port="$JGIT_DAEMON_PORT" . &
+ JGIT_DAEMON_PID=$!
+ } &&
+ test_when_finished kill "$JGIT_DAEMON_PID" &&
+ sleep 1 && # allow jgit daemon some time to set up
+ test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
+'
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 0/2] handle empty spec-compliant remote repos correctly
From: Jonathan Tan @ 2016-09-02 17:15 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
This issue was noticed when trying to clone an empty repository served by a
server that uses the JGit library.
Reference Discovery in Documentation/technical/pack-protocol.txt dictates that
servers should send a zero ID when there are no refs in the queried repository,
and implementations like JGit do, but the C client does not handle them
correctly (treating them as an actual ref and subsequently returning incorrect
responses or errors).
These patches fix those while maintaining backwards compatibility with existing
implementations that do not send the zero ID in such a case.
Jonathan Tan (2):
tests: move test_lazy_prereq JGIT to test-lib.sh
connect: know that zero-ID is not a ref
connect.c | 7 +++++++
t/t5310-pack-bitmaps.sh | 4 ----
t/t5512-ls-remote.sh | 22 ++++++++++++++++++++++
t/test-lib.sh | 4 ++++
4 files changed, 33 insertions(+), 4 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Dennis Kaarsemaker @ 2016-09-02 17:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609021557060.129229@virtualbox>
On vr, 2016-09-02 at 16:22 +0200, Johannes Schindelin wrote:
> I hope this clarifies why I am not so concerned about some issues
> such as translation, or commit messages, or grammar, and more so
> about others, such as incorrect code.
It does, thanks!
D.
^ permalink raw reply
* Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests
From: Matthieu Moy @ 2016-09-02 16:36 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Ævar Arnfjörð Bjarmason, Jeff King, Git,
Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1609021406340.129229@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Ævar,
>
> On Fri, 2 Sep 2016, Ævar Arnfjörð Bjarmason wrote:
>
>> On Wed, Aug 31, 2016 at 5:05 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > The biggest problem with Strawberry Perl is that it is virtually
>> > impossible to build the Subversion-Perl bindings using the Git for
>> > Windows SDK when using Strawberry Perl.
>> >
>> > Which pretty much precludes it from being used in Git for Windows.
>> >
>> > And then there are the path issues... Git's Perl scripts are pretty
>> > certain that they live in a POSIX-y environment. Which MSYS2 Perl
>> > provides. Strawberry Perl not.
>>
>> This might be me missing the point, and I'm really just trying to be
>> helpful here and make "prove" work for you because it's awesome, but
>> as far as just you running this for development purposes does any of
>> this SVN stuff matter? I.e. you can build Git itself not with
>> Strawberry, but just use Strawberry to get a working copy of "prove".
>
> Yes, the SVN stuff matters, because of the many t9*svn* tests (which, BTW
> take a substantial time to run). So if I run the test suite, I better do
> it with a perl.exe in the PATH that can run the SVN tests. Otherwise I
> might just as well not bother with running the entire test suite...
Maybe something like
\path\to\strawberry-perl\perl.exe \path\to\prove ...
without changing the PATH would work. I wouldn't call that convenient
though.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* [PATCH 9/9] rebase -i: rearrange fixup/squash lines using the rebase--helper
From: Johannes Schindelin @ 2016-09-02 16:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472833365.git.johannes.schindelin@gmx.de>
This operation has quadratic complexity, which is especially painful
on Windows, where shell scripts are *already* slow (mainly due to the
overhead of the POSIX emulation layer).
Let's reimplement this with linear complexity (using a hash map to
match the commits' subject lines) for the common case; Sadly, the
fixup/squash feature's design neglected performance considerations,
allowing arbitrary prefixes (read: `fixup! hell` will match the
commit subject `hello world`), which means that we are stuck with
quadratic performance in the worst case.
The reimplemented logic also happens to fix a bug where commented-out
lines (representing empty patches) were dropped by the previous code.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rebase--helper.c | 6 +-
git-rebase--interactive.sh | 90 +-------------------
sequencer.c | 197 +++++++++++++++++++++++++++++++++++++++++++
sequencer.h | 1 +
t/t3415-rebase-autosquash.sh | 2 +-
5 files changed, 205 insertions(+), 91 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index de3ccd9..e6591f0 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
- CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS
+ CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -33,6 +33,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
N_("check the todo list"), CHECK_TODO_LIST),
OPT_CMDMODE(0, "skip-unnecessary-picks", &command,
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
+ OPT_CMDMODE(0, "rearrange-squash", &command,
+ N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_END()
};
@@ -59,5 +61,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
return !!skip_unnecessary_picks();
+ if (command == REARRANGE_SQUASH && argc == 1)
+ return !!rearrange_squash();
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a34ebdc..68a6e6a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -734,94 +734,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-sha1s
}
-# Rearrange the todo list that has both "pick sha1 msg" and
-# "pick sha1 fixup!/squash! msg" appears in it so that the latter
-# comes immediately after the former, and change "pick" to
-# "fixup"/"squash".
-#
-# Note that if the config has specified a custom instruction format
-# each log message will be re-retrieved in order to normalize the
-# autosquash arrangement
-rearrange_squash () {
- format=$(git config --get rebase.instructionFormat)
- # extract fixup!/squash! lines and resolve any referenced sha1's
- while read -r pick sha1 message
- do
- test -z "${format}" || message=$(git log -n 1 --format="%s" ${sha1})
- case "$message" in
- "squash! "*|"fixup! "*)
- action="${message%%!*}"
- rest=$message
- prefix=
- # skip all squash! or fixup! (but save for later)
- while :
- do
- case "$rest" in
- "squash! "*|"fixup! "*)
- prefix="$prefix${rest%%!*},"
- rest="${rest#*! }"
- ;;
- *)
- break
- ;;
- esac
- done
- printf '%s %s %s %s\n' "$sha1" "$action" "$prefix" "$rest"
- # if it's a single word, try to resolve to a full sha1 and
- # emit a second copy. This allows us to match on both message
- # and on sha1 prefix
- if test "${rest#* }" = "$rest"; then
- fullsha="$(git rev-parse -q --verify "$rest" 2>/dev/null)"
- if test -n "$fullsha"; then
- # prefix the action to uniquely identify this line as
- # intended for full sha1 match
- echo "$sha1 +$action $prefix $fullsha"
- fi
- fi
- esac
- done >"$1.sq" <"$1"
- test -s "$1.sq" || return
-
- used=
- while read -r pick sha1 message
- do
- case " $used" in
- *" $sha1 "*) continue ;;
- esac
- printf '%s\n' "$pick $sha1 $message"
- test -z "${format}" || message=$(git log -n 1 --format="%s" ${sha1})
- used="$used$sha1 "
- while read -r squash action msg_prefix msg_content
- do
- case " $used" in
- *" $squash "*) continue ;;
- esac
- emit=0
- case "$action" in
- +*)
- action="${action#+}"
- # full sha1 prefix test
- case "$msg_content" in "$sha1"*) emit=1;; esac ;;
- *)
- # message prefix test
- case "$message" in "$msg_content"*) emit=1;; esac ;;
- esac
- if test $emit = 1; then
- if test -n "${format}"
- then
- msg_content=$(git log -n 1 --format="${format}" ${squash})
- else
- msg_content="$(echo "$msg_prefix" | sed "s/,/! /g")$msg_content"
- fi
- printf '%s\n' "$action $squash $msg_content"
- used="$used$squash "
- fi
- done <"$1.sq"
- done >"$1.rearranged" <"$1"
- cat "$1.rearranged" >"$1"
- rm -f "$1.sq" "$1.rearranged"
-}
-
# Add commands after a pick or after a squash/fixup serie
# in the todo list.
add_exec_commands () {
@@ -1084,7 +996,7 @@ then
fi
test -s "$todo" || echo noop >> "$todo"
-test -n "$autosquash" && rearrange_squash "$todo"
+test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
test -n "$cmd" && add_exec_commands "$todo"
todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
diff --git a/sequencer.c b/sequencer.c
index 6cc94c9..4b50a4c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "log-tree.h"
#include "wt-status.h"
+#include "hashmap.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -2664,3 +2665,199 @@ int skip_unnecessary_picks(void)
return 0;
}
+
+struct subject2item_entry {
+ struct hashmap_entry entry;
+ int i;
+ char subject[FLEX_ARRAY];
+};
+
+static int subject2item_cmp(const struct subject2item_entry *a,
+ const struct subject2item_entry *b, const void *key)
+{
+ return key ? strcmp(a->subject, key) : strcmp(a->subject, b->subject);
+}
+
+/*
+ * Rearrange the todo list that has both "pick sha1 msg" and "pick sha1
+ * fixup!/squash! msg" in it so that the latter is put immediately after the
+ * former, and change "pick" to "fixup"/"squash".
+ *
+ * Note that if the config has specified a custom instruction format, each log
+ * message will have to be retrieved from the commit (as the oneline in the
+ * script cannot be trusted) in order to normalize the autosquash arrangement.
+ */
+int rearrange_squash(void)
+{
+ const char *todo_file = rebase_path_todo();
+ struct todo_list todo_list = TODO_LIST_INIT;
+ struct hashmap subject2item;
+ int res = 0, rearranged = 0, *next, *tail, fd, i;
+ char **subjects;
+
+ fd = open(todo_file, O_RDONLY);
+ if (fd < 0)
+ return error_errno(_("Could not open %s"), todo_file);
+ if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
+ close(fd);
+ return error(_("Could not read %s."), todo_file);
+ }
+ close(fd);
+ if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
+ todo_list_release(&todo_list);
+ return -1;
+ }
+
+ /*
+ * The hashmap maps onelines to the respective todo list index.
+ *
+ * If any items need to be rearranged, the next[i] value will indicate
+ * which item was moved directly after the i'th.
+ *
+ * In that case, last[i] will indicate the index of the latest item to
+ * be moved to appear after the i'th.
+ */
+ hashmap_init(&subject2item, (hashmap_cmp_fn) subject2item_cmp,
+ todo_list.nr);
+ ALLOC_ARRAY(next, todo_list.nr);
+ ALLOC_ARRAY(tail, todo_list.nr);
+ ALLOC_ARRAY(subjects, todo_list.nr);
+ for (i = 0; i < todo_list.nr; i++) {
+ struct strbuf buf = STRBUF_INIT;
+ struct todo_item *item = todo_list.items + i;
+ const char *commit_buffer, *subject, *p;
+ int i2 = -1;
+ struct subject2item_entry *entry;
+
+ next[i] = tail[i] = -1;
+ if (item->command >= TODO_EXEC) {
+ subjects[i] = NULL;
+ continue;
+ }
+
+ if (is_fixup(item->command)) {
+ todo_list_release(&todo_list);
+ return error(_("The script was already rearranged."));
+ }
+
+ item->commit->util = item;
+
+ parse_commit(item->commit);
+ commit_buffer = get_commit_buffer(item->commit, NULL);
+ find_commit_subject(commit_buffer, &subject);
+ format_subject(&buf, subject, " ");
+ subject = subjects[i] = buf.buf;
+ unuse_commit_buffer(item->commit, commit_buffer);
+ if ((skip_prefix(subject, "fixup! ", &p) ||
+ skip_prefix(subject, "squash! ", &p))) {
+ struct commit *commit2;
+
+ for (;;) {
+ while (isspace(*p))
+ p++;
+ if (!skip_prefix(p, "fixup! ", &p) &&
+ !skip_prefix(p, "squash! ", &p))
+ break;
+ }
+
+ if ((entry = hashmap_get_from_hash(&subject2item,
+ strhash(p), p)))
+ /* found by title */
+ i2 = entry->i;
+ else if (!strchr(p, ' ') &&
+ (commit2 =
+ lookup_commit_reference_by_name(p)) &&
+ commit2->util)
+ /* found by commit name */
+ i2 = (struct todo_item *)commit2->util
+ - todo_list.items;
+ else {
+ /* copy can be a prefix of the commit subject */
+ for (i2 = 0; i2 < i; i2++)
+ if (subjects[i2] &&
+ starts_with(subjects[i2], p))
+ break;
+ if (i2 == i)
+ i2 = -1;
+ }
+ }
+ if (i2 >= 0) {
+ rearranged = 1;
+ todo_list.items[i].command =
+ starts_with(subject, "fixup!") ?
+ TODO_FIXUP : TODO_SQUASH;
+ if (next[i2] < 0)
+ next[i2] = i;
+ else
+ next[tail[i2]] = i;
+ tail[i2] = i;
+ }
+ else if (!hashmap_get_from_hash(&subject2item,
+ strhash(subject), subject)) {
+ FLEX_ALLOC_MEM(entry, subject, buf.buf, buf.len);
+ entry->i = i;
+ hashmap_entry_init(entry, strhash(entry->subject));
+ hashmap_put(&subject2item, entry);
+ }
+ strbuf_detach(&buf, NULL);
+ }
+
+ if (rearranged) {
+ struct strbuf buf = STRBUF_INIT;
+ char *format = NULL;
+
+ git_config_get_string("rebase.instructionFormat", &format);
+ for (i = 0; i < todo_list.nr; i++) {
+ enum todo_command command = todo_list.items[i].command;
+ int cur = i;
+
+ /*
+ * Initially, all commands are 'pick's. If it is a
+ * fixup or a squash now, we have rearranged it.
+ */
+ if (is_fixup(command))
+ continue;
+
+ while (cur >= 0) {
+ int offset = todo_list.items[cur].offset_in_buf;
+ int end_offset = cur + 1 < todo_list.nr ?
+ todo_list.items[cur + 1].offset_in_buf :
+ todo_list.buf.len;
+ char *bol = todo_list.buf.buf + offset;
+ char *eol = todo_list.buf.buf + end_offset;
+
+ /* replace 'pick', by 'fixup' or 'squash' */
+ command = todo_list.items[cur].command;
+ if (is_fixup(command)) {
+ strbuf_addstr(&buf,
+ todo_command_info[command].str);
+ bol += strcspn(bol, " \t");
+ }
+
+ strbuf_add(&buf, bol, eol - bol);
+
+ cur = next[cur];
+ }
+ }
+
+ fd = open(todo_file, O_WRONLY);
+ if (fd < 0)
+ res |= error_errno(_("Could not open %s"), todo_file);
+ else if (write(fd, buf.buf, buf.len) < 0)
+ res |= error_errno(_("Could not read %s."), todo_file);
+ else if (ftruncate(fd, buf.len) < 0)
+ res |= error_errno(_("Could not finish %s"), todo_file);
+ close(fd);
+ strbuf_release(&buf);
+ }
+
+ free(next);
+ free(tail);
+ for (i = 0; i < todo_list.nr; i++)
+ free(subjects[i]);
+ free(subjects);
+ hashmap_free(&subject2item, 1);
+ todo_list_release(&todo_list);
+
+ return res;
+}
diff --git a/sequencer.h b/sequencer.h
index 4da215c..a3aa3cb 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -64,6 +64,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
int transform_todo_ids(int shorten_sha1s);
int check_todo_list(void);
int skip_unnecessary_picks(void);
+int rearrange_squash(void);
extern const char sign_off_header[];
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 9fd629a..b9e2600 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -278,7 +278,7 @@ set_backup_editor () {
test_set_editor "$PWD/backup-editor.sh"
}
-test_expect_failure 'autosquash with multiple empty patches' '
+test_expect_success 'autosquash with multiple empty patches' '
test_tick &&
git commit --allow-empty -m "empty" &&
test_tick &&
--
2.9.3.windows.3
^ permalink raw reply related
* [PATCH 7/9] rebase -i: skip unnecessary picks using the rebase--helper
From: Johannes Schindelin @ 2016-09-02 16:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472833365.git.johannes.schindelin@gmx.de>
In particular on Windows, where shell scripts are even more expensive
than on MacOSX or Linux, it makes sense to move a loop that forks
Git at least once for every line in the todo list into a builtin.
Note: The original code did not try to skip unnecessary picks of root
commits but punts instead (probably --root was not considered common
enough of a use case to bother optimizing). We do the same, for now.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rebase--helper.c | 6 +++-
git-rebase--interactive.sh | 41 ++-------------------
sequencer.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++
sequencer.h | 1 +
4 files changed, 99 insertions(+), 39 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index e706eac..de3ccd9 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
- CHECK_TODO_LIST
+ CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -31,6 +31,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
OPT_CMDMODE(0, "check-todo-list", &command,
N_("check the todo list"), CHECK_TODO_LIST),
+ OPT_CMDMODE(0, "skip-unnecessary-picks", &command,
+ N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_END()
};
@@ -55,5 +57,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!transform_todo_ids(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
+ if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
+ return !!skip_unnecessary_picks();
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 02a7698..a34ebdc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -703,43 +703,6 @@ do_rest () {
done
}
-# skip picking commits whose parents are unchanged
-skip_unnecessary_picks () {
- fd=3
- while read -r command rest
- do
- # fd=3 means we skip the command
- case "$fd,$command" in
- 3,pick|3,p)
- # pick a commit whose parent is current $onto -> skip
- sha1=${rest%% *}
- case "$(git rev-parse --verify --quiet "$sha1"^)" in
- "$onto"*)
- onto=$sha1
- ;;
- *)
- fd=1
- ;;
- esac
- ;;
- 3,"$comment_char"*|3,)
- # copy comments
- ;;
- *)
- fd=1
- ;;
- esac
- printf '%s\n' "$command${rest:+ }$rest" >&$fd
- done <"$todo" >"$todo.new" 3>>"$done" &&
- mv -f "$todo".new "$todo" &&
- case "$(peek_next_command)" in
- squash|s|fixup|f)
- record_in_rewritten "$onto"
- ;;
- esac ||
- die "$(gettext "Could not skip unnecessary pick commands")"
-}
-
transform_todo_ids () {
while read -r command rest
do
@@ -1165,7 +1128,9 @@ git rebase--helper --check-todo-list || {
expand_todo_ids
-test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
+test -d "$rewritten" || test -n "$force_rebase" ||
+onto="$(git rebase--helper --skip-unnecessary-picks)" ||
+die "Could not skip unnecessary pick commands"
checkout_onto
if test -z "$rebase_root" && test ! -d "$rewritten"
diff --git a/sequencer.c b/sequencer.c
index 0c82925..6cc94c9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2574,3 +2574,93 @@ int check_todo_list(void)
return res;
}
+
+/* skip picking commits whose parents are unchanged */
+int skip_unnecessary_picks(void)
+{
+ const char *todo_file = rebase_path_todo();
+ struct strbuf buf = STRBUF_INIT;
+ struct todo_list todo_list = TODO_LIST_INIT;
+ struct object_id onto_oid, *oid = &onto_oid, *parent_oid;
+ int fd, i;
+
+ if (!read_oneliner(&buf, rebase_path_onto(), 0))
+ return error("Could not read 'onto'");
+ if (get_sha1(buf.buf, onto_oid.hash)) {
+ strbuf_release(&buf);
+ return error("Need a HEAD to fixup");
+ }
+ strbuf_release(&buf);
+
+ fd = open(todo_file, O_RDONLY);
+ if (fd < 0) {
+ return error_errno(_("Could not open '%s'"), todo_file);
+ }
+ if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
+ close(fd);
+ return error(_("Could not read '%s'."), todo_file);
+ }
+ close(fd);
+ if (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {
+ todo_list_release(&todo_list);
+ return -1;
+ }
+
+ for (i = 0; i < todo_list.nr; i++) {
+ struct todo_item *item = todo_list.items + i;
+
+ if (item->command >= TODO_NOOP)
+ continue;
+ if (item->command != TODO_PICK)
+ break;
+ if (parse_commit(item->commit)) {
+ todo_list_release(&todo_list);
+ return error(_("Could not parse commit '%s'"),
+ oid_to_hex(&item->commit->object.oid));
+ }
+ if (!item->commit->parents)
+ break; /* root commit */
+ if (item->commit->parents->next)
+ break; /* merge commit */
+ parent_oid = &item->commit->parents->item->object.oid;
+ if (hashcmp(parent_oid->hash, oid->hash))
+ break;
+ oid = &item->commit->object.oid;
+ }
+ if (i > 0) {
+ int offset = i < todo_list.nr ?
+ todo_list.items[i].offset_in_buf : todo_list.buf.len;
+ const char *done_path = rebase_path_done();
+
+ fd = open(done_path, O_CREAT | O_WRONLY | O_APPEND, 0666);
+ if (write_in_full(fd, todo_list.buf.buf, offset) < 0) {
+ todo_list_release(&todo_list);
+ return error_errno(_("Could not write to '%s'"),
+ done_path);
+ }
+ close(fd);
+
+ fd = open(rebase_path_todo(), O_WRONLY, 0666);
+ if (write_in_full(fd, todo_list.buf.buf + offset,
+ todo_list.buf.len - offset) < 0) {
+ todo_list_release(&todo_list);
+ return error_errno(_("Could not write to '%s'"),
+ rebase_path_todo());
+ }
+ if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
+ todo_list_release(&todo_list);
+ return error_errno(_("Could not truncate '%s'"),
+ rebase_path_todo());
+ }
+ close(fd);
+
+ todo_list.current = i;
+ if (is_fixup(peek_command(&todo_list, 0)))
+ record_in_rewritten(oid, peek_command(&todo_list, 0));
+ }
+
+ todo_list_release(&todo_list);
+ printf("%s\n", oid_to_hex(oid));
+
+ return 0;
+}
diff --git a/sequencer.h b/sequencer.h
index 8e3daf9..4da215c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -63,6 +63,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
int transform_todo_ids(int shorten_sha1s);
int check_todo_list(void);
+int skip_unnecessary_picks(void);
extern const char sign_off_header[];
--
2.9.3.windows.3
^ permalink raw reply related
* [PATCH 8/9] t3415: test fixup with wrapped oneline
From: Johannes Schindelin @ 2016-09-02 16:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472833365.git.johannes.schindelin@gmx.de>
The `git commit --fixup` command unwraps wrapped onelines when
constructing the commit message, without wrapping the result.
We need to make sure that `git rebase --autosquash` keeps handling such
cases correctly, in particular since we are about to move the autosquash
handling into the rebase--helper.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t3415-rebase-autosquash.sh | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 48346f1..9fd629a 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -304,4 +304,18 @@ test_expect_success 'extra spaces after fixup!' '
test $base = $parent
'
+test_expect_success 'wrapped original subject' '
+ if test -d .git/rebase-merge; then git rebase --abort; fi &&
+ base=$(git rev-parse HEAD) &&
+ echo "wrapped subject" >wrapped &&
+ git add wrapped &&
+ test_tick &&
+ git commit --allow-empty -m "$(printf "To\nfixup")" &&
+ test_tick &&
+ git commit --allow-empty -m "fixup! To fixup" &&
+ git rebase -i --autosquash --keep-empty HEAD~2 &&
+ parent=$(git rev-parse HEAD^) &&
+ test $base = $parent
+'
+
test_done
--
2.9.3.windows.3
^ permalink raw reply related
* [PATCH 6/9] rebase -i: check for missing commits in the rebase--helper
From: Johannes Schindelin @ 2016-09-02 16:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472833365.git.johannes.schindelin@gmx.de>
In particular on Windows, where shell scripts are even more expensive
than on MacOSX or Linux, it makes sense to move a loop that forks
Git at least once for every line in the todo list into a builtin.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rebase--helper.c | 7 +-
git-rebase--interactive.sh | 164 ++-------------------------------------------
sequencer.c | 124 ++++++++++++++++++++++++++++++++++
sequencer.h | 1 +
4 files changed, 136 insertions(+), 160 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 9444c8d..e706eac 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,7 +13,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
- CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
+ CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+ CHECK_TODO_LIST
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -28,6 +29,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
OPT_CMDMODE(0, "expand-sha1s", &command,
N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+ OPT_CMDMODE(0, "check-todo-list", &command,
+ N_("check the todo list"), CHECK_TODO_LIST),
OPT_END()
};
@@ -50,5 +53,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!transform_todo_ids(1);
if (command == EXPAND_SHA1S && argc == 1)
return !!transform_todo_ids(0);
+ if (command == CHECK_TODO_LIST && argc == 1)
+ return !!check_todo_list();
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f642ec2..02a7698 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -880,96 +880,6 @@ add_exec_commands () {
mv "$1.new" "$1"
}
-# Check if the SHA-1 passed as an argument is a
-# correct one, if not then print $2 in "$todo".badsha
-# $1: the SHA-1 to test
-# $2: the line number of the input
-# $3: the input filename
-check_commit_sha () {
- badsha=0
- if test -z "$1"
- then
- badsha=1
- else
- sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
- if test -z "$sha1_verif"
- then
- badsha=1
- fi
- fi
-
- if test $badsha -ne 0
- then
- line="$(sed -n -e "${2}p" "$3")"
- warn "$(eval_gettext "\
-Warning: the SHA-1 is missing or isn't a commit in the following line:
- - \$line")"
- warn
- fi
-
- return $badsha
-}
-
-# prints the bad commits and bad commands
-# from the todolist in stdin
-check_bad_cmd_and_sha () {
- retval=0
- lineno=0
- while read -r command rest
- do
- lineno=$(( $lineno + 1 ))
- case $command in
- "$comment_char"*|''|noop|x|exec)
- # Doesn't expect a SHA-1
- ;;
- "$cr")
- # Work around CR left by "read" (e.g. with Git for
- # Windows' Bash).
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha "${rest%%[ ]*}" "$lineno" "$1"
- then
- retval=1
- fi
- ;;
- *)
- line="$(sed -n -e "${lineno}p" "$1")"
- warn "$(eval_gettext "\
-Warning: the command isn't recognized in the following line:
- - \$line")"
- warn
- retval=1
- ;;
- esac
- done <"$1"
- return $retval
-}
-
-# Print the list of the SHA-1 of the commits
-# from stdin to stdout
-todo_list_to_sha_list () {
- git stripspace --strip-comments |
- while read -r command sha1 rest
- do
- case $command in
- "$comment_char"*|''|noop|x|"exec")
- ;;
- *)
- long_sha=$(git rev-list --no-walk "$sha1" 2>/dev/null)
- printf "%s\n" "$long_sha"
- ;;
- esac
- done
-}
-
-# Use warn for each line in stdin
-warn_lines () {
- while read -r line
- do
- warn " - $line"
- done
-}
-
# Switch to the branch in $into and notify it in the reflog
checkout_onto () {
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
@@ -984,74 +894,6 @@ get_missing_commit_check_level () {
printf '%s' "$check_level" | tr 'A-Z' 'a-z'
}
-# Check if the user dropped some commits by mistake
-# Behaviour determined by rebase.missingCommitsCheck.
-# Check if there is an unrecognized command or a
-# bad SHA-1 in a command.
-check_todo_list () {
- raise_error=f
-
- check_level=$(get_missing_commit_check_level)
-
- case "$check_level" in
- warn|error)
- # Get the SHA-1 of the commits
- todo_list_to_sha_list <"$todo".backup >"$todo".oldsha1
- todo_list_to_sha_list <"$todo" >"$todo".newsha1
-
- # Sort the SHA-1 and compare them
- sort -u "$todo".oldsha1 >"$todo".oldsha1+
- mv "$todo".oldsha1+ "$todo".oldsha1
- sort -u "$todo".newsha1 >"$todo".newsha1+
- mv "$todo".newsha1+ "$todo".newsha1
- comm -2 -3 "$todo".oldsha1 "$todo".newsha1 >"$todo".miss
-
- # Warn about missing commits
- if test -s "$todo".miss
- then
- test "$check_level" = error && raise_error=t
-
- warn "$(gettext "\
-Warning: some commits may have been dropped accidentally.
-Dropped commits (newer to older):")"
-
- # Make the list user-friendly and display
- opt="--no-walk=sorted --format=oneline --abbrev-commit --stdin"
- git rev-list $opt <"$todo".miss | warn_lines
-
- warn "$(gettext "\
-To avoid this message, use \"drop\" to explicitly remove a commit.
-
-Use 'git config rebase.missingCommitsCheck' to change the level of warnings.
-The possible behaviours are: ignore, warn, error.")"
- warn
- fi
- ;;
- ignore)
- ;;
- *)
- warn "$(eval_gettext "Unrecognized setting \$check_level for option rebase.missingCommitsCheck. Ignoring.")"
- ;;
- esac
-
- if ! check_bad_cmd_and_sha "$todo"
- then
- raise_error=t
- fi
-
- if test $raise_error = t
- then
- # Checkout before the first commit of the
- # rebase: this way git rebase --continue
- # will work correctly as it expects HEAD to be
- # placed before the commit of the next action
- checkout_onto
-
- warn "$(gettext "You can fix this with 'git rebase --edit-todo'.")"
- die "$(gettext "Or you can abort the rebase with 'git rebase --abort'.")"
- fi
-}
-
# The whole contents of this file is run by dot-sourcing it from
# inside a shell function. It used to be that "return"s we see
# below were not inside any function, and expected to return
@@ -1315,7 +1157,11 @@ git_sequence_editor "$todo" ||
has_action "$todo" ||
return 2
-check_todo_list
+git rebase--helper --check-todo-list || {
+ ret=$?
+ checkout_onto
+ exit $ret
+}
expand_todo_ids
diff --git a/sequencer.c b/sequencer.c
index ee4fdb0..0c82925 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2450,3 +2450,127 @@ int transform_todo_ids(int shorten_sha1s)
todo_list_release(&todo_list);
return 0;
}
+
+enum check_level {
+ CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
+};
+
+static enum check_level get_missing_commit_check_level(void)
+{
+ const char *value;
+
+ if (git_config_get_value("rebase.missingcommitscheck", &value) ||
+ !strcasecmp("ignore", value))
+ return CHECK_IGNORE;
+ if (!strcasecmp("warn", value))
+ return CHECK_WARN;
+ if (!strcasecmp("error", value))
+ return CHECK_ERROR;
+ warning(_("Unrecognized setting $check_level for option"
+ "rebase.missingCommitsCheck. Ignoring."));
+ return CHECK_IGNORE;
+}
+
+/*
+ * Check if the user dropped some commits by mistake
+ * Behaviour determined by rebase.missingCommitsCheck.
+ * Check if there is an unrecognized command or a
+ * bad SHA-1 in a command.
+ */
+int check_todo_list(void)
+{
+ enum check_level check_level = get_missing_commit_check_level();
+ struct strbuf todo_file = STRBUF_INIT;
+ struct todo_list todo_list = TODO_LIST_INIT;
+ struct commit_list *missing = NULL;
+ int raise_error = 0, res = 0, fd, i;
+
+ strbuf_addstr(&todo_file, rebase_path_todo());
+ fd = open(todo_file.buf, O_RDONLY);
+ if (fd < 0) {
+ res = error_errno(_("Could not open %s"), todo_file.buf);
+ goto leave_check;
+ }
+ if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
+ close(fd);
+ res = error(_("Could not read %s."), todo_file.buf);
+ goto leave_check;
+ }
+ close(fd);
+ raise_error = res =
+ parse_insn_buffer(todo_list.buf.buf, &todo_list);
+
+ if (check_level == CHECK_IGNORE)
+ goto leave_check;
+
+ /* Get the SHA-1 of the commits */
+ for (i = 0; i < todo_list.nr; i++) {
+ struct commit *commit = todo_list.items[i].commit;
+ if (commit)
+ commit->util = todo_list.items + i;
+ }
+
+ todo_list_release(&todo_list);
+ strbuf_addstr(&todo_file, ".backup");
+ fd = open(todo_file.buf, O_RDONLY);
+ if (fd < 0) {
+ res = error_errno(_("Could not open %s"), todo_file.buf);
+ goto leave_check;
+ }
+ if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
+ close(fd);
+ res = error(_("Could not read %s."), todo_file.buf);
+ goto leave_check;
+ }
+ close(fd);
+ strbuf_release(&todo_file);
+ res = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);
+
+ /* Find commits that are missing after editing */
+ for (i = 0; i < todo_list.nr; i++) {
+ struct commit *commit = todo_list.items[i].commit;
+ if (commit && !commit->util) {
+ commit_list_insert(commit, &missing);
+ commit->util = todo_list.items + i;
+ }
+ }
+
+ /* Warn about missing commits */
+ if (!missing)
+ goto leave_check;
+
+ if (check_level == CHECK_ERROR)
+ raise_error = res = 1;
+
+ fprintf(stderr,
+ _("Warning: some commits may have been dropped accidentally.\n"
+ "Dropped commits (newer to older):\n"));
+
+ /* Make the list user-friendly and display */
+ while (missing) {
+ struct commit *commit = pop_commit(&missing);
+ struct todo_item *item = commit->util;
+
+ fprintf(stderr, " - %s %.*s\n", short_commit_name(commit),
+ item->arg_len, item->arg);
+ }
+ free_commit_list(missing);
+
+ fprintf(stderr, _("To avoid this message, use \"drop\" to "
+ "explicitly remove a commit.\n\n"
+ "Use 'git config rebase.missingCommitsCheck' to change "
+ "the level of warnings.\n"
+ "The possible behaviours are: ignore, warn, error.\n\n"));
+
+leave_check:
+ strbuf_release(&todo_file);
+ todo_list_release(&todo_list);
+
+ if (raise_error)
+ fprintf(stderr,
+ _("You can fix this with 'git rebase --edit-todo'.\n"
+ "Or you can abort the rebase with 'git rebase"
+ " --abort'.\n"));
+
+ return res;
+}
diff --git a/sequencer.h b/sequencer.h
index 5feb525..8e3daf9 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -62,6 +62,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
int argc, const char **argv);
int transform_todo_ids(int shorten_sha1s);
+int check_todo_list(void);
extern const char sign_off_header[];
--
2.9.3.windows.3
^ permalink raw reply related
* [PATCH 5/9] t3404: relax rebase.missingCommitsCheck tests
From: Johannes Schindelin @ 2016-09-02 16:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472833365.git.johannes.schindelin@gmx.de>
These tests were a bit anal about the *exact* warning/error message
printed by git rebase. But those messages are intended for the *end
user*, therefore it does not make sense to test so rigidly for the
*exact* wording.
In the following, we will reimplement the missing commits check in
the sequencer, with slightly different words.
So let's just test for the parts in the warning/error message that
we *really* care about, nothing more, nothing less.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t3404-rebase-interactive.sh | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 597e94e..a18759e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1215,20 +1215,13 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
'
-cat >expect <<EOF
-Warning: the command isn't recognized in the following line:
- - badcmd $(git rev-list --oneline -1 master~1)
-
-You can fix this with 'git rebase --edit-todo'.
-Or you can abort the rebase with 'git rebase --abort'.
-EOF
-
test_expect_success 'static check of bad command' '
rebase_setup_and_clean bad-cmd &&
set_fake_editor &&
test_must_fail env FAKE_LINES="1 2 3 bad 4 5" \
git rebase -i --root 2>actual &&
- test_i18ncmp expect actual &&
+ test_i18ngrep "badcmd $(git rev-list --oneline -1 master~1)" actual &&
+ test_i18ngrep "You can fix this with .git rebase --edit-todo.." actual &&
FAKE_LINES="1 2 3 drop 4 5" git rebase --edit-todo &&
git rebase --continue &&
test E = $(git cat-file commit HEAD | sed -ne \$p) &&
@@ -1250,20 +1243,13 @@ test_expect_success 'tabs and spaces are accepted in the todolist' '
test E = $(git cat-file commit HEAD | sed -ne \$p)
'
-cat >expect <<EOF
-Warning: the SHA-1 is missing or isn't a commit in the following line:
- - edit XXXXXXX False commit
-
-You can fix this with 'git rebase --edit-todo'.
-Or you can abort the rebase with 'git rebase --abort'.
-EOF
-
test_expect_success 'static check of bad SHA-1' '
rebase_setup_and_clean bad-sha &&
set_fake_editor &&
test_must_fail env FAKE_LINES="1 2 edit fakesha 3 4 5 #" \
git rebase -i --root 2>actual &&
- test_i18ncmp expect actual &&
+ test_i18ngrep "edit XXXXXXX False commit" actual &&
+ test_i18ngrep "You can fix this with .git rebase --edit-todo.." actual &&
FAKE_LINES="1 2 4 5 6" git rebase --edit-todo &&
git rebase --continue &&
test E = $(git cat-file commit HEAD | sed -ne \$p)
--
2.9.3.windows.3
^ permalink raw reply related
* [PATCH 4/9] rebase -i: also expand/collapse the SHA-1s via the rebase--helper
From: Johannes Schindelin @ 2016-09-02 16:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472833365.git.johannes.schindelin@gmx.de>
This is crucial to improve performance on Windows, as the speed is now
mostly dominated by the SHA-1 transformation (because it spawns a new
rev-parse process for *every* line, and spawning processes is pretty
slow from Git for Windows' MSYS2 Bash).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rebase--helper.c | 10 +++++++-
git-rebase--interactive.sh | 4 ++--
sequencer.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
sequencer.h | 2 ++
4 files changed, 72 insertions(+), 3 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 821058d..9444c8d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
- CONTINUE = 1, ABORT, MAKE_SCRIPT
+ CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
ABORT),
OPT_CMDMODE(0, "make-script", &command,
N_("make rebase script"), MAKE_SCRIPT),
+ OPT_CMDMODE(0, "shorten-sha1s", &command,
+ N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+ OPT_CMDMODE(0, "expand-sha1s", &command,
+ N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
OPT_END()
};
@@ -42,5 +46,9 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!sequencer_remove_state(&opts);
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
+ if (command == SHORTEN_SHA1S && argc == 1)
+ return !!transform_todo_ids(1);
+ if (command == EXPAND_SHA1S && argc == 1)
+ return !!transform_todo_ids(0);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0eb5583..f642ec2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -764,11 +764,11 @@ transform_todo_ids () {
}
expand_todo_ids() {
- transform_todo_ids
+ git rebase--helper --expand-sha1s
}
collapse_todo_ids() {
- transform_todo_ids --short
+ git rebase--helper --shorten-sha1s
}
# Rearrange the todo list that has both "pick sha1 msg" and
diff --git a/sequencer.c b/sequencer.c
index 43e078a..ee4fdb0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2391,3 +2391,62 @@ int sequencer_make_script(int keep_empty, FILE *out,
strbuf_release(&buf);
return 0;
}
+
+
+int transform_todo_ids(int shorten_sha1s)
+{
+ const char *todo_file = rebase_path_todo();
+ struct todo_list todo_list = TODO_LIST_INIT;
+ int fd, res, i;
+ FILE *out;
+
+ strbuf_reset(&todo_list.buf);
+ fd = open(todo_file, O_RDONLY);
+ if (fd < 0)
+ return error_errno(_("Could not open '%s'"), todo_file);
+ if (strbuf_read(&todo_list.buf, fd, 0) < 0) {
+ close(fd);
+ return error(_("Could not read %s."), todo_file);
+ }
+ close(fd);
+
+ res = parse_insn_buffer(todo_list.buf.buf, &todo_list);
+ if (res) {
+ todo_list_release(&todo_list);
+ return error(_("Unusable instruction sheet: %s"), todo_file);
+ }
+
+ out = fopen(todo_file, "w");
+ if (!out) {
+ todo_list_release(&todo_list);
+ return error(_("Unable to open '%s' for writing"), todo_file);
+ }
+ for (i = 0; i < todo_list.nr; i++) {
+ struct todo_item *item = todo_list.items + i;
+ int bol = item->offset_in_buf;
+ const char *p = todo_list.buf.buf + bol;
+ int eol = i + 1 < todo_list.nr ?
+ todo_list.items[i + 1].offset_in_buf :
+ todo_list.buf.len;
+
+ if (item->command >= TODO_EXEC && item->command != TODO_DROP)
+ fwrite(p, eol - bol, 1, out);
+ else {
+ int eoc = strcspn(p, " \t");
+ const char *sha1 = shorten_sha1s ?
+ short_commit_name(item->commit) :
+ oid_to_hex(&item->commit->object.oid);
+
+ if (!eoc) {
+ p += strspn(p, " \t");
+ eoc = strcspn(p, " \t");
+ }
+
+ fprintf(out, "%.*s %s %.*s\n",
+ eoc, p, sha1, item->arg_len, item->arg);
+ }
+ }
+ fclose(out);
+ todo_list_release(&todo_list);
+ return 0;
+}
diff --git a/sequencer.h b/sequencer.h
index bc524be..5feb525 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -61,6 +61,8 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
int sequencer_make_script(int keep_empty, FILE *out,
int argc, const char **argv);
+int transform_todo_ids(int shorten_sha1s);
+
extern const char sign_off_header[];
void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
--
2.9.3.windows.3
^ permalink raw reply related
* [PATCH 3/9] rebase -i: do not invent onelines when expanding/collapsing SHA-1s
From: Johannes Schindelin @ 2016-09-02 16:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1472833365.git.johannes.schindelin@gmx.de>
To avoid problems with short SHA-1s that become non-unique during the
rebase, we rewrite the todo script with short/long SHA-1s before and
after letting the user edit the script. Since SHA-1s are not intuitive
for humans, rebase -i also provides the onelines (commit message
subjects) in the script, purely for the user's convenience.
It is very possible to generate a todo script via different means than
rebase -i and then to let rebase -i run with it; In this case, these
onelines are not required.
And this is where the expand/collapse machinery has a bug: it *expects*
that oneline, and failing to find one reuses the previous SHA-1 as
"oneline".
It was most likely an oversight, and made implementation in the (quite
limiting) shell script language less convoluted. However, we are about
to reimplement performance-critical parts in C (and due to spawning a
git.exe process for every single line of the todo script, the
expansion/collapsing of the SHA-1s *is* performance-hampering on
Windows), therefore let's fix this bug to make cross-validation with the
C version of that functionality possible.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
git-rebase--interactive.sh | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5df5850..0eb5583 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -750,7 +750,12 @@ transform_todo_ids () {
;;
*)
sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[ ]*}) &&
- rest="$sha1 ${rest#*[ ]}"
+ if test "a$rest" = "a${rest#*[ ]}"
+ then
+ rest=$sha1
+ else
+ rest="$sha1 ${rest#*[ ]}"
+ fi
;;
esac
printf '%s\n' "$command${rest:+ }$rest"
--
2.9.3.windows.3
^ permalink raw reply related
* [PATCH 0/9] The final building block for a faster rebase -i
From: Johannes Schindelin @ 2016-09-02 16:22 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
This patch series reimplements the expensive pre- and post-processing of
the todo script in C.
And it concludes the work I did to accelerate rebase -i.
Johannes Schindelin (9):
rebase -i: generate the script via rebase--helper
rebase -i: remove useless indentation
rebase -i: do not invent onelines when expanding/collapsing SHA-1s
rebase -i: also expand/collapse the SHA-1s via the rebase--helper
t3404: relax rebase.missingCommitsCheck tests
rebase -i: check for missing commits in the rebase--helper
rebase -i: skip unnecessary picks using the rebase--helper
t3415: test fixup with wrapped oneline
rebase -i: rearrange fixup/squash lines using the rebase--helper
builtin/rebase--helper.c | 29 ++-
git-rebase--interactive.sh | 362 ++++-------------------------
sequencer.c | 514 ++++++++++++++++++++++++++++++++++++++++++
sequencer.h | 7 +
t/t3404-rebase-interactive.sh | 22 +-
t/t3415-rebase-autosquash.sh | 16 +-
6 files changed, 614 insertions(+), 336 deletions(-)
Based-On: rebase--helper at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git rebase--helper
Published-As: https://github.com/dscho/git/releases/tag/rebase-i-extra-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-extra-v1
--
2.9.3.windows.3
base-commit: 4c39918f42eb8228ea4241073f86f2ac851f4636
^ permalink raw reply
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