* Re: Prove "Tests out of sequence" Error
From: Jeff King @ 2016-10-21 8:43 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021082035.xad6wfny5i6wtshh@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 04:20:35AM -0400, Jeff King wrote:
> Yes, but I do not see how it can trigger this:
>
> > > Parse errors: Tests out of sequence. Found (2) but expected (3)
> > > Tests out of sequence. Found (3) but expected (4)
> > > Tests out of sequence. Found (4) but expected (5)
> > > Bad plan. You planned 4 tests but ran 5.
>
> The TAP output from one of our tests should look something like:
>
> ok 1 - subject one
> ok 2 - subject two
> ok 3 - subject three
> ok 4 - subject four
> # passed all 4 test(s)
> 1..4
>
> the "plan" is the bit at the end. That looks like $test_count
> accidentally got incremented by one and we generated something like:
>
> ok 1 - subject one
> ok 3 - subject two
> ok 4 - subject three
> ok 5 - subject four
> 1..4
>
> which would explain the "out of sequence" errors as well as the "planned
> 4 but ran 5".
Hmm, actually the numbering problem is the other way around. It finds 2
but expects 3, so it is more like:
ok 1 - subject one
ok 2 - something else stuck in here!
ok 2 - subject two
...
which gives us a clue. And thanks to TAP auto-numbering, you can also
trigger this like:
ok 1 - subject one
ok
ok 2 - subject two
The "ok" by itself is taken to mean "ok 2". And now I have enough to
generate this locally. t5547 does:
test_commit ok &&
...
git cat-file commit $commit
which will print a line with just "ok" on it. Normally this is not sent
to stdout at all; test output goes to /dev/null unless "--verbose" is
given. When "--verbose" is used, we get all manner of random program
output intermingled with our TAP output, which is an accident waiting to
happen. Usually nobody cares, because they only use "--verbose" when
debugging a test individually (and nothing is parsing the TAP output).
But I can trigger the problem with:
prove t5547-push-quarantine.sh :: -v
The Travis tests do exactly this (along with --tee to actually save the
output). It seems like a minor miracle that this is the first test
output that has actually triggered as TAP input. I'd suggest that the
problem is not in the test, though, but that our "--verbose" option is
unsuitable for using with a TAP harness.
The obvious fix would be to send "--verbose" output to stderr, but I
suspect that would end up annoying for people who do:
./t5547-push-quarantine.sh -v | less
to read long output. Probably we need some option like "--log" which
logs in the same way that "--tee" does, but _without_ sending the data
to stdout. Naively, that just means replacing the "tee" invocation with
"cat", but I suspect it will be a lot more complicated than that,
because we still need to let the TAP output go to stdout.
-Peff
^ permalink raw reply
* Re: [BUG] [PATCH]: run-command.c
From: Jeff King @ 2016-10-21 9:00 UTC (permalink / raw)
To: git
In-Reply-To: <20161021055013.GA31554@dimstar.local.net>
On Fri, Oct 21, 2016 at 04:50:13PM +1100, Duncan Roe wrote:
> For example, if .git/config has this alias (the sleep is to leave time to
> examine output from ps, &c.):
>
> [alias]
> tryme = "!echo $PWD;sleep 600"
>
> [...]
> 16:42:06$ ps axf|grep -A2 trym[e]
> 2599 pts/4 S+ 0:00 \_ git tryme
> 2601 pts/4 S+ 0:00 \_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600
> 2602 pts/4 S+ 0:00 \_ sleep 600
> 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo
> /bin/sh
> -c
> echo $PWD;sleep 600
> echo $PWD;sleep 600
This duplicated argument is expected and normal. The arguments after "-c
whatever" become positional parameters $0, $1, etc. The actual script
arguments start at "$1", and "$0" is typically the "script name".
So you have to stick some placeholder value in the "$0" slot, so that
the sub-script can find the actual arguments. E.g., try:
sh -c '
for i in "$@"; do
echo "got $i"
done
' one two three
it will print only:
got two
got three
But if you stick a placeholder there, it works:
sh -c '
for i in "$@"; do
echo "got $i"
done
' placeholder one two three
The value of the placeholder does not matter to the shell. But it is
accessible to the script inside via $0:
sh -c '
echo "\$0 = $0"
echo "\$1 = $1"
echo "\$2 = $2"
echo "\$3 = $3"
' placeholder one two three
Since our script does not have a filename, we just stick the script
contents there (which is really just a convention, and one I doubt
anybody is really relying on, but there's no point in breaking it now).
> --- a/run-command.c
> +++ b/run-command.c
> @@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
> else
> argv_array_pushf(out, "%s \"$@\"", argv[0]);
> }
> -
> - argv_array_pushv(out, argv);
> + else
> + argv_array_pushv(out, argv);
> return out->argv;
> }
Try running "make test" with this. Lots of things break, because we are
not sending the positional parameters to the shell script at all.
If we just cared about the positional parmeters, we _could_ do something
like:
if (argv[0]) {
argv_array_push(out, "sh");
argv_array_pushv(out, argv + 1);
}
That would omit "$0" entirely when we have no positional parameters (and
the shell generally fills in "sh" there itself), and provide a dummy
"sh" value when we need to use it as a placeholder.
But again, there's no real value in breaking the existing convention.
-Peff
^ permalink raw reply
* [PATCH 1/2] t9000-addresses: update expected results after fix
From: Matthieu Moy @ 2016-10-21 9:20 UTC (permalink / raw)
To: gitster; +Cc: git, Ramsay Jones, Matthieu Moy
In-Reply-To: <xmqq1szaeda9.fsf@gitster.mtv.corp.google.com>
e3fdbcc8e1 (parse_mailboxes: accept extra text after <...> address,
2016-10-13) improved our in-house address parser and made it closer to
Mail::Address. As a consequence, some tests comparing it to
Mail::Address now pass, but e3fdbcc8e1 forgot to update the test.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
t/t9000/test.pl | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
index 2d05d3eeab..dfeaa9c655 100755
--- a/t/t9000/test.pl
+++ b/t/t9000/test.pl
@@ -32,15 +32,15 @@ my @success_list = (q[Jane],
q["Jane\" Doe" <jdoe@example.com>],
q[Doe, jane <jdoe@example.com>],
q["Jane Doe <jdoe@example.com>],
- q['Jane 'Doe' <jdoe@example.com>]);
+ q['Jane 'Doe' <jdoe@example.com>],
+ q[Jane@:;\.,()<>Doe <jdoe@example.com>],
+ q[Jane <jdoe@example.com> Doe],
+ q[<jdoe@example.com> Jane Doe]);
my @known_failure_list = (q[Jane\ Doe <jdoe@example.com>],
q["Doe, Ja"ne <jdoe@example.com>],
q["Doe, Katarina" Jane <jdoe@example.com>],
- q[Jane@:;\.,()<>Doe <jdoe@example.com>],
q[Jane jdoe@example.com],
- q[<jdoe@example.com> Jane Doe],
- q[Jane <jdoe@example.com> Doe],
q["Jane "Kat"a" ri"na" ",Doe" <jdoe@example.com>],
q[Jane Doe],
q[Jane "Doe <jdoe@example.com>"],
--
2.10.1.651.gffd0de0
^ permalink raw reply related
* [PATCH 2/2] Git.pm: add comment pointing to t9000
From: Matthieu Moy @ 2016-10-21 9:20 UTC (permalink / raw)
To: gitster; +Cc: git, Ramsay Jones, Matthieu Moy
In-Reply-To: <20161021092024.15861-1-Matthieu.Moy@imag.fr>
parse_mailboxes should probably eventually be completely equivalent to
Mail::Address, and if this happens we can drop the Mail::Address
dependency. Add a comment in the code reminding the current state of the
code, and point to the corresponding failing test to help future
contributors to get it right.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
perl/Git.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/perl/Git.pm b/perl/Git.pm
index 42e0895ef7..8bb2b7c7e3 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -870,6 +870,8 @@ Return an array of mailboxes extracted from a string.
=cut
+# Very close to Mail::Address's parser, but we still have minor
+# differences in some cases (see t9000 for examples).
sub parse_mailboxes {
my $re_comment = qr/\((?:[^)]*)\)/;
my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
--
2.10.1.651.gffd0de0
^ permalink raw reply related
* [PATCH 0/3] fix travis TAP/--verbose conflict
From: Jeff King @ 2016-10-21 10:41 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021084348.dp4jfpfownodl7nz@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 04:43:48AM -0400, Jeff King wrote:
> The obvious fix would be to send "--verbose" output to stderr, but I
> suspect that would end up annoying for people who do:
>
> ./t5547-push-quarantine.sh -v | less
>
> to read long output. Probably we need some option like "--log" which
> logs in the same way that "--tee" does, but _without_ sending the data
> to stdout. Naively, that just means replacing the "tee" invocation with
> "cat", but I suspect it will be a lot more complicated than that,
> because we still need to let the TAP output go to stdout.
Yeah, it was definitely a lot more complicated. This patch series fixes
it.
[1/3]: test-lib: handle TEST_OUTPUT_DIRECTORY with spaces
[2/3]: test-lib: add --verbose-log option
[3/3]: travis: use --verbose-log test option
.travis.yml | 2 +-
t/README | 6 ++++++
t/test-lib.sh | 24 ++++++++++++++++++++----
3 files changed, 27 insertions(+), 5 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/3] test-lib: handle TEST_OUTPUT_DIRECTORY with spaces
From: Jeff King @ 2016-10-21 10:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
We are careful in test_done to handle a results directory
with a space in it, but the "--tee" code path does not.
Doing:
export TEST_OUTPUT_DIRECTORY='/tmp/path with spaces'
./t000-init.sh --tee
results in errors. Let's consistently double-quote our path
variables so that this works.
Signed-off-by: Jeff King <peff@peff.net>
---
I imagine nobody cares much about this, but I just happened to notice it
while writing the rest of the patches (and it's obviously quite easy to
fix).
t/test-lib.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..33cbbb7806 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -58,8 +58,8 @@ done,*)
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
- echo $? > $BASE.exit) | tee $BASE.out
- test "$(cat $BASE.exit)" = 0
+ echo $? >"$BASE.exit") | tee "$BASE.out"
+ test "$(cat "$BASE.exit")" = 0
exit
;;
esac
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* [PATCH 2/3] test-lib: add --verbose-log option
From: Jeff King @ 2016-10-21 10:48 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
The "--verbose" option redirects output from arbitrary
test commands to stdout. This is useful for examining the
output manually, like:
./t5547-push-quarantine.sh -v | less
But it also means that the output is intermingled with the
TAP directives, which can confuse a TAP parser like "prove".
This has always been a potential problem, but became an
issue recently when one test happened to output the word
"ok" on a line by itself, which prove interprets as a test
success:
$ prove t5547-push-quarantine.sh :: -v
t5547-push-quarantine.sh .. 1/? To dest.git
* [new branch] HEAD -> master
To dest.git
! [remote rejected] reject -> reject (pre-receive hook declined)
error: failed to push some refs to 'dest.git'
fatal: git cat-file d08c8eba97f4e683ece08654c7c8d2ba0c03b129: bad file
t5547-push-quarantine.sh .. Failed -1/4 subtests
Test Summary Report
-------------------
t5547-push-quarantine.sh (Wstat: 0 Tests: 5 Failed: 0)
Parse errors: Tests out of sequence. Found (2) but expected (3)
Tests out of sequence. Found (3) but expected (4)
Tests out of sequence. Found (4) but expected (5)
Bad plan. You planned 4 tests but ran 5.
Files=1, Tests=5, 0 wallclock secs ( 0.01 usr + 0.01 sys = 0.02 CPU)
Result: FAIL
One answer is "if it hurts, don't do it", but that's not
quite the whole story. The Travis tests use "--verbose
--tee" so that they can get the benefit of prove's parallel
options, along with a verbose log in case there is a
failure. We just need the verbose output to go to the log,
but keep stdout clean.
Getting this right turns out to be surprisingly difficult.
Here's the progression of alternatives I considered:
1. Add an option to write verbose output to stderr. This is
hard to capture, though, because we want each test to
have its own log (because they're all run in parallel
and the jumbled output would be useless).
2. Add an option to write verbose output to a file in
test-results. This works, but the log is missing all of
the non-verbose output, which gives context.
3. Like (2), but teach say_color() to additionally output
to the log. This mostly works, but misses any output
that happens outside of the say() functions (which isn't
a lot, but is a potential maintenance headache).
4. Like (2), but make the log file the same as the "--tee"
file. That almost works, but now we have two processes
opening the same file. That gives us two separate
descriptors, each with their own idea of the current
position. They'll each start writing at offset 0, and
overwrite each other's data.
5. Like (4), but in each case open the file for appending.
That atomically positions each write at the end of the
file.
It's possible we may still get sheared writes between
the two processes, but this is already the case when
writing to stdout. It's not a problem in practice
because the test harness generally waits for snippets to
finish before writing the TAP output.
We can ignore buffering issues with tee, because POSIX
mandates that it does not buffer. Likewise, POSIX
specifies "tee -a", so it should be available
everywhere.
This patch implements option (5), which seems to work well
in practice.
Signed-off-by: Jeff King <peff@peff.net>
---
Arguably we don't need a new option for this, and could just do it
automatically. I wasn't sure when, though:
- if "--tee --verbose" triggers it, then somebody running that outside
of "prove" who _wants_ the verbose output on stdout (because they're
looking at it, but also want to save a copy to the log) would be
regressed
- possibly "--tee" could just always write verbose output to the
logfile (but not stdout). That's kind of weirdly magical, and we'd
have to update the travis invocation anyway.
So I went with a new option which implies the other ones. No chance of
regression, and it's easy to explain.
t/README | 6 ++++++
t/test-lib.sh | 22 +++++++++++++++++++---
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/t/README b/t/README
index 0f764c0aef..4982d1c521 100644
--- a/t/README
+++ b/t/README
@@ -153,6 +153,12 @@ appropriately before running "make".
As the names depend on the tests' file names, it is safe to
run the tests with this option in parallel.
+--verbose-log::
+ Write verbose output to the same logfile as `--tee`, but do
+ _not_ write it to stdout. Unlike `--tee --verbose`, this option
+ is safe to use when stdout is being consumed by a TAP parser
+ like `prove`. Implies `--tee` and `--verbose`.
+
--with-dashes::
By default tests are run without dashed forms of
commands (like git-commit) in the PATH (it only uses
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 33cbbb7806..85946ec40d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -54,11 +54,21 @@ case "$GIT_TEST_TEE_STARTED, $* " in
done,*)
# do not redirect again
;;
-*' --tee '*|*' --va'*)
+*' --tee '*|*' --va'*|*' --verbose-log '*)
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
+
+ # Make this filename available to the sub-process in case it is using
+ # --verbose-log.
+ GIT_TEST_TEE_OUTPUT_FILE=$BASE.out
+ export GIT_TEST_TEE_OUTPUT_FILE
+
+ # Truncate before calling "tee -a" to get rid of the results
+ # from any previous runs.
+ >"$GIT_TEST_TEE_OUTPUT_FILE"
+
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
- echo $? >"$BASE.exit") | tee "$BASE.out"
+ echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$BASE.exit")" = 0
exit
;;
@@ -246,6 +256,9 @@ do
trace=t
verbose=t
shift ;;
+ --verbose-log)
+ verbose_log=t
+ shift ;;
*)
echo "error: unknown test option '$1'" >&2; exit 1 ;;
esac
@@ -319,7 +332,10 @@ fi
exec 5>&1
exec 6<&0
-if test "$verbose" = "t"
+if test "$verbose_log" = "t"
+then
+ exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
+elif test "$verbose" = "t"
then
exec 4>&2 3>&1
else
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* [PATCH 3/3] travis: use --verbose-log test option
From: Jeff King @ 2016-10-21 10:48 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
Because we run the tests via "prove", the output from
"--verbose" may interfere with our TAP output. Using
"--verbose-log" solves this while letting us retain our
on-disk log.
Signed-off-by: Jeff King <peff@peff.net>
---
.travis.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.travis.yml b/.travis.yml
index 37a1e1fb6d..9a65514d82 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -31,7 +31,7 @@ env:
- LINUX_GIT_LFS_VERSION="1.2.0"
- DEFAULT_TEST_TARGET=prove
- GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
- - GIT_TEST_OPTS="--verbose --tee"
+ - GIT_TEST_OPTS="--verbose-log"
- GIT_TEST_HTTPD=true
- GIT_TEST_CLONE_2GB=YesPlease
# t9810 occasionally fails on Travis CI OS X
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* Re: Drastic jump in the time required for the test suite
From: Duy Nguyen @ 2016-10-21 10:59 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <e0e320ce-14a1-b4a8-aa07-6bc4f3c4a424@web.de>
On Thu, Oct 20, 2016 at 11:40 PM, René Scharfe <l.s.r@web.de> wrote:
> Am 20.10.2016 um 13:02 schrieb Duy Nguyen:
>> On Wed, Oct 19, 2016 at 4:18 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Junio,
>>>
>>> I know you are a fan of testing things thoroughly in the test suite, but I
>>> have to say that it is getting out of hand, in particular due to our
>>> over-use of shell script idioms (which really only run fast on Linux, not
>>> a good idea for a portable software).
>>>
>>> My builds of `pu` now time out, after running for 3h straight in the VM
>>> dedicated to perform the daily routine of building and testing the git.git
>>> branches in Git for Windows' SDK. For comparison, `next` passes build &
>>> tests in 2.6h. That is quite the jump.
>>
>> I'm just curious, will running git.exe from WSL [1] help speed things
>> up a bit (or, hopefully, a lot)? I'm assuming that shell's speed in
>> WSL is quite fast.
>>
>> I'm pretty sure the test suite would need some adaptation, but if the
>> speedup is significant, maybe it's worth spending time on.
>>
>> [1] https://news.ycombinator.com/item?id=12748395
>
> I get this on WSL with prove -j8:
>
> Files=750, Tests=13657, 906 wallclock secs ( 8.51 usr 17.17 sys + 282.62 cusr 3731.85 csys = 4040.15 CPU)
>
> And this for a run on Debian inside a Hyper-V VM on the same system:
>
> Files=759, Tests=13895, 99 wallclock secs ( 4.81 usr 1.06 sys + 39.70 cusr 25.82 csys = 71.39 CPU)
>
> All tests pass on master.
Thank you for doing this. 10 times slower is probably not worth
following up (though absolute numbers still look amazing, you have
some beefy machine there).
--
Duy
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Duy Nguyen @ 2016-10-21 11:03 UTC (permalink / raw)
To: Dennis Kaarsemaker
Cc: Jeff King, Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <1476996017.28685.10.camel@kaarsemaker.net>
On Fri, Oct 21, 2016 at 3:40 AM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On Thu, 2016-10-20 at 08:31 -0400, Jeff King wrote:
>
>> I'm also not entirely convinced that the test suite being a shell script
>> is the main culprit for its slowness. We run git a lot of times, and
>> that's inherent in testing it. I ran the whole test suite under
>> "strace -f -e execve". There are ~335K execs. Here's the breakdown of
>> the top ones:
>
> You're measuring execve's, but fork (well, fork emulation. There's no
> actual fork) is also expensive on windows iirc, so subshells add a lot
> to this cost.
shells fork on piping as well, and redirection and some other
construct if I remember correctly (I attempted to port busybox ash to
windows and had to find and "fix" all the forks)
> That said, strace -eclone says that a 'make test' forks
> ~408k times, and while this is significantly more than the amount of
> execs in your example, this does include cvs and svn tests and it's
> still in the same ballpark.
--
Duy
^ permalink raw reply
* Re: [BUG] [PATCH]: run-command.c
From: Duncan Roe @ 2016-10-21 11:07 UTC (permalink / raw)
To: git
In-Reply-To: <20161021090029.glr5u6gwrxluavir@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 05:00:29AM -0400, Jeff King wrote:
> On Fri, Oct 21, 2016 at 04:50:13PM +1100, Duncan Roe wrote:
>
> > For example, if .git/config has this alias (the sleep is to leave time to
> > examine output from ps, &c.):
> >
> > [alias]
> > tryme = "!echo $PWD;sleep 600"
> >
> > [...]
> > 16:42:06$ ps axf|grep -A2 trym[e]
> > 2599 pts/4 S+ 0:00 \_ git tryme
> > 2601 pts/4 S+ 0:00 \_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600
> > 2602 pts/4 S+ 0:00 \_ sleep 600
> > 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo
> > /bin/sh
> > -c
> > echo $PWD;sleep 600
> > echo $PWD;sleep 600
>
> This duplicated argument is expected and normal. The arguments after "-c
> whatever" become positional parameters $0, $1, etc. The actual script
> arguments start at "$1", and "$0" is typically the "script name".
> So you have to stick some placeholder value in the "$0" slot, so that
> the sub-script can find the actual arguments. E.g., try:
>
> sh -c '
> for i in "$@"; do
> echo "got $i"
> done
> ' one two three
>
> it will print only:
>
> got two
> got three
>
> But if you stick a placeholder there, it works:
>
> sh -c '
> for i in "$@"; do
> echo "got $i"
> done
> ' placeholder one two three
>
> The value of the placeholder does not matter to the shell. But it is
> accessible to the script inside via $0:
>
> sh -c '
> echo "\$0 = $0"
> echo "\$1 = $1"
> echo "\$2 = $2"
> echo "\$3 = $3"
> ' placeholder one two three
>
> Since our script does not have a filename, we just stick the script
> contents there (which is really just a convention, and one I doubt
> anybody is really relying on, but there's no point in breaking it now).
>
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
> > else
> > argv_array_pushf(out, "%s \"$@\"", argv[0]);
> > }
> > -
> > - argv_array_pushv(out, argv);
> > + else
> > + argv_array_pushv(out, argv);
> > return out->argv;
> > }
>
> Try running "make test" with this. Lots of things break, because we are
> not sending the positional parameters to the shell script at all.
>
> If we just cared about the positional parmeters, we _could_ do something
> like:
>
> if (argv[0]) {
> argv_array_push(out, "sh");
> argv_array_pushv(out, argv + 1);
> }
>
> That would omit "$0" entirely when we have no positional parameters (and
> the shell generally fills in "sh" there itself), and provide a dummy
> "sh" value when we need to use it as a placeholder.
>
> But again, there's no real value in breaking the existing convention.
>
> -Peff
Agreed - tests 110 and 111 in t1300-repo-config.sh fail. After that, "make test"
gives up, losing about 14000 lines of output.
Sorry for the noise,
Cheers ... Duncan.
^ permalink raw reply
* Re: [PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts
From: Johannes Schindelin @ 2016-10-21 11:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqmvhyg4ru.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 20 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > The return value of do_recursive_merge() may be positive (indicating merge
> > conflicts), or 0 (indicating success). It also may be negative, indicating
> > a fatal error that requires us to abort.
> >
> > Now, if the return value indicates that there are merge conflicts, we
> > should not try to commit those changes, of course.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > sequencer.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index cbc3742..9ffc090 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> > res = allow;
> > goto leave;
> > }
> > - if (!opts->no_commit)
> > + if (!res && !opts->no_commit)
> > res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> > opts, allow, opts->edit, 0, 0);
>
> This by itself looks more like a bugfix than preparation for later
> steps. The only reason why it is not a bugfix is because there is
> nothing in this function that makes res a non-zero value and reach
> this if statement at this step. We would have been caught by an
> "if (res) { ... rerere(); goto leave; }" or
> "if (allow < 0) { res = allow; goto leave; }"
> that appear before this part of the code.
>
> So while it is not wrong per-se, I think this should be part of an
> actual change that makes it possible for the control flow to reach
> here with non-zero res.
It looks like it is no longer needed (I *think* that it was made obsolete
by the change where I now "goto fast_forward_edit" only in case there were
no errors).
In any case, the patch's gone now,
Dscho
^ permalink raw reply
* Re: [PATCH v4 20/25] sequencer: refactor write_message()
From: Johannes Schindelin @ 2016-10-21 11:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqd1iug42f.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 20 Oct 2016, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > If I were doing this, I would make this into three separate steps:
> >
> > - move the strbuf_release(msgbuf) to the caller in
> > do_pick_commit();
> >
> > - add the missing rollback_lock_file(), which is a bugfix; and
> > then finally
> >
> > - allow the helper to take not a strbuf but <buf, len> pair as
> > parameters.
> >
> > The end result of this patch achieves two thirds of the above, but
> > especially given that write_message() only has two call sites in a
> > single function, I think it is OK and preferrable even to do all
> > three.
>
> Ah, make that four steps. The final one is:
>
> - add append_eol parameter that nobody uses at this step in the
> series.
Done,
Dscho
^ permalink raw reply
* Re: [BUG] fetch output is ugly in 'next'
From: Duy Nguyen @ 2016-10-21 12:11 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161021002654.qo6kcya4gocp73rs@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 7:26 AM, Jeff King <peff@peff.net> wrote:
> I recently started using lt/abbrev-auto via 'next'. This is the fetch
> output I saw today:
>
> $ git fetch
> remote: Counting objects: 332, done.
> remote: Compressing objects: 100% (237/237), done.
> remote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31
> Receiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (184/184), completed with 26 local objects.
> From git://github.com/gitster/git
> + fb65135d9c...16ab66ec97 jch -> origin/jch (forced update)
> fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline -> origin/jk/diff-submodule-diff-inline
> * [new branch] jk/no-looking-at-dotgit-outside-repo -> origin/jk/no-looking-at-dotgit-outside-repo
> + 3a8f853f16...1369f9b5ba jt/trailer-with-cruft -> origin/jt/trailer-with-cruft (forced update)
> * [new branch] pt/gitgui-updates -> origin/pt/gitgui-updates
> + 7594b34cbb...be8e40093b pu -> origin/pu (forced update)
> + 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix -> origin/tg/add-chmod+x-fix (forced update)
> + c4cf1f93cf...221912514c va/i18n-perl-scripts -> origin/va/i18n-perl-scripts (forced update)
> * [new branch] yk/git-tag-remove-mention-of-old-layout-in-doc -> origin/yk/git-tag-remove-mention-of-old-layout-in-doc
>
> Yuck.
My eyes!
> I could maybe get over the wasted space due to the longer sha1s,
> but we clearly need to fix the alignment computation. I haven't dug on
> it at all; it's probably low-hanging fruit if somebody is interested.
Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something
sensible should do it.
--
Duy
^ permalink raw reply
* [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Johannes Schindelin @ 2016-10-21 12:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1476450940.git.johannes.schindelin@gmx.de>
This patch series marks the '4' in the countdown to speed up rebase -i
by implementing large parts in C (read: there will be three more patch
series after that before the full benefit hits git.git: sequencer-i,
rebase--helper and rebase-i-extra). It is based on the `libify-sequencer`
patch series I submitted earlier.
The patches in this series merely prepare the sequencer code for the
next patch series that actually teaches the sequencer to run rebase -i's
commands.
The reason to split these two patch series is simple: to keep them at a
sensible size.
The two patch series after that are much smaller: a two-patch "series"
that switches rebase -i to use the sequencer (except with --root or
--preserve-merges), and a couple of patches to move several pretty
expensive script processing steps to C (think: autosquash).
The end game of this patch series is a git-rebase--helper that makes
rebase -i 5x faster on Windows (according to t/perf/p3404). Travis says
that even MacOSX and Linux benefit (4x and 3x, respectively).
I have been working on this since early February, whenever time allowed,
and it is time to put it into the users' hands. To that end, I already
integrated the whole shebang into Git for Windows 2.10.0 and 2.10.1
where it has been running without complaints (and some quite positive
feedback).
It would be *really* nice if we could get this patch series at least into
`next` soon, as it gets late and later for the rest of the patches to make
it into `master` in time for v2.11 (and it is not for lack of trying on my
end...).
Changes vs v4:
- clarified in a code comment that read_oneliner() is lenient when it
comes to multi-line files: it still reads the entire file, but strips
off only the final EOL (if any).
- rephrased a commit message to stop judging Junio's taste ;-)
- changed "strip LF" to "skip LF", as requested.
- rephrased a commit message to talk about pluggin memory leaks instead
of stating that the memory is now eventually released.
- dropped the "sequencer: do not try to commit when there were merge
conflicts" patch that appears to be no longer necessary.
- split and modified the commit refactoring write_message() according to
Junio's suggestions.
Johannes Schindelin (27):
sequencer: use static initializers for replay_opts
sequencer: use memoized sequencer directory path
sequencer: avoid unnecessary indirection
sequencer: future-proof remove_sequencer_state()
sequencer: plug memory leaks for the option values
sequencer: future-proof read_populate_todo()
sequencer: refactor the code to obtain a short commit name
sequencer: completely revamp the "todo" script parsing
sequencer: strip CR from the todo script
sequencer: avoid completely different messages for different actions
sequencer: get rid of the subcommand field
sequencer: remember the onelines when parsing the todo file
sequencer: prepare for rebase -i's commit functionality
sequencer: introduce a helper to read files written by scripts
sequencer: allow editing the commit message on a case-by-case basis
sequencer: support amending commits
sequencer: support cleaning up commit messages
sequencer: left-trim lines read from the script
sequencer: stop releasing the strbuf in write_message()
sequencer: roll back lock file if write_message() failed
sequencer: refactor write_message() to take a pointer/length
sequencer: teach write_message() to append an optional LF
sequencer: remove overzealous assumption in rebase -i mode
sequencer: mark action_name() for translation
sequencer: quote filenames in error messages
sequencer: start error messages consistently with lower case
sequencer: mark all error messages for translation
builtin/commit.c | 2 +-
builtin/revert.c | 46 ++-
sequencer.c | 680 ++++++++++++++++++++++++++++--------------
sequencer.h | 23 +-
t/t3501-revert-cherry-pick.sh | 2 +-
5 files changed, 492 insertions(+), 261 deletions(-)
base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v5
Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v5
Interdiff vs v4:
diff --git a/sequencer.c b/sequencer.c
index 1cf70f7..a61fe76 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -234,8 +234,8 @@ static void print_advice(int show_hint, struct replay_opts *opts)
}
}
-static int write_with_lock_file(const char *filename,
- const void *buf, size_t len, int append_eol)
+static int write_message(const void *buf, size_t len, const char *filename,
+ int append_eol)
{
static struct lock_file msg_file;
@@ -258,16 +258,12 @@ static int write_with_lock_file(const char *filename,
return 0;
}
-static int write_message(struct strbuf *msgbuf, const char *filename)
-{
- int res = write_with_lock_file(filename, msgbuf->buf, msgbuf->len, 0);
- strbuf_release(msgbuf);
- return res;
-}
-
/*
- * Reads a file that was presumably written by a shell script, i.e.
- * with an end-of-line marker that needs to be stripped.
+ * Reads a file that was presumably written by a shell script, i.e. with an
+ * end-of-line marker that needs to be stripped.
+ *
+ * Note that only the last end-of-line marker is stripped, consistent with the
+ * behavior of "$(cat path)" in a shell script.
*
* Returns 1 if the file was read, 0 if it could not be read or does not exist.
*/
@@ -762,12 +758,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
head, &msgbuf, opts);
if (res < 0)
return res;
- res |= write_message(&msgbuf, git_path_merge_msg());
+ res |= write_message(msgbuf.buf, msgbuf.len,
+ git_path_merge_msg(), 0);
} else {
struct commit_list *common = NULL;
struct commit_list *remotes = NULL;
- res = write_message(&msgbuf, git_path_merge_msg());
+ res = write_message(msgbuf.buf, msgbuf.len,
+ git_path_merge_msg(), 0);
commit_list_insert(base, &common);
commit_list_insert(next, &remotes);
@@ -777,6 +775,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
free_commit_list(common);
free_commit_list(remotes);
}
+ strbuf_release(&msgbuf);
/*
* If the merge was clean or if it failed due to conflict, we write
@@ -808,7 +807,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
res = allow;
goto leave;
}
- if (!res && !opts->no_commit)
+ if (!opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
opts, allow, opts->edit, 0, 0);
@@ -934,7 +933,7 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
for (i = 1; *p; i++, p = next_p) {
char *eol = strchrnul(p, '\n');
- next_p = *eol ? eol + 1 /* strip LF */ : eol;
+ next_p = *eol ? eol + 1 /* skip LF */ : eol;
if (p != eol && eol[-1] == '\r')
eol--; /* strip Carriage Return */
--
2.10.1.583.g721a9e0
^ permalink raw reply
* [PATCH v5 02/27] sequencer: use memoized sequencer directory path
From: Johannes Schindelin @ 2016-10-21 12:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/commit.c | 2 +-
sequencer.c | 11 ++++++-----
sequencer.h | 5 +----
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1cba3b7..9fddb19 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -183,7 +183,7 @@ static void determine_whence(struct wt_status *s)
whence = FROM_MERGE;
else if (file_exists(git_path_cherry_pick_head())) {
whence = FROM_CHERRY_PICK;
- if (file_exists(git_path(SEQ_DIR)))
+ if (file_exists(git_path_seq_dir()))
sequencer_in_use = 1;
}
else
diff --git a/sequencer.c b/sequencer.c
index eec8a60..cb16cbd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -21,10 +21,11 @@
const char sign_off_header[] = "Signed-off-by: ";
static const char cherry_picked_prefix[] = "(cherry picked from commit ";
-static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE)
-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)
+GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
+
+static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
+static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
+static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
static int is_rfc2822_line(const char *buf, int len)
{
@@ -112,7 +113,7 @@ static void remove_sequencer_state(void)
{
struct strbuf seq_dir = STRBUF_INIT;
- strbuf_addstr(&seq_dir, git_path(SEQ_DIR));
+ strbuf_addstr(&seq_dir, git_path_seq_dir());
remove_dir_recursively(&seq_dir, 0);
strbuf_release(&seq_dir);
}
diff --git a/sequencer.h b/sequencer.h
index db425ad..dd4d33a 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -1,10 +1,7 @@
#ifndef SEQUENCER_H
#define SEQUENCER_H
-#define SEQ_DIR "sequencer"
-#define SEQ_HEAD_FILE "sequencer/head"
-#define SEQ_TODO_FILE "sequencer/todo"
-#define SEQ_OPTS_FILE "sequencer/opts"
+const char *git_path_seq_dir(void);
#define APPEND_SIGNOFF_DEDUP (1u << 0)
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v5 01/27] sequencer: use static initializers for replay_opts
From: Johannes Schindelin @ 2016-10-21 12:23 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
This change is not completely faithful: instead of initializing all fields
to 0, we choose to initialize command and subcommand to -1 (instead of
defaulting to REPLAY_REVERT and REPLAY_NONE, respectively). Practically,
it makes no difference at all, but future-proofs the code to require
explicit assignments for both fields.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/revert.c | 6 ++----
sequencer.h | 1 +
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 4e69380..7365559 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -178,10 +178,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
int cmd_revert(int argc, const char **argv, const char *prefix)
{
- struct replay_opts opts;
+ struct replay_opts opts = REPLAY_OPTS_INIT;
int res;
- memset(&opts, 0, sizeof(opts));
if (isatty(0))
opts.edit = 1;
opts.action = REPLAY_REVERT;
@@ -195,10 +194,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
{
- struct replay_opts opts;
+ struct replay_opts opts = REPLAY_OPTS_INIT;
int res;
- memset(&opts, 0, sizeof(opts));
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
diff --git a/sequencer.h b/sequencer.h
index 5ed5cb1..db425ad 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -47,6 +47,7 @@ struct replay_opts {
/* Only used by REPLAY_NONE */
struct rev_info *revs;
};
+#define REPLAY_OPTS_INIT { -1, -1 }
int sequencer_pick_revisions(struct replay_opts *opts);
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v5 04/27] sequencer: future-proof remove_sequencer_state()
From: Johannes Schindelin @ 2016-10-21 12:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
In a couple of commits, we will teach the sequencer to handle the
nitty gritty of the interactive rebase, which keeps its state in a
different directory.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index c2fbf6f..8d272fb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,11 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static const char *get_dir(const struct replay_opts *opts)
+{
+ return git_path_seq_dir();
+}
+
static int is_rfc2822_line(const char *buf, int len)
{
int i;
@@ -109,13 +114,13 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
return 1;
}
-static void remove_sequencer_state(void)
+static void remove_sequencer_state(const struct replay_opts *opts)
{
- struct strbuf seq_dir = STRBUF_INIT;
+ struct strbuf dir = STRBUF_INIT;
- strbuf_addstr(&seq_dir, git_path_seq_dir());
- remove_dir_recursively(&seq_dir, 0);
- strbuf_release(&seq_dir);
+ strbuf_addf(&dir, "%s", get_dir(opts));
+ remove_dir_recursively(&dir, 0);
+ strbuf_release(&dir);
}
static const char *action_name(const struct replay_opts *opts)
@@ -940,7 +945,7 @@ static int sequencer_rollback(struct replay_opts *opts)
}
if (reset_for_rollback(sha1))
goto fail;
- remove_sequencer_state();
+ remove_sequencer_state(opts);
strbuf_release(&buf);
return 0;
fail:
@@ -1034,7 +1039,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
* Sequence of picks finished successfully; cleanup by
* removing the .git/sequencer directory
*/
- remove_sequencer_state();
+ remove_sequencer_state(opts);
return 0;
}
@@ -1095,7 +1100,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
* one that is being continued
*/
if (opts->subcommand == REPLAY_REMOVE_STATE) {
- remove_sequencer_state();
+ remove_sequencer_state(opts);
return 0;
}
if (opts->subcommand == REPLAY_ROLLBACK)
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* Git context menu in Windows Exporer has a “git bash” option but it doesn't work
From: Stefan Monov @ 2016-10-21 12:23 UTC (permalink / raw)
To: git
Hi.
The default git distribution for windows contains, besides the CLI
tools, a shell integration, i.e. a context menu for Windows Explorer.
This context menu has a "Git bash" option which I suppose should open
a Git bash prompt cd'd to the current directory. But instead, the git
bash window opens and immediately closes, giving me no chance to even
see what it says inside it. Any fix?
Note: Opening Git Bash from the Windows Start Menu works fine, but if
I open it that way, then I need to manually cd to the needed dir.
TIA,
Stefan Monov
^ permalink raw reply
* [PATCH v5 03/27] sequencer: avoid unnecessary indirection
From: Johannes Schindelin @ 2016-10-21 12:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
We really do not need the *pointer to a* pointer to the options in
the read_populate_opts() function.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index cb16cbd..c2fbf6f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -813,7 +813,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
return 0;
}
-static int read_populate_opts(struct replay_opts **opts)
+static int read_populate_opts(struct replay_opts *opts)
{
if (!file_exists(git_path_opts_file()))
return 0;
@@ -823,7 +823,7 @@ static int read_populate_opts(struct replay_opts **opts)
* about this case, though, because we wrote that file ourselves, so we
* are pretty certain that it is syntactically correct.
*/
- if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
+ if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0)
return error(_("Malformed options sheet: %s"),
git_path_opts_file());
return 0;
@@ -1054,7 +1054,7 @@ static int sequencer_continue(struct replay_opts *opts)
if (!file_exists(git_path_todo_file()))
return continue_single_pick();
- if (read_populate_opts(&opts) ||
+ if (read_populate_opts(opts) ||
read_populate_todo(&todo_list, opts))
return -1;
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v5 05/27] sequencer: plug memory leaks for the option values
From: Johannes Schindelin @ 2016-10-21 12:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
like a one-shot command when it reads its configuration: memory is
allocated and released only when the command exits.
This is kind of okay for git-cherry-pick, which *is* a one-shot
command. All the work to make the sequencer its work horse was
done to allow using the functionality as a library function, though,
including proper clean-up after use.
To remedy that, we now take custody of the option values in question,
requiring those values to be malloc()ed or strdup()ed (an alternative
approach, to add a list of pointers to malloc()ed data and to ask the
sequencer to release all of them in the end, was proposed earlier but
rejected during review).
Note: this means that we now have to take care to strdup() the values
passed via the command-line.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/revert.c | 4 ++++
sequencer.c | 26 ++++++++++++++++++++++----
sequencer.h | 6 +++---
3 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 7365559..ba5a88c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -174,6 +174,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
if (argc > 1)
usage_with_options(usage_str, options);
+
+ /* These option values will be free()d */
+ opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
+ opts->strategy = xstrdup_or_null(opts->strategy);
}
int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 8d272fb..04c55f2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -117,6 +117,13 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
static void remove_sequencer_state(const struct replay_opts *opts)
{
struct strbuf dir = STRBUF_INIT;
+ int i;
+
+ free(opts->gpg_sign);
+ free(opts->strategy);
+ for (i = 0; i < opts->xopts_nr; i++)
+ free(opts->xopts[i]);
+ free(opts->xopts);
strbuf_addf(&dir, "%s", get_dir(opts));
remove_dir_recursively(&dir, 0);
@@ -280,7 +287,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
struct merge_options o;
struct tree *result, *next_tree, *base_tree, *head_tree;
int clean;
- const char **xopt;
+ char **xopt;
static struct lock_file index_lock;
hold_locked_index(&index_lock, 1);
@@ -583,7 +590,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
commit_list_insert(base, &common);
commit_list_insert(next, &remotes);
- res |= try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+ res |= try_merge_command(opts->strategy,
+ opts->xopts_nr, (const char **)opts->xopts,
common, sha1_to_hex(head), remotes);
free_commit_list(common);
free_commit_list(remotes);
@@ -783,6 +791,16 @@ static int read_populate_todo(struct commit_list **todo_list,
return 0;
}
+static int git_config_string_dup(char **dest,
+ const char *var, const char *value)
+{
+ if (!value)
+ return config_error_nonbool(var);
+ free(*dest);
+ *dest = xstrdup(value);
+ return 0;
+}
+
static int populate_opts_cb(const char *key, const char *value, void *data)
{
struct replay_opts *opts = data;
@@ -803,9 +821,9 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
else if (!strcmp(key, "options.mainline"))
opts->mainline = git_config_int(key, value);
else if (!strcmp(key, "options.strategy"))
- git_config_string(&opts->strategy, key, value);
+ git_config_string_dup(&opts->strategy, key, value);
else if (!strcmp(key, "options.gpg-sign"))
- git_config_string(&opts->gpg_sign, key, value);
+ git_config_string_dup(&opts->gpg_sign, key, value);
else if (!strcmp(key, "options.strategy-option")) {
ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
opts->xopts[opts->xopts_nr++] = xstrdup(value);
diff --git a/sequencer.h b/sequencer.h
index dd4d33a..8453669 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -34,11 +34,11 @@ struct replay_opts {
int mainline;
- const char *gpg_sign;
+ char *gpg_sign;
/* Merge strategy */
- const char *strategy;
- const char **xopts;
+ char *strategy;
+ char **xopts;
size_t xopts_nr, xopts_alloc;
/* Only used by REPLAY_NONE */
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v5 06/27] sequencer: future-proof read_populate_todo()
From: Johannes Schindelin @ 2016-10-21 12:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
Over the next commits, we will work on improving the sequencer to the
point where it can process the todo script of an interactive rebase. To
that end, we will need to teach the sequencer to read interactive
rebase's todo file. In preparation, we consolidate all places where
that todo file is needed to call a function that we will later extend.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 04c55f2..fb0b94b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -32,6 +32,11 @@ static const char *get_dir(const struct replay_opts *opts)
return git_path_seq_dir();
}
+static const char *get_todo_path(const struct replay_opts *opts)
+{
+ return git_path_todo_file();
+}
+
static int is_rfc2822_line(const char *buf, int len)
{
int i;
@@ -769,25 +774,24 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
static int read_populate_todo(struct commit_list **todo_list,
struct replay_opts *opts)
{
+ const char *todo_file = get_todo_path(opts);
struct strbuf buf = STRBUF_INIT;
int fd, res;
- fd = open(git_path_todo_file(), O_RDONLY);
+ fd = open(todo_file, O_RDONLY);
if (fd < 0)
- return error_errno(_("Could not open %s"),
- git_path_todo_file());
+ return error_errno(_("Could not open %s"), todo_file);
if (strbuf_read(&buf, fd, 0) < 0) {
close(fd);
strbuf_release(&buf);
- return error(_("Could not read %s."), git_path_todo_file());
+ return error(_("Could not read %s."), todo_file);
}
close(fd);
res = parse_insn_buffer(buf.buf, todo_list, opts);
strbuf_release(&buf);
if (res)
- return error(_("Unusable instruction sheet: %s"),
- git_path_todo_file());
+ return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
}
@@ -1075,7 +1079,7 @@ static int sequencer_continue(struct replay_opts *opts)
{
struct commit_list *todo_list = NULL;
- if (!file_exists(git_path_todo_file()))
+ if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
if (read_populate_opts(opts) ||
read_populate_todo(&todo_list, opts))
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v5 08/27] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-21 12:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
When we came up with the "sequencer" idea, we really wanted to have
kind of a plumbing equivalent of the interactive rebase. Hence the
choice of words: the "todo" script, a "pick", etc.
However, when it came time to implement the entire shebang, somehow this
idea got lost and the sequencer was used as working horse for
cherry-pick and revert instead. So as not to interfere with the
interactive rebase, it even uses a separate directory to store its
state.
Furthermore, it also is stupidly strict about the "todo" script it
accepts: while it parses commands in a way that was *designed* to be
similar to the interactive rebase, it then goes on to *error out* if the
commands disagree with the overall action (cherry-pick or revert).
Finally, the sequencer code chose to deviate from the interactive rebase
code insofar that when it comes to writing the file with the remaining
commands, it *reformats* the "todo" script instead of just writing the
part of the parsed script that were not yet processed. This is not only
unnecessary churn, but might well lose information that is valuable to
the user (i.e. comments after the commands).
Let's just bite the bullet and rewrite the entire parser; the code now
becomes not only more elegant: it allows us to go on and teach the
sequencer how to parse *true* "todo" scripts as used by the interactive
rebase itself. In a way, the sequencer is about to grow up to do its
older brother's job. Better.
In particular, we choose to maintain the list of commands in an array
instead of a linked list: this is flexible enough to allow us later on to
even implement rebase -i's reordering of fixup!/squash! commits very
easily (and with a very nice speed bonus, at least on Windows).
While at it, do not stop at the first problem, but list *all* of the
problems. This will help the user when the sequencer will do `rebase
-i`'s work by allowing to address all issues in one go rather than going
back and forth until the todo list is valid.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 284 ++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 163 insertions(+), 121 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 499f5ee..145de78 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -470,7 +470,26 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
return 1;
}
-static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
+enum todo_command {
+ TODO_PICK = 0,
+ TODO_REVERT
+};
+
+static const char *todo_command_strings[] = {
+ "pick",
+ "revert"
+};
+
+static const char *command_to_string(const enum todo_command command)
+{
+ if (command < ARRAY_SIZE(todo_command_strings))
+ return todo_command_strings[command];
+ die("Unknown command: %d", command);
+}
+
+
+static int do_pick_commit(enum todo_command command, struct commit *commit,
+ struct replay_opts *opts)
{
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -529,10 +548,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
return fast_forward_to(commit->object.oid.hash, head, unborn, opts);
if (parent && parse_commit(parent) < 0)
- /* TRANSLATORS: The first %s will be "revert" or
- "cherry-pick", the second %s a SHA1 */
+ /* TRANSLATORS: The first %s will be a "todo" command like
+ "revert" or "pick", the second %s a SHA1. */
return error(_("%s: cannot parse parent commit %s"),
- action_name(opts), oid_to_hex(&parent->object.oid));
+ command_to_string(command),
+ oid_to_hex(&parent->object.oid));
if (get_message(commit, &msg) != 0)
return error(_("Cannot get commit message for %s"),
@@ -545,7 +565,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
* reverse of it if we are revert.
*/
- if (opts->action == REPLAY_REVERT) {
+ if (command == TODO_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -586,7 +606,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
}
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
head, &msgbuf, opts);
if (res < 0)
@@ -613,17 +633,17 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
* However, if the merge did not even start, then we don't want to
* write it at all.
*/
- if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&
+ if (command == TODO_PICK && !opts->no_commit && (res == 0 || res == 1) &&
update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
res = -1;
- if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
+ if (command == TODO_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
res = -1;
if (res) {
- error(opts->action == REPLAY_REVERT
+ error(command == TODO_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
short_commit_name(commit), msg.subject);
@@ -684,116 +704,122 @@ static int read_and_refresh_cache(struct replay_opts *opts)
return 0;
}
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
- struct replay_opts *opts)
+struct todo_item {
+ enum todo_command command;
+ struct commit *commit;
+ size_t offset_in_buf;
+};
+
+struct todo_list {
+ struct strbuf buf;
+ struct todo_item *items;
+ int nr, alloc, current;
+};
+
+#define TODO_LIST_INIT { STRBUF_INIT }
+
+static void todo_list_release(struct todo_list *todo_list)
{
- struct commit_list *cur = NULL;
- const char *sha1_abbrev = NULL;
- const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
- const char *subject;
- int subject_len;
+ strbuf_release(&todo_list->buf);
+ free(todo_list->items);
+ todo_list->items = NULL;
+ todo_list->nr = todo_list->alloc = 0;
+}
- for (cur = todo_list; cur; cur = cur->next) {
- const char *commit_buffer = get_commit_buffer(cur->item, NULL);
- sha1_abbrev = find_unique_abbrev(cur->item->object.oid.hash, DEFAULT_ABBREV);
- subject_len = find_commit_subject(commit_buffer, &subject);
- strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
- subject_len, subject);
- unuse_commit_buffer(cur->item, commit_buffer);
- }
- return 0;
+static struct todo_item *append_new_todo(struct todo_list *todo_list)
+{
+ ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
+ return todo_list->items + todo_list->nr++;
}
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
{
unsigned char commit_sha1[20];
- enum replay_action action;
char *end_of_object_name;
- int saved, status, padding;
-
- if (starts_with(bol, "pick")) {
- action = REPLAY_PICK;
- bol += strlen("pick");
- } else if (starts_with(bol, "revert")) {
- action = REPLAY_REVERT;
- bol += strlen("revert");
- } else
- return NULL;
+ int i, saved, status, padding;
+
+ for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
+ if (skip_prefix(bol, todo_command_strings[i], &bol)) {
+ item->command = i;
+ break;
+ }
+ if (i >= ARRAY_SIZE(todo_command_strings))
+ return -1;
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
if (!padding)
- return NULL;
+ return -1;
bol += padding;
- end_of_object_name = bol + strcspn(bol, " \t\n");
+ end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
saved = *end_of_object_name;
*end_of_object_name = '\0';
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
- /*
- * Verify that the action matches up with the one in
- * opts; we don't support arbitrary instructions
- */
- if (action != opts->action) {
- if (action == REPLAY_REVERT)
- error((opts->action == REPLAY_REVERT)
- ? _("Cannot revert during another revert.")
- : _("Cannot revert during a cherry-pick."));
- else
- error((opts->action == REPLAY_REVERT)
- ? _("Cannot cherry-pick during a revert.")
- : _("Cannot cherry-pick during another cherry-pick."));
- return NULL;
- }
-
if (status < 0)
- return NULL;
+ return -1;
- return lookup_commit_reference(commit_sha1);
+ item->commit = lookup_commit_reference(commit_sha1);
+ return !item->commit;
}
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
- struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
{
- struct commit_list **next = todo_list;
- struct commit *commit;
- char *p = buf;
- int i;
+ struct todo_item *item;
+ char *p = buf, *next_p;
+ int i, res = 0;
- for (i = 1; *p; i++) {
+ for (i = 1; *p; i++, p = next_p) {
char *eol = strchrnul(p, '\n');
- commit = parse_insn_line(p, eol, opts);
- if (!commit)
- return error(_("Could not parse line %d."), i);
- next = commit_list_append(commit, next);
- p = *eol ? eol + 1 : eol;
+
+ next_p = *eol ? eol + 1 /* skip LF */ : eol;
+
+ item = append_new_todo(todo_list);
+ item->offset_in_buf = p - todo_list->buf.buf;
+ if (parse_insn_line(item, p, eol)) {
+ res = error(_("Invalid line %d: %.*s"),
+ i, (int)(eol - p), p);
+ item->command = -1;
+ }
}
- if (!*todo_list)
+ if (!todo_list->nr)
return error(_("No commits parsed."));
- return 0;
+ return res;
}
-static int read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
{
const char *todo_file = get_todo_path(opts);
- struct strbuf buf = STRBUF_INIT;
int fd, res;
+ 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(&buf, fd, 0) < 0) {
+ if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
close(fd);
- strbuf_release(&buf);
return error(_("Could not read %s."), todo_file);
}
close(fd);
- res = parse_insn_buffer(buf.buf, todo_list, opts);
- strbuf_release(&buf);
+ res = parse_insn_buffer(todo_list->buf.buf, todo_list);
+ if (!res) {
+ enum todo_command valid =
+ opts->action == REPLAY_PICK ? TODO_PICK : TODO_REVERT;
+ int i;
+
+ for (i = 0; i < todo_list->nr; i++)
+ if (valid == todo_list->items[i].command)
+ continue;
+ else if (valid == TODO_PICK)
+ return error(_("Cannot cherry-pick during a revert."));
+ else
+ return error(_("Cannot revert during a cherry-pick."));
+ }
+
if (res)
return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
@@ -860,18 +886,31 @@ static int read_populate_opts(struct replay_opts *opts)
return 0;
}
-static int walk_revs_populate_todo(struct commit_list **todo_list,
+static int walk_revs_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
{
+ enum todo_command command = opts->action == REPLAY_PICK ?
+ TODO_PICK : TODO_REVERT;
+ const char *command_string = todo_command_strings[command];
struct commit *commit;
- struct commit_list **next;
if (prepare_revs(opts))
return -1;
- next = todo_list;
- while ((commit = get_revision(opts->revs)))
- next = commit_list_append(commit, next);
+ while ((commit = get_revision(opts->revs))) {
+ struct todo_item *item = append_new_todo(todo_list);
+ const char *commit_buffer = get_commit_buffer(commit, NULL);
+ const char *subject;
+ int subject_len;
+
+ item->command = command;
+ item->commit = commit;
+ item->offset_in_buf = todo_list->buf.len;
+ subject_len = find_commit_subject(commit_buffer, &subject);
+ strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
+ short_commit_name(commit), subject_len, subject);
+ unuse_commit_buffer(commit, commit_buffer);
+ }
return 0;
}
@@ -979,30 +1018,22 @@ static int sequencer_rollback(struct replay_opts *opts)
return -1;
}
-static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
{
static struct lock_file todo_lock;
- struct strbuf buf = STRBUF_INIT;
- int fd;
+ const char *todo_path = get_todo_path(opts);
+ int next = todo_list->current, offset, fd;
- fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
+ fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
if (fd < 0)
- return error_errno(_("Could not lock '%s'"),
- git_path_todo_file());
- if (format_todo(&buf, todo_list, opts) < 0) {
- strbuf_release(&buf);
- return error(_("Could not format %s."), git_path_todo_file());
- }
- if (write_in_full(fd, buf.buf, buf.len) < 0) {
- strbuf_release(&buf);
- return error_errno(_("Could not write to %s"),
- git_path_todo_file());
- }
- if (commit_lock_file(&todo_lock) < 0) {
- strbuf_release(&buf);
- return error(_("Error wrapping up %s."), git_path_todo_file());
- }
- strbuf_release(&buf);
+ return error_errno(_("Could not lock '%s'"), todo_path);
+ offset = next < todo_list->nr ?
+ todo_list->items[next].offset_in_buf : todo_list->buf.len;
+ if (write_in_full(fd, todo_list->buf.buf + offset,
+ todo_list->buf.len - offset) < 0)
+ return error_errno(_("Could not write to '%s'"), todo_path);
+ if (commit_lock_file(&todo_lock) < 0)
+ return error(_("Error wrapping up %s."), todo_path);
return 0;
}
@@ -1041,9 +1072,8 @@ static int save_opts(struct replay_opts *opts)
return res;
}
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
{
- struct commit_list *cur;
int res;
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -1053,10 +1083,12 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
if (read_and_refresh_cache(opts))
return -1;
- for (cur = todo_list; cur; cur = cur->next) {
- if (save_todo(cur, opts))
+ while (todo_list->current < todo_list->nr) {
+ struct todo_item *item = todo_list->items + todo_list->current;
+ if (save_todo(todo_list, opts))
return -1;
- res = do_pick_commit(cur->item, opts);
+ res = do_pick_commit(item->command, item->commit, opts);
+ todo_list->current++;
if (res)
return res;
}
@@ -1081,38 +1113,46 @@ static int continue_single_pick(void)
static int sequencer_continue(struct replay_opts *opts)
{
- struct commit_list *todo_list = NULL;
+ struct todo_list todo_list = TODO_LIST_INIT;
+ int res;
if (!file_exists(get_todo_path(opts)))
return continue_single_pick();
- if (read_populate_opts(opts) ||
- read_populate_todo(&todo_list, opts))
+ if (read_populate_opts(opts))
return -1;
+ if ((res = read_populate_todo(&todo_list, opts)))
+ goto release_todo_list;
/* Verify that the conflict has been resolved */
if (file_exists(git_path_cherry_pick_head()) ||
file_exists(git_path_revert_head())) {
- int ret = continue_single_pick();
- if (ret)
- return ret;
+ res = continue_single_pick();
+ if (res)
+ goto release_todo_list;
}
- if (index_differs_from("HEAD", 0))
- return error_dirty_index(opts);
- todo_list = todo_list->next;
- return pick_commits(todo_list, opts);
+ if (index_differs_from("HEAD", 0)) {
+ res = error_dirty_index(opts);
+ goto release_todo_list;
+ }
+ todo_list.current++;
+ res = pick_commits(&todo_list, opts);
+release_todo_list:
+ todo_list_release(&todo_list);
+ return res;
}
static int single_pick(struct commit *cmit, struct replay_opts *opts)
{
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
- return do_pick_commit(cmit, opts);
+ return do_pick_commit(opts->action == REPLAY_PICK ?
+ TODO_PICK : TODO_REVERT, cmit, opts);
}
int sequencer_pick_revisions(struct replay_opts *opts)
{
- struct commit_list *todo_list = NULL;
+ struct todo_list todo_list = TODO_LIST_INIT;
unsigned char sha1[20];
- int i;
+ int i, res;
if (opts->subcommand == REPLAY_NONE)
assert(opts->revs);
@@ -1187,7 +1227,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
- return pick_commits(todo_list, opts);
+ res = pick_commits(&todo_list, opts);
+ todo_list_release(&todo_list);
+ return res;
}
void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v5 09/27] sequencer: strip CR from the todo script
From: Johannes Schindelin @ 2016-10-21 12:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
It is not unheard of that editors on Windows write CR/LF even if the
file originally had only LF. This is particularly awkward for exec lines
of a rebase -i todo sheet. Take for example the insn "exec echo": The
shell script parser splits at the LF and leaves the CR attached to
"echo", which leads to the unknown command "echo\r".
Work around that by stripping CR when reading the todo commands, as we
already do for LF.
This happens to fix t9903.14 and .15 in MSYS1 environments (with the
rebase--helper patches based on this patch series): the todo script
constructed in such a setup contains CR/LF thanks to MSYS1 runtime's
cleverness.
Based on a report and a patch by Johannes Sixt.
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 145de78..04fcfd8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -776,6 +776,9 @@ static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
next_p = *eol ? eol + 1 /* skip LF */ : eol;
+ if (p != eol && eol[-1] == '\r')
+ eol--; /* strip Carriage Return */
+
item = append_new_todo(todo_list);
item->offset_in_buf = p - todo_list->buf.buf;
if (parse_insn_line(item, p, eol)) {
--
2.10.1.583.g721a9e0
^ permalink raw reply related
* [PATCH v5 07/27] sequencer: refactor the code to obtain a short commit name
From: Johannes Schindelin @ 2016-10-21 12:24 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
Not only does this DRY up the code (providing a better documentation what
the code is about, as well as allowing to change the behavior in a single
place), it also makes it substantially shorter to use the same
functionality in functions to be introduced when we teach the sequencer to
process interactive-rebase's git-rebase-todo file.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
sequencer.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index fb0b94b..499f5ee 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -147,13 +147,18 @@ struct commit_message {
const char *message;
};
+static const char *short_commit_name(struct commit *commit)
+{
+ return find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
+}
+
static int get_message(struct commit *commit, struct commit_message *out)
{
const char *abbrev, *subject;
int subject_len;
out->message = logmsg_reencode(commit, NULL, get_commit_output_encoding());
- abbrev = find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
+ abbrev = short_commit_name(commit);
subject_len = find_commit_subject(out->message, &subject);
@@ -621,8 +626,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
error(opts->action == REPLAY_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
- find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV),
- msg.subject);
+ short_commit_name(commit), msg.subject);
print_advice(res == 1, opts);
rerere(opts->allow_rerere_auto);
goto leave;
--
2.10.1.583.g721a9e0
^ 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