Various fixes to the behaviour of "rebase -i" when the command got
interrupted by conflicting changes.
cf. <6b927687-cf6e-d73e-78fb-bd4f46736928@gmx.de>
* pw/rebase-i-after-failure:
rebase -i: fix adding failed command to the todo list
rebase --continue: refuse to commit after failed command
rebase: fix rewritten list for failed pick
sequencer: factor out part of pick_commits()
sequencer: use rebase_path_message()
rebase -i: remove patch file after conflict resolution
rebase -i: move unlink() calls
"git for-each-ref --sort='contents:size'" sorts the refs according
to size numerically, giving a ref that points at a blob twelve-byte
(12) long before showing a blob hundred-byte (100) long.
* ks/ref-filter-sort-numerically:
ref-filter: sort numerically when ":size" is used
"git diff --no-such-option" and other corner cases around the exit
status of the "diff" command has been corrected.
* jk/diff-result-code-cleanup:
diff: drop useless "status" parameter from diff_result_code()
diff: drop useless return values in git-diff helpers
diff: drop useless return from run_diff_{files,index} functions
diff: die when failing to read index in git-diff builtin
diff: show usage for unknown builtin_diff_files() options
diff-files: avoid negative exit value
diff: spell DIFF_INDEX_CACHED out when calling run_diff_index()
transfer.unpackLimit ought to be used as a fallback, but overrode
fetch.unpackLimit and receive.unpackLimit instead.
* ts/unpacklimit-config-fix:
transfer.unpackLimit: fetch/receive.unpackLimit takes precedence
"git diff -w --exit-code" with various options did not work
correctly, which is being addressed.
* jc/diff-exit-code-with-w-fixes:
diff: the -w option breaks --exit-code for --raw and other output modes
t4040: remove test that succeeded for a wrong reason
diff: teach "--stat -w --exit-code" to notice differences
diff: mode-only change should be noticed by "--patch -w --exit-code"
diff: move the fallback "--exit-code" code down
The commit-graph verification code that detects mixture of zero and
non-zero generation numbers has been updated.
* tb/commit-graph-verify-fix:
commit-graph: avoid repeated mixed generation number warnings
t/t5318-commit-graph.sh: test generation zero transitions during fsck
commit-graph: verify swapped zero/non-zero generation cases
commit-graph: introduce `commit_graph_generation_from_graph()`
Overly long label names used in the sequencer machinery are now
chopped to fit under filesystem limitation.
* mp/rebase-label-length-limit:
rebase: allow overriding the maximal length of the generated labels
sequencer: truncate labels to accommodate loose refs
Tests with LSan from time to time seem to emit harmless message
that makes our tests unnecessarily flakey; we work it around by
filtering the uninteresting output.
* jk/test-lsan-denoise-output:
test-lib: ignore uninteresting LSan output
When rebasing commands are moved from the todo list in "git-rebase-todo"
to the "done" file (which is used by "git status" to show the recently
executed commands) just before they are executed. This means that if a
command fails because it would overwrite an untracked file it has to be
added back into the todo list before the rebase stops for the user to
fix the problem.
Unfortunately when a failed command is added back into the todo list the
command preceding it is erroneously appended to the "done" file. This
means that when rebase stops after "pick B" fails the "done" file
contains
pick A
pick B
pick A
instead of
pick A
pick B
This happens because save_todo() updates the "done" file with the
previous command whenever "git-rebase-todo" is updated. When we add the
failed pick back into "git-rebase-todo" we do not want to update
"done". Fix this by adding a "reschedule" parameter to save_todo() which
prevents the "done" file from being updated when adding a failed command
back into the "git-rebase-todo" file. A couple of the existing tests are
modified to improve their coverage as none of them trigger this bug or
check the "done" file.
Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If a commit cannot be picked because it would overwrite an untracked
file then "git rebase --continue" should refuse to commit any staged
changes as the commit was not picked. This is implemented by refusing to
commit if the message file is missing. The message file is chosen for
this check because it is only written when "git rebase" stops for the
user to resolve merge conflicts.
Existing commands that refuse to commit staged changes when continuing
such as a failed "exec" rely on checking for the absence of the author
script in run_git_commit(). This prevents the staged changes from being
committed but prints
error: could not open '.git/rebase-merge/author-script' for
reading
before the message about not being able to commit. This is confusing to
users and so checking for the message file instead improves the user
experience. The existing test for refusing to commit after a failed exec
is updated to check that we do not print the error message about a
missing author script anymore.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git rebase keeps a list that maps the OID of each commit before it was
rebased to the OID of the equivalent commit after the rebase. This list
is used to drive the "post-rewrite" hook that is called at the end of a
successful rebase. When a rebase stops for the user to resolve merge
conflicts the OID of the commit being picked is written to
".git/rebase-merge/stopped-sha". Then when the rebase is continued that
OID is added to the list of rewritten commits. Unfortunately if a commit
cannot be picked because it would overwrite an untracked file we still
write the "stopped-sha1" file. This means that when the rebase is
continued the commit is added into the list of rewritten commits even
though it has not been picked yet.
Fix this by not calling error_with_patch() for failed commands. The pick
has failed so there is nothing to commit and therefore we do not want to
set up the state files for committing staged changes when the rebase
continues. This change means we no-longer write a patch for the failed
command or display the error message printed by error_with_patch(). As
the command has failed the patch isn't really useful and in any case the
user can inspect the commit associated with the failed command by
inspecting REBASE_HEAD. Unless the user has disabled it we already print
an advice message that is more helpful than the message from
error_with_patch() which the user will still see. Even if the advice is
disabled the user will see the messages from the merge machinery
detailing the problem.
The code to add a failed command back into the todo list is duplicated
between pick_one_commit() and the loop in pick_commits(). Both sites
print advice about the command being rescheduled, decrement the current
item and save the todo list. To avoid duplicating this code
pick_one_commit() is modified to set a flag to indicate that the command
should be rescheduled in the main loop. This simplifies things as only
the remaining copy of the code needs to be modified to set REBASE_HEAD
rather than calling error_with_patch().
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a rebase stops for the user to resolve conflicts it writes a patch
for the conflicting commit to .git/rebase-merge/patch. This file has
been written since the introduction of "git-rebase-interactive.sh" in
1b1dce4bae (Teach rebase an interactive mode, 2007-06-25). I assume the
idea was to enable the user inspect the conflicting commit in the same
way as they could for the patch based rebase. This file should be
deleted when the rebase continues as if the rebase stops for a failed
"exec" command or a "break" command it is confusing to the user if there
is a stale patch lying around from an unrelated command. As the path is
now used in two different places rebase_path_patch() is added and used
to obtain the path for the patch.
To construct the path write_patch() previously used get_dir() which
returns different paths depending on whether we're rebasing or
cherry-picking/reverting. As this function is only called when
rebasing it is safe to use a hard coded string for the directory
instead. An assertion is added to make sure we don't starting calling
this function when cherry-picking in the future.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Atoms like "raw" and "contents" have a ":size" option which can be used
to know the size of the data. Since these atoms have the cmp_type
FIELD_STR, they are sorted alphabetically from 'a' to 'z' and '0' to
'9'. Meaning, even when the ":size" option is used and what we
ultimatlely have is numbers, we still sort alphabetically.
For example, consider the the following case in a repo
refname contents:size raw:size
======= ============= ========
refs/heads/branch1 1130 1210
refs/heads/master 300 410
refs/tags/v1.0 140 260
Sorting with "--format="%(refname) %(contents:size) --sort=contents:size"
would give
refs/heads/branch1 1130
refs/tags/v1.0.0 140
refs/heads/master 300
which is an alphabetic sort, while what one might really expect is
refs/tags/v1.0.0 140
refs/heads/master 300
refs/heads/branch1 1130
which is a numeric sort (that is, a "$ sort -n file" as opposed to a
"$ sort file", where "file" contains only the "contents:size" or
"raw:size" info, each of which is on a newline).
Same is the case with "--sort=raw:size".
So, sort numerically whenever the sort is done with "contents:size" or
"raw:size" and do it the normal alphabetic way when "contents" or "raw"
are used with some other option (they are FIELD_STR anyways).
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When t5583-push-branches.sh was originally introduced via 425b4d7f47
(push: introduce '--branches' option, 2023-05-06), it was not leak-free.
In fact, the test did not even run correctly until 022fbb655d (t5583:
fix shebang line, 2023-05-12), but after applying that patch, we see a
failure at t5583.8:
==2529087==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 384 byte(s) in 1 object(s) allocated from:
#0 0x7fb536330986 in __interceptor_realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
#1 0x55e07606cbf9 in xrealloc wrapper.c:140
#2 0x55e075fb6cb3 in prio_queue_put prio-queue.c:42
#3 0x55e075ec81cb in get_reachable_subset commit-reach.c:917
#4 0x55e075fe9cce in add_missing_tags remote.c:1518
#5 0x55e075fea1e4 in match_push_refs remote.c:1665
#6 0x55e076050a8e in transport_push transport.c:1378
#7 0x55e075e2eb74 in push_with_options builtin/push.c:401
#8 0x55e075e2edb0 in do_push builtin/push.c:458
#9 0x55e075e2ff7a in cmd_push builtin/push.c:702
#10 0x55e075d8aaf0 in run_builtin git.c:452
#11 0x55e075d8af08 in handle_builtin git.c:706
#12 0x55e075d8b12c in run_argv git.c:770
#13 0x55e075d8b6a0 in cmd_main git.c:905
#14 0x55e075e81f07 in main common-main.c:60
#15 0x7fb5360ab6c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#16 0x7fb5360ab784 in __libc_start_main_impl ../csu/libc-start.c:360
#17 0x55e075d88f40 in _start (git+0x1ff40) (BuildId: 38ad998b85a535e786129979443630d025ec2453)
SUMMARY: LeakSanitizer: 384 byte(s) leaked in 1 allocation(s).
This leak was addressed independently via 68b51172e3 (commit-reach: fix
memory leak in get_reachable_subset(), 2023-06-03), which makes t5583
leak-free.
But t5583 was not in the tree when 68b51172e3 was written, and the two
only met after the latter was merged back in via 693bde461c (Merge
branch 'mh/commit-reach-get-reachable-plug-leak', 2023-06-20).
At that point, t5583 was leak-free. Let's mark it as such accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This test was leak-free when t3321 was originally introduced, but never
marked as such:
$ rev="$(git log --format='%H' --reverse -1 HEAD^ -- t/t3321-notes-stripspace.sh)"
$ git checkout $rev
$ make SANITIZE=leak
[...]
$ make -C t GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=--immediate t3321-notes-stripspace.sh
[...]
# passed all 27 test(s)
1..27
# faking up non-zero exit with --invert-exit-code
make: *** [Makefile:66: t3321-notes-stripspace.sh] Error 1
make: Leaving directory '/home/ttaylorr/src/git/t'
Mark this test as leak-free accordingly.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the topic merged via 5a4f8381b6 (Merge branch
'ab/mark-leak-free-tests', 2021-10-25), a handful of tests in the suite
were marked as leak-free.
Since then, a handful of tests have become leak-free due to changes like
- 861c56f6f9 (branch: fix a leak in setup_tracking, 2023-06-11), and
- 866b43e644 (do_read_index(): always mark index as initialized unless
erroring out, 2023-06-29)
, but weren't updated at the time to mark themselves as such. This leads
to test "failures" when running:
$ make SANITIZE=leak
$ make -C t \
GIT_TEST_PASSING_SANITIZE_LEAK=check \
GIT_TEST_SANITIZE_LEAK_LOG=true \
GIT_TEST_OPTS=-vi test
This patch closes those gaps by exporting TEST_PASSES_SANITIZE_LEAK=true
before sourcing t/test-lib.sh on most remaining leak-free tests.
There are a couple of other tests which are similarly leak-free, but not
included in the list of tests touched by this patch. The remaining tests
will be addressed in the subsequent two patches.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When I run the tests in leak-checking mode the same way our CI job does,
like:
make SANITIZE=leak \
GIT_TEST_PASSING_SANITIZE_LEAK=true \
GIT_TEST_SANITIZE_LEAK_LOG=true \
test
then LSan can racily produce useless entries in the log files that look
like this:
==git==3034393==Unable to get registers from thread 3034307.
I think they're mostly harmless based on the source here:
7e0a52e8e9/compiler-rt/lib/lsan/lsan_common.cpp (L414)
which reads:
PtraceRegistersStatus have_registers =
suspended_threads.GetRegistersAndSP(i, ®isters, &sp);
if (have_registers != REGISTERS_AVAILABLE) {
Report("Unable to get registers from thread %llu.\n", os_id);
// If unable to get SP, consider the entire stack to be reachable unless
// GetRegistersAndSP failed with ESRCH.
if (have_registers == REGISTERS_UNAVAILABLE_FATAL)
continue;
sp = stack_begin;
}
The program itself still runs fine and LSan doesn't cause us to abort.
But test-lib.sh looks for any non-empty LSan logs and marks the test as
a failure anyway, under the assumption that we simply missed the failing
exit code somehow.
I don't think I've ever seen this happen in the CI job, but running
locally using clang-14 on an 8-core machine, I can't seem to make it
through a full run of the test suite without having at least one
failure. And it's a different one every time (though they do seem to
often be related to packing tests, which makes sense, since that is one
of our biggest users of threaded code).
We can hack around this by only counting LSan log files that contain a
line that doesn't match our known-uninteresting pattern.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some users have strong aversions to Scalar's opinion that the repository
should be in a 'src' directory, even though this creates a clean slate
for placing build artifacts in adjacent directories.
The new --no-src option allows users to opt out of the default behavior.
While adding options, make sure the usage output by 'scalar clone -h'
reports the same as the SYNOPSIS line in Documentation/scalar.txt.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The transfer.unpackLimit configuration variable is documented to be
used only as a fallback value when the more operation-specific
fetch.unpackLimit and receive.unpackLimit variables are not set, but
the implementation had the precedence reversed. Apparently this was
broken since the transfer.unpackLimit was introduced in e28714c5
(Consolidate {receive,fetch}.unpackLimit, 2007-01-24).
Often when documentation and code have diverged for so long, we
prefer to change the documentation instead, to avoid disrupting
users. But doing so would make these weirdly unlike most other
"specific overrides general" config options. And the fact that the
bug has existed for so long without anyone noticing implies to me
that nobody really tries to mix and match them much.
Signed-off-by: Taylor Santiago <taylorsantiago@google.com>
[jc: rewrote the log message, added tests, covered receive-pack as well]
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The output from "--raw", "--name-status", and "--name-only" modes in
"git diff" does depend on and does not reflect how certain different
contents are considered equal, unlike "--patch" and "--stat" output
modes do, when used with options like "-w" (another way of thinking
about it is that it is not like we recompute the hash of the blob
after removing all whitespaces to show "git diff --raw -w" output).
But the fact that "--raw" and friends ignore "-w" is not a good
excuse for "diff --raw -w --exit-code" to also ignore the request to
report the differences with its exit status. When run without "-w",
"git diff --exit-code --raw" does report with its exit status the
differences as requested, and we should do the same when run with
"-w", too.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When validating that a commit-graph has either all zero, or all non-zero
generation numbers, we emit a warning on both the rising and falling
edge of transitioning between the two.
So if we are unfortunate enough to see a commit-graph which has a
repeating sequence of zero, then non-zero generation numbers, we'll
generate many warnings that contain more or less the same information.
Avoid this by keeping track of a single example for a commit with zero-
and non-zero generation, and emit a single warning at the end of
verification if both are non-NULL.
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The second test called "detect incorrect generation number" asserts that
we correctly warn during an fsck when we see a non-zero generation
number after seeing a zero beforehand.
The other transition (going from non-zero to zero) was previously
untested. Test both directions, and rename the existing test to make
clear which direction it is exercising.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 2ee11f7261 (commit-graph: return generation from memory, 2023-03-20),
the `commit_graph_generation()` function stopped returning zeros when
asked to locate the generation number of a given commit.
This was done at the time to prepare for a later change which set
generation values in memory, meaning that we could no longer rely on
`graph_pos` alone to tell us whether or not to trust the generation
number returned by this function.
In 2ee11f7261, it was noted that this change only impacted very old
commit-graphs, which were written with all commits having generation
number 0. Indeed, zero is not a valid generation number, so we should
never expect to see that value outside of the aforementioned case.
The test fallout in 2ee11f7261 indicated that we were no longer able to
fsck a specific old case of commit-graph corruption, where we see a
non-zero generation number after having seen a generation number of 0
earlier.
Introduce a variant of `commit_graph_generation()` which behaves like
that function did prior to 2ee11f7261, known as
`commit_graph_generation_from_graph()`. Then use this function in the
context of `verify_one_commit_graph()`, where we only want to trust the
values from the graph.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The git-diff command has many modes (comparing worktree to index, index
to HEAD, individual blobs, etc). As a result, it dispatches to many
helper functions and cannot completely parse its options until we're in
those helper functions.
Most of them, when seeing an unknown option, exit immediately by calling
usage(). But builtin_diff_files(), which is the default if no revision
or blob arguments are given, instead prints an error() and returns -1.
One obvious shortcoming here is that the user doesn't get to see the
usual usage message. But there's a much more important bug: the -1
return is fed to diff_result_code(), which is not ready to handle it.
By default, it passes the code along as an exit code. We try to avoid
negative exit codes because they get converted to unsigned values, but
it should at least consistently show up as non-zero (i.e., a failure).
But much worse is that when --exit-code is in effect, diff_result_code()
will _ignore_ the status passed in by the caller, and instead only
report on whether the diff found changes. It didn't, of course, because
we never ran the diff, and the program unexpectedly exits with success!
We can fix this bug by just calling usage(), like the other helpers do.
Another option would of course be to teach diff_result_code() to handle
this value. But as we'll see in the next few patches, it can be cleaned
up even further. Let's just fix this bug directly to start with.
Reported-by: Romain Chossart <romainchossart@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"diff-tree -b --exit-code" without "--patch" exits with 0 status,
not because it finds that the two input files are equivalent while
ignoring whitespaces, but because the implied "--raw" mode always
exits with 0 when whitespace tweaking options like "-b" and "-w"
are given, which is a long-standing bug.
We are about to fix it so that "--raw" and friends report the
differences with the exit status (even though they ignore the
whitespace tweaking options when producing their output), which
will make this test, which succeeded for a wrong reason, start
failing. Remove it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When options like "-w" is used while "--exit-code" option is in
effect, instead of the usual "do we have any filepair whose preimage
and postimage have different <mode,object>?" check, we need to compare
the contents of the blobs, taking into account that certain changes
are considered no-op.
With the previous step, we taught "--patch" codepath to set the
.found_changes bit correctly, even for a change that only affects
the mode and not object. The "--stat" codepath, however, did not
set the .found_changes bit at all. This lead to
$ git diff --stat -w --exit-code
for a change that does have an output to exit with status 0.
Set the bit by inspecting the list of paths the diffstat output is
given for (a mode-only change will still appear as a "0-line added
0-line deleted" change) to fix it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The codepath to notice the content-level changes, taking certain
no-op changes like "ignore whitespace" into account, forgot that
a mode-only change is still a change. This resulted in
$ git diff --patch --exit-code -w
to exit with status 0 even when there is such a mode-only change,
breaking both "--patch" and "--quiet" output formats.
Teach the builtin_diff() codepath that creation and deletion as well
as mode changes are all interesting changes.
Note that the test specifically checks removal of an empty file,
because if there is anything in the preimage (i.e. the removed file
is not empty), the removal would still trigger textual patch output
and the codepath for that does update .found_changes bit to report
that it found an interesting change. We need to make sure that the
.found_changes bit is set even without triggering textual patch
output.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust to newer Term::ReadLine to prevent it from breaking
the interactive prompt code in send-email.
* jk/send-email-with-new-readline:
send-email: avoid creating more than one Term::ReadLine object
send-email: drop FakeTerm hack
This fixes an occasional hang I see when running t4053 with
--verbose-log using dash.
Commit 1e3f26542a (diff --no-index: support reading from named pipes,
2023-07-05) added a test that "diff --no-index" will complain when
comparing a named pipe and a directory. The minimum we need to test this
is to mkfifo the pipe, and then run "git diff --no-index pipe some_dir".
But the test does one thing more: it spawns a background shell process
that opens the pipe for writing, like this:
{
(>pipe) &
} &&
This extra writer _could_ be useful if Git misbehaves and tries to open
the pipe for reading. Without the writer, Git would block indefinitely
and the test would never end. But since we do not have such a bug, Git
does not open the pipe and it is the writing process which will block
indefinitely, since there are no readers. The test addresses this by
running "kill $!" in a test_when_finished block. Since the writer should
be blocking forever, this kill command will reliably find it waiting.
However, this seems to be somewhat racy, in that the writing process
sometimes hangs around even after the "kill". In a normal run of the
test script without options, this doesn't have any effect; the
main test script completes anyway. But with --verbose-log, we spawn a
"tee" process that reads the script output, and it won't end until all
descriptors pointing to its input pipe are closed. And the background
process that is hanging around still has its stderr, etc, pointed into
that pipe.
You can reproduce the situation like this:
cd t
./t4053-diff-no-index.sh --verbose-log --stress
Let that run for a few minutes, and then you'll find that some of the
runs have hung. For example, at 11:53, I ran:
$ ps xk start o pid,start,command | grep tee | head
713459 11:48:06 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-9.out
713527 11:48:06 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-15.out
719434 11:48:07 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-1.out
728117 11:48:08 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-5.out
738738 11:48:09 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-31.out
739457 11:48:09 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-27.out
744432 11:48:10 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-21.out
744471 11:48:10 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-29.out
761961 11:48:12 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-0.out
812299 11:48:19 tee -a /home/peff/compile/git/t/test-results/t4053-diff-no-index.stress-8.out
All of these have been hung for several minutes. We can investigate one
and see that it's waiting to get EOF on its input:
$ strace -p 713459
strace: Process 713459 attached
read(0,
^C
Who else has that descriptor open?
$ lsof -a -p 713459 -d 0 +E
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
tee 713459 peff 0r FIFO 0,13 0t0 3943636 pipe 719203,sh,5w 719203,sh,7w 719203,sh,12w 719203,sh,13w
sh 719203 peff 5w FIFO 0,13 0t0 3943636 pipe 713459,tee,0r 719203,sh,7w 719203,sh,12w 719203,sh,13w
sh 719203 peff 7w FIFO 0,13 0t0 3943636 pipe 713459,tee,0r 719203,sh,5w 719203,sh,12w 719203,sh,13w
sh 719203 peff 12w FIFO 0,13 0t0 3943636 pipe 713459,tee,0r 719203,sh,5w 719203,sh,7w 719203,sh,13w
sh 719203 peff 13w FIFO 0,13 0t0 3943636 pipe 713459,tee,0r 719203,sh,5w 719203,sh,7w 719203,sh,12w
It's a shell, presumably a subshell spawned by the main script. Though
it may seem odd, having the same descriptor open several times is not
unreasonable (they're all basically the original stdout/stderr of the
script that has been copied). And they should all close when the process
exits. So what's it doing? Curiously, it will exit as soon as we strace
it:
$ strace -s 64 -p 719203
strace: Process 719203 attached
openat(AT_FDCWD, "pipe", O_WRONLY|O_CREAT|O_TRUNC, 0666) = -1 ENOENT (No such file or directory)
write(2, "./t4053-diff-no-index.sh: 7: eval: ", 35) = 35
write(2, "cannot create pipe: Directory nonexistent", 41) = 41
write(2, "\n", 1) = 1
exit_group(2) = ?
+++ exited with 2 +++
I think what happens is this:
- it is blocking in the openat() call for the pipe, as we expect (so
this is definitely the backgrounded subshell mentioned above)
- strace sends signals (probably STOP/CONT); those cause the kernel to
stop blocking, but libc will restart the system call automatically
- by this time, the "pipe" fifo is gone, so we'll actually try to
create a regular file. But of course the surrounding directory is
gone, too! So we get ENOENT, and then exit as normal.
So the blocking is something we expect to happen. But what we didn't
expect is for the process to still exist at all! It should have been
killed earlier when the parent process called "kill", but it wasn't. And
we can't catch the race at this point, because it happened much earlier.
One can guess, though, that there is some race with the shell setting up
the signal handling in the backgrounded subshell, and possibly blocking
or ignoring signals at the time that the "kill" is received. Curiously,
the race does not seem to happen if I use "bash" instead of "dash", so
presumably bash's setup here is more atomic.
One fix might be to try killing the subshell more aggressively, either
using SIGKILL, or looping on kill/wait. But that seems complex and
likely to introduce new problems/races. Instead, we can observe that the
writer is not needed at all. Git will notice the pipe via stat() before
it is ever opened. So we can simply drop the writer subshell entirely.
If we ever changed Git to open the path and fstat() it, this would
result in the test hanging. But we're not likely to do that. After all,
we have to stat() paths to see if they are openable at all (e.g., it
could be a directory), so this seems like a low risk. And anybody who
does make such a change will immediately see the issue, as Git would
hang consistently.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With this change, users can override the compiled-in default for the
maximal length of the label names generated by `git rebase
--rebase-merges`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Mark Ruvald Pedersen <mped@demant.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The test 'diff --no-index reads from pipes' starts a couple of
background processes that write to the pipes that are passed to "diff
--no-index". If the test passes then we expect these processes to exit
as all their output will have been read. However if the test fails
then we want to make sure they do not hang about on the users machine
and the test remembers they should be killed by calling
test_when_finished "! kill $!"
after each background process is created. Unfortunately there is a
race where test_when_finished may run before the background process
exits even when all its output has been read resulting in the kill
command succeeding which causes the test to fail. Fix this by ignoring
the exit status of the kill command. If the diff is successful we
could instead wait for the background process to exit and check their
status but that feels like it is testing the platform's printf
implementation rather than git's code.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git rebase -i" with a series of squash/fixup, when one of the
steps stopped in conflicts and ended up getting skipped, did not
handle the accumulated commit log messages, which has been
corrected.
* pw/rebase-skip-commit-message-fix:
rebase --skip: fix commit message clean up when skipping squash
Exclude "." from the set of characters to be removed from the
beginning and the end of the human-readable name.
* bc/ident-dot-is-no-longer-crud-letter:
ident: don't consider '.' a crud
This was added by 3ece9bf0f9 (send-email: clear the $message_id after
validation, 2023-05-17) for no apparent reason, as this is required only
in cases when git's stdin is (must be) redirected, which isn't the case
here.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When moving a directory onto another with `git mv` various checks are
performed. One of of these validates that the destination is not existing.
When calling `lstat` on the destination path and it fails as the path
doesn't exist, some environments seem to overwrite the passed in
`stat` memory nonetheless (I observed this issue on debian 12 of x86_64,
running on OrbStack on ARM, emulated with Rosetta).
This would affect the code that followed as it would still acccess a now
modified `st` structure, which now seems to contain uninitialized memory.
`S_ISDIR(st_dir_mode)` would then typically return false causing the code
to run into a bad case.
The fix avoids overwriting the existing `st` structure, providing an
alternative that exists only for that purpose.
Note that this patch minimizes complexity instead of stack-frame size.
Signed-off-by: Sebastian Thiel <sebastian.thiel@icloud.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Every time git-send-email calls its ask() function to prompt the user,
we call term(), which instantiates a new Term::ReadLine object. But in
v1.46 of Term::ReadLine::Gnu (which provides the Term::ReadLine
interface on some platforms), its constructor refuses to create a second
instance[1]. So on systems with that version of the module, most
git-send-email instances will fail (as we usually prompt for both "to"
and "in-reply-to" unless the user provided them on the command line).
We can fix this by keeping a single instance variable and returning it
for each call to term(). In perl 5.10 and up, we could do that with a
"state" variable. But since we only require 5.008, we'll do it the
old-fashioned way, with a lexical "my" in its own scope.
Note that the tests in t9001 detect this problem as-is, since the
failure mode is for the program to die. But let's also beef up the
"Prompting works" test to check that it correctly handles multiple
inputs (if we had chosen to keep our FakeTerm hack in the previous
commit, then the failure mode would be incorrectly ignoring prompts
after the first).
[1] For discussion of why multiple instances are forbidden, see:
https://github.com/hirooih/perl-trg/issues/16
Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
By necessity, this script needs to verify that certain Git output
matches expectations, including text indented with spaces instead of
tabs.
Most recently, such a check was introduced in 448abbba63 (short help:
allow multi-line opthelp, 2023-07-18) which is reported by `git diff
--check 448abbba6347^!` as having whitespace issues.
Let's not complain about this because it is intentional.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Acked-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git branch -f X" to repoint the branch X said that X was "checked
out" in another worktree, even when branch X was not and instead
being bisected or rebased. The message was reworded to say the
branch was "in use".
* jc/branch-in-use-error-message:
branch: update the message to refuse touching a branch in-use
"git blame --contents=file" has been taught to work in a bare
repository.
* hy/blame-in-bare-with-contents:
blame: allow --contents to work with bare repo
Command line parser fix, and a small parse-options API update.
* jc/parse-options-short-help:
short help: allow a gap smaller than USAGE_GAP
remote: simplify "remote add --tags" help text
short help: allow multi-line opthelp
Fix tests with unportable regex patterns.
* ja/worktree-orphan-fix:
t2400: rewrite regex to avoid unintentional PCRE
builtin/worktree.c: convert tab in advice to space
t2400: drop no-op `--sq` from rev-parse call
During a series of "fixup" and/or "squash" commands, the interactive
rebase accumulates a commit message from all the commits that are being
squashed together. If one of the commits has conflicts when it is picked
and the user chooses to skip that commit then we need to remove that
commit's message from accumulated messages. To do this 15ef69314d
(rebase --skip: clean up commit message after a failed fixup/squash,
2018-04-27) updated commit_staged_changes() to reset the accumulated
message to the commit message of HEAD (which does not contain the
message from the skipped commit) when the last command was "fixup" or
"squash" and there are no staged changes. Unfortunately the code to do
this contains two bugs.
(1) If parse_head() fails we pass an invalid pointer to
unuse_commit_buffer().
(2) The reconstructed message uses the entire commit buffer from HEAD
including the headers, rather than just the commit message.
The first issue is fixed by splitting up the "if" condition into several
statements each with its own error handling. The second issue is fixed
by finding the start of the commit message within the commit buffer
using find_commit_subject().
The existing test added by 15ef69314d is modified to show the effect of
this bug. The bug is triggered when skipping the first command in the
chain (as the test does before this commit) but the effect is hidden
because opts->current_fixup_count is set to zero which leads
update_squash_messages() to recreate the squash message file from
scratch overwriting the bad message created by
commit_staged_changes(). The test is also updated to explicitly check
the commit messages rather than relying on grep to ensure they do not
contain any stray commit headers.
To check the commit message the function test_commit_message() is moved
from t3437-rebase-fixup-options.sh to test-lib.sh. As the function is
now publicly available it is updated to provide better error detection
and avoid overwriting the commonly used files "actual" and "expect".
Support for reading the expected commit message from stdin is also
added.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we process a user's name (as in user.name), we strip all
leading and trailing crud from it. Right now, we consider a dot
a crud character, and strip it off.
However, this is unsuitable for many personal names because humans
frequently have abbreviated suffixes, such as "Jr." or "Sr." at the end
of their names, and this corrupts them. Some other users may wish to
use an abbreviated name or initial, which will pose a problem especially
in cultures that write the family name first, followed by the personal
name.
Since the current approach causes lots of practical problems, let's
avoid it by no longer considering a dot to be crud.
Note that "." in the name forces the entire name to be quoted to
please mailers, but stripping "." only at the beginning and the end
does not help a name with "." in the middle (like "brian m. carlson")
so this change will not make it much worse. A name like "Given
Family, Jr." that did not have to be quoted now would need to be, in
order to be placed on the e-mail headers, though.
This is based on a weather-balloon patch by Jeff King sent in Aug 2021
https://lore.kernel.org/git/YSKm8Q8nyTavQaox@coredump.intra.peff.net/
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
