* Re: git-bisect reset not deleting .git/BISECT_LOG
From: Jeff King @ 2023-11-13 21:08 UTC (permalink / raw)
To: Janik Haag; +Cc: git
In-Reply-To: <bef9d5b3-bb64-4662-8952-d000872c5244@aq0.de>
On Mon, Nov 13, 2023 at 09:42:09PM +0100, Janik Haag wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> ```bash
> git init /tmp/reproduce-bisect-warning
> cd /tmp/reproduce-bisect-warning
> touch .git/BISECT_LOG
> git bisect reset
> git switch -c log
> ```
>
> What did you expect to happen? (Expected behavior)
>
> `git-bisect reset` should have deleted .git/BISECT_LOG
>
> What happened instead? (Actual behavior)
>
> .git/BISECT_LOG is still there
I don't think this is really specific to BISECT_LOG. In "bisect reset",
we'll call bisect_clean_state(), which removes a bunch of files. The
problem in your example is that "bisect reset" sees that we are not
bisecting and bails early.
Which I can kind of see, as part of the "reset" process is to reset to
the original pre-bisect commit. If it's not given on the command line,
the default is to use BISECT_START, which of course does not exist.
As a side note, "git bisect reset HEAD" will do what you want, because
it skips the BISECT_START check. But obviously that's not something
normal users should need to know about, and I think is even an
accidental side-effect of the conversion in 5e82c3dd22
(bisect--helper: `bisect_reset` shell function in C, 2019-01-02).
So really, you just want the "clean" part of "bisect reset", but not the
"reset". We could have a separate "bisect clean" that would do what you
want (clean state without trying to reset HEAD). But I don't think it
would be unreasonable to "reset" to just unconditionally clean. I think
it would probably just be a few lines in bisect_reset() to avoid the
early return, and skip the call to "checkout" when we have no branch to
go back to.
Maybe a good simple patch for somebody interested in getting into Git
development?
-Peff
^ permalink raw reply
* commit-graph paranoia performance, was Re: [ANNOUNCE] Git v2.43.0-rc1
From: Jeff King @ 2023-11-13 20:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
In-Reply-To: <xmqq8r785ev1.fsf@gitster.g>
On Thu, Nov 09, 2023 at 02:33:54AM +0900, Junio C Hamano wrote:
> * The codepath to traverse the commit-graph learned to notice that a
> commit is missing (e.g., corrupt repository lost an object), even
> though it knows something about the commit (like its parents) from
> what is in commit-graph.
> (merge 7a5d604443 ps/do-not-trust-commit-graph-blindly-for-existence later to maint).
I happened to be timing "rev-list" for an unrelated topic today, and I
noticed that this change had a rather large effect. The commit message
for 7a5d604443 claims a 30% performance regression. But that's when
using "--topo-order", and actually writing out the result.
Running "rev-list --count" on a copy of linux.git with a fully-built
commit-graph shows that the run-time doubles:
Benchmark 1: git.v2.42.1 rev-list --count HEAD
Time (mean ± σ): 658.0 ms ± 5.2 ms [User: 613.5 ms, System: 44.4 ms]
Range (min … max): 650.2 ms … 666.0 ms 10 runs
Benchmark 2: git.v2.43.0-rc1 rev-list --count HEAD
Time (mean ± σ): 1.333 s ± 0.019 s [User: 1.263 s, System: 0.069 s]
Range (min … max): 1.302 s … 1.361 s 10 runs
Summary
git.v2.42.1 rev-list --count HEAD ran
2.03 ± 0.03 times faster than git.v2.43.0-rc1 rev-list --count HEAD
Now in defense of that patch, this particular command is going to be one
of the most sensitive in terms of percent change, simply because it
isn't doing much besides walking the commits. And 650ms isn't _that_ big
in an absolute sense. But it also doesn't quite feel like nothing, even
tacked onto a command that might otherwise take 1000ms to run.
Should we default GIT_COMMIT_GRAPH_PARANOIA to "0"? Yes, some operations
might miss a breakage, but that is true of so much of Git. For day to
day commands we generally assume that the repository is not corrupted,
and avoid looking at any data we can. Other commands (like "commit-graph
verify", but maybe others) would probably want to be more careful
(either by checking this case explicitly, or by enabling the paranoia
flag themselves).
-Peff
^ permalink raw reply
* git-bisect reset not deleting .git/BISECT_LOG
From: Janik Haag @ 2023-11-13 20:42 UTC (permalink / raw)
To: git
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
```bash
git init /tmp/reproduce-bisect-warning
cd /tmp/reproduce-bisect-warning
touch .git/BISECT_LOG
git bisect reset
git switch -c log
```
What did you expect to happen? (Expected behavior)
`git-bisect reset` should have deleted .git/BISECT_LOG
What happened instead? (Actual behavior)
.git/BISECT_LOG is still there
What's different between what you expected and what actually happened?
git switch and some external tools like shell prompts are relying on
that file to know if we are doing a bisect so switch printed a warning
that we are still bisecting. Funnily enough git checkout doesn't print
that warning.
Anything else you want to add:
In case you are wondering why I created the .git/BISECT_LOG file with
touch instead of using the git cli, I forgot how I ended up there in the
first place that was like a year ago and the warning didn't really
bother me, today someone told me to look for anything in .git namend
BISECT* and after deleting the log by hand it worked. So I figured
git-bisect reset should probably have done that after I ended up in that
unkown state. the content I had in BISECT_LOG before deleting it was:
```
# good: [e24028141f2] qdmr: fixup
git bisect good e24028141f278590407dd5389330f23d01c89cff
```
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path:
/nix/store/lf0wpjrj8yx4gsmw2s3xfl58ixmqk8qa-bash-5.2-p15/bin/bash
uname: Linux 6.1.61 #1-NixOS SMP PREEMPT_DYNAMIC Thu Nov 2 08:35:33 UTC
2023 x86_64
compiler info: gnuc: 12.3
libc info: glibc: 2.38
$SHELL (typically, interactive shell): /run/current-system/sw/bin/zsh
[Enabled Hooks]
^ permalink raw reply
* Re: rev-list default order sometimes very slow (size and metadata dependent)
From: Jeff King @ 2023-11-13 20:39 UTC (permalink / raw)
To: Kevin Lyles; +Cc: git@vger.kernel.org, allred.sean@gmail.com
In-Reply-To: <DM6PR17MB247594CF3A0511EF95893935AEADA@DM6PR17MB2475.namprd17.prod.outlook.com>
On Sat, Nov 11, 2023 at 04:17:17PM +0000, Kevin Lyles wrote:
> If the archive branch is created with author/commit dates older than the
> rest of the repository, we're able to run:
>
> $ git rev-list --count --all
>
> in ~9-10 seconds on a mirror clone with a commit-graph. However, if the
> archive branch is instead created with author/commit dates newer than the
> rest of the repository, it takes 4-5 minutes.
Thanks for providing a reproduction script. I had trouble following the
text explanation, but I can easily reproduce the issue using your
script.
Running the slow case under perf, it looks like we are spending most of
the time in commit_list_insert_by_date(). Which makes sense. The queue
of commits to visit during the traversal is kept as an ordered linked
list that uses linear search for the insertion. So the worst case to
insert N commits is quadratic.
In practice, it tends not to be a big problem because we visit commits
in roughly reverse-timestamp order as we traverse. So it's more like
O(N*width), where "width" is the average number of simultaneous lines of
history. And your archive graph is...very wide.
Playing with the commit timestamps helps your case because it just
happens to reorder the queue in such a way that we put off looking at
those huge archive merges until much later in the traversal, when our
remaining "N" is much smaller.
The right solution is obviously to switch to a better data structure.
And some of the infrastructure is in place from the last time we dealt
with a similar case:
https://lore.kernel.org/git/HE1PR0702MB3788FCDAB764252D9CBB42E5B0560@HE1PR0702MB3788.eurprd07.prod.outlook.com/
There we started using a priority queue instead of the linked list.
That patch stopped shorting of replacing the main revs->commits
traversal list, though, as it's referenced in a ton of places (some of
which really care about its list-like nature). Here's a hacky patch that
tries to lazily convert the list to a queue. It makes your case much
faster, but it also causes a few failures in the test suite (related to
commit ordering), so obviously it's violating some assumptions
somewhere. I'm not sure if it's a fruitful direction or not.
And as you noticed, using --topo-order does not suffer from the same
problem. It follows a completely different path, which...you guessed it,
uses a priority queue. I'm not sure what the current state of things is,
but one of the promises of commit-graphs is that with stored generation
numbers, we could compute topo-order incrementally and efficiently. So
another possible route is that we'd simply enable it by default. But I'm
not sure if there's more work that needs to be done to get there, or if
there are other downsides.
diff --git a/revision.c b/revision.c
index 00d5c29bfc..62d33dbfee 100644
--- a/revision.c
+++ b/revision.c
@@ -3119,6 +3119,7 @@ void release_revisions(struct rev_info *revs)
clear_decoration(&revs->treesame, free);
line_log_free(revs);
oidset_clear(&revs->missing_commits);
+ clear_prio_queue(&revs->commitq);
}
static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
@@ -4161,6 +4162,36 @@ static void track_linear(struct rev_info *revs, struct commit *commit)
static struct commit *get_revision_1(struct rev_info *revs)
{
+ /*
+ * This is kind of hacky. We'll dynamically move commits from the
+ * linear list over to our priority queue, which has better worst-case
+ * behavior. Doing it here serves two purposes:
+ *
+ * - we'll let all of the setup code still operate on the "commits"
+ * list, and then just move it over to the queue lazily. This
+ * probably could be done in prepare_revision_walk() or similar,
+ * but by doing it here we know we're catching all code paths.
+ *
+ * - we'll likewise catch any code which tries to add new commits to
+ * the list _during_ the traversal. The main one would be
+ * process_parents() below, which we've taught to use the queue
+ * directly. But this will catch any other random bits of code,
+ * including callers of the revision API, which add to the
+ * traversal as they go.
+ *
+ * A cleaner solution would probably be to get rid of the "commits"
+ * list entirely, but that's a much bigger task.
+ */
+ if (revs->commits && !revs->topo_order) {
+ struct commit_list *p;
+ if (!revs->unsorted_input)
+ revs->commitq.compare = compare_commits_by_commit_date;
+ for (p = revs->commits; p; p = p->next)
+ prio_queue_put(&revs->commitq, p->item);
+ free_commit_list(revs->commits);
+ revs->commits = NULL;
+ }
+
while (1) {
struct commit *commit;
@@ -4169,7 +4200,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
else if (revs->topo_walk_info)
commit = next_topo_commit(revs);
else
- commit = pop_commit(&revs->commits);
+ commit = prio_queue_get(&revs->commitq);
if (!commit)
return NULL;
@@ -4191,7 +4222,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
try_to_simplify_commit(revs, commit);
else if (revs->topo_walk_info)
expand_topo_walk(revs, commit);
- else if (process_parents(revs, commit, &revs->commits, NULL) < 0) {
+ else if (process_parents(revs, commit, NULL, &revs->commitq) < 0) {
if (!revs->ignore_missing_links)
die("Failed to traverse parents of commit %s",
oid_to_hex(&commit->object.oid));
diff --git a/revision.h b/revision.h
index 94c43138bc..4301b825a0 100644
--- a/revision.h
+++ b/revision.h
@@ -12,6 +12,7 @@
#include "ident.h"
#include "list-objects-filter-options.h"
#include "strvec.h"
+#include "prio-queue.h"
/**
* The revision walking API offers functions to build a list of revisions
@@ -124,6 +125,12 @@ struct rev_info {
struct object_array pending;
struct repository *repo;
+ /*
+ * We'll use this queue during the actual traversal, rather than
+ * adding and popping from the "commits" list.
+ */
+ struct prio_queue commitq;
+
/* Parents of shown commits */
struct object_array boundary_commits;
^ permalink raw reply related
* [PATCH v2] ci: avoid running the test suite _twice_
From: Johannes Schindelin via GitGitGadget @ 2023-11-13 20:25 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1613.git.1699894837844.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
This is a late amendment of 4a6e4b960263 (CI: remove Travis CI support,
2021-11-23), whereby the `.prove` file (being written by the `prove`
command that is used to run the test suite) is no longer retained
between CI builds: This feature was only ever used in the Travis CI
builds, we tried for a while to do the same in Azure Pipelines CI runs
(but I gave up on it after a while), and we never used that feature in
GitHub Actions (nor does the new GitLab CI code use it).
Retaining the Prove cache has been fragile from the start, even though
the idea seemed good at the time, the idea being that the `.prove` file
caches information about previous `prove` runs (`save`) and uses them
(`slow`) to run the tests in the order from longer-running to shorter
ones, making optimal use of the parallelism implied by `--jobs=<N>`.
However, using a Prove cache can cause some surprising behavior: When
the `prove` caches information about a test script it has run,
subsequent `prove` runs (with `--state=slow`) will run the same test
script again even if said script is not specified on the `prove`
command-line!
So far, this bug did not matter. Right until d8f416bbb87c (ci: run unit
tests in CI, 2023-11-09) did it not matter.
But starting with that commit, we invoke `prove` _twice_ in CI, once to
run the regular test suite of regression test scripts, and once to run
the unit tests. Due to the bug, the second invocation re-runs all of the
tests that were already run as part of the first invocation. This not
only wastes build minutes, it also frequently causes the `osx-*` jobs to
fail because they already take a long time and now are likely to run
into a timeout.
The worst part about it is that there is actually no benefit to keep
running with `--state=slow,save`, ever since we decided no longer to
try to reuse the Prove cache between CI runs.
So let's just drop that Prove option and live happily ever after.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
ci: avoid running the test suite twice
This
[https://github.com/git/git/actions/runs/6845537889/job/18614840166] is
an example of a osx-* job that times out. Here
[https://github.com/git/git/actions/runs/6845537889/job/18614840166#step:4:839],
it is running t0013, and here
[https://github.com/git/git/actions/runs/6845537889/job/18614840166#step:4:2765],
it is run again (in the middle of the entire test suite, as part of make
unit-tests).
While this tries to fix a bug uncovered by js/doc-unit-tests, to avoid
merge conflicts, this is based on ps/ci-gitlab.
Changes since v1:
* Reworded the commit message, specifically avoiding a reference to a
patch that has not been upstreamed from Git for Windows yet.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1613%2Fdscho%2Favoid-running-test-suite-twice-in-ci-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1613/dscho/avoid-running-test-suite-twice-in-ci-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1613
Range-diff vs v1:
1: c74eda36246 ! 1: 880f2c19478 ci: avoid running the test suite _twice_
@@ Metadata
## Commit message ##
ci: avoid running the test suite _twice_
- This is a late amendment of 19ec39aab54 (ci: stop linking the `prove`
- cache, 2022-07-10), fixing a bug that had been hidden so far.
-
- The bug is that the `.prove` cache stores information about previous
- `prove` runs (`save`) and uses them (`slow`, to run the tests in the
- order from longer-running to shorter ones).
-
- This bug can cause some surprising behavior: when the Prove cache
- contains a reference to a test script, subsequent `prove` runs (with
- `--state=slow`) will run the same test script again even if said script
- is not specified on the `prove` command-line!
+ This is a late amendment of 4a6e4b960263 (CI: remove Travis CI support,
+ 2021-11-23), whereby the `.prove` file (being written by the `prove`
+ command that is used to run the test suite) is no longer retained
+ between CI builds: This feature was only ever used in the Travis CI
+ builds, we tried for a while to do the same in Azure Pipelines CI runs
+ (but I gave up on it after a while), and we never used that feature in
+ GitHub Actions (nor does the new GitLab CI code use it).
+
+ Retaining the Prove cache has been fragile from the start, even though
+ the idea seemed good at the time, the idea being that the `.prove` file
+ caches information about previous `prove` runs (`save`) and uses them
+ (`slow`) to run the tests in the order from longer-running to shorter
+ ones, making optimal use of the parallelism implied by `--jobs=<N>`.
+
+ However, using a Prove cache can cause some surprising behavior: When
+ the `prove` caches information about a test script it has run,
+ subsequent `prove` runs (with `--state=slow`) will run the same test
+ script again even if said script is not specified on the `prove`
+ command-line!
So far, this bug did not matter. Right until d8f416bbb87c (ci: run unit
- tests in CI, 2023-11-09) it did not matter.
-
- But starting with that commit, we run `prove` _twice_ in CI, and with
- completely different sets of tests to run. Due to the bug, the second
- invocation re-runs all of the tests that were already run as part of the
- first invocation. This not only wastes build minutes, it also frequently
- causes the `osx-*` jobs to fail because they already take a long time
- and now are likely to run into a timeout.
+ tests in CI, 2023-11-09) did it not matter.
+
+ But starting with that commit, we invoke `prove` _twice_ in CI, once to
+ run the regular test suite of regression test scripts, and once to run
+ the unit tests. Due to the bug, the second invocation re-runs all of the
+ tests that were already run as part of the first invocation. This not
+ only wastes build minutes, it also frequently causes the `osx-*` jobs to
+ fail because they already take a long time and now are likely to run
+ into a timeout.
The worst part about it is that there is actually no benefit to keep
running with `--state=slow,save`, ever since we decided no longer to
ci/lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 6dfc90d7f53..307a8df0b5a 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -281,7 +281,7 @@ else
fi
MAKEFLAGS="$MAKEFLAGS --jobs=$JOBS"
-GIT_PROVE_OPTS="--timer --jobs $JOBS --state=failed,slow,save"
+GIT_PROVE_OPTS="--timer --jobs $JOBS"
GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
case "$CI_OS_NAME" in
base-commit: 0e3b67e2aa25edb7e1a5c999c87b52a7b3a7649a
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: Jeff King @ 2023-11-13 19:00 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Junio C Hamano, Simon Ser
In-Reply-To: <0c0d685c-29e5-462c-a743-4573a56d7e04@web.de>
On Sun, Nov 12, 2023 at 07:38:22PM +0100, René Scharfe wrote:
> > Grepping for other
> > code that does the same thing, I see that show_log() and
> > cmd_format_patch() copy a lot more.
>
> show_log() copies almost half of the struct 6d167fd7cc members
> from struct rev_info!
>
> cmd_format_patch() doesn't seem have a struct pretty_print_context,
> though...
Doh, you're right. I grepped for spots setting encode_email_headers, but
the one in cmd_format_patch() is copying it from the config-default into
the rev_info, not into the pretty-print context.
Which makes sense. It is going to call show_log() to show each commit,
which is where the value is copied into the pretty-print context.
> > (For that matter, why doesn't
> > make_cover_letter() just use the context set up by
> > cmd_format_patch()?).
>
> ... which answers this question, but did you perhaps mean a different
> function?
Right, I was just confused.
> > 2. Why are we copying this stuff at all? When we introduced the
> > pretty-print context back in 6bf139440c (clean up calling
> > conventions for pretty.c functions, 2011-05-26), the idea was just
> > to keep all of the format options together. But later, 6d167fd7cc
> > (pretty: use fmt_output_email_subject(), 2017-03-01) added a
> > pointer to the rev_info directly.
>
> Hmm. Was sticking the rev_info pointer in unwise? Does it tangle up
> things that should be kept separate? It uses force_in_body_from,
> grep_filter, sources, nr, total and subject_prefix from struct rev_info.
> That seems a lot, but is just a small fraction of its total members and
> we could just copy those we need. Or prepare the subject string and
> pass it in, as before 6d167fd7cc.
I don't know that it was unwise. I was mostly just noting the history
because that explains why we _didn't_ simply refer to ctx->revs in
6bf139440c. Has the separation between the two been valuable since then?
I'm not sure. We do have some code paths that do not have a rev_info at
all (e.g., pp_commit_easy(), which makes an ad-hoc empty context
struct).
> > So could/should we just be using
> > pp->rev->encode_email_headers here?
>
> Perhaps. If we want struct pretty_print_context to be a collection of
> named parameters, though, then it makes sense to avoid using
> complicated types to provide a nice interface to its callers, and
> struct rev_info is one of our largest structs we have.
Yeah, philosophically it may be better to keep the modules separated.
But if we end up having to just copy a ton of fields, it may not be as
practical. If we can at least factor that out into a single spot,
though, it may not be so bad.
> > Or if that field is not always set (or we want to override some
> > elements), should there be a single helper function to initialize
> > the pretty_print_context from a rev_info, that could be shared
> > between spots like show_log() and make_cover_letter()?
>
> That would help avoid forgetting to copy something. But copying is
> questionable in general, as you mentioned. Given the extent of the
> overlap, would it make sense to embed struct pretty_print_context in
> struct rev_info instead? Or a subset thereof?
I had a similar thought, but the pretty_print_context carries both input
options from the caller, as well as computed state used internally by
the pretty-print code. So you'd have to split those two up, I would
think. And now all of the pretty-print functions have to pass around
_two_ contexts.
That's more annoying, but arguably is a cleaner design (the internal
struct can be a private thing that is not even defined outside of
pretty.c). I dunno.
-Peff
^ permalink raw reply
* Re: [PATCH] ci: avoid running the test suite _twice_
From: Jeff King @ 2023-11-13 18:49 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: Josh Steadmon, Phillip Wood, git, Johannes Schindelin
In-Reply-To: <pull.1613.git.1699894837844.gitgitgadget@gmail.com>
On Mon, Nov 13, 2023 at 05:00:37PM +0000, Johannes Schindelin via GitGitGadget wrote:
> This is a late amendment of 19ec39aab54 (ci: stop linking the `prove`
> cache, 2022-07-10), fixing a bug that had been hidden so far.
We don't seem to have that commit in Junio's tree; it is only in
git-for-windows.
Not that we should not fix things if they are broken, but I am trying
to understand if git/git is experiencing the same bug. It sounds like
not yet, though from looking at 19ec39aab54, I would expect to get these
doubled runs any time we store the prove state. But maybe without that
commit our state-file symlink is going somewhere invalid, and prove
fails to actually store anything?
> But starting with that commit, we run `prove` _twice_ in CI, and with
> completely different sets of tests to run. Due to the bug, the second
> invocation re-runs all of the tests that were already run as part of the
> first invocation. This not only wastes build minutes, it also frequently
> causes the `osx-*` jobs to fail because they already take a long time
> and now are likely to run into a timeout.
>
> The worst part about it is that there is actually no benefit to keep
> running with `--state=slow,save`, ever since we decided no longer to
> try to reuse the Prove cache between CI runs.
>
> So let's just drop that Prove option and live happily ever after.
Yes, I think this is the right thing to do regardless. If we are not
saving the state to use between two related runs, there is no point
storing it in the first place.
I do have to wonder, though, as somebody who did not follow the
unit-test topic closely: why are the unit tests totally separate from
the rest of the suite? I would think we'd want them run from one or more
t/t*.sh scripts. That would make bugs like this impossible, but also:
1. They'd be run via "make test", so developers don't have to remember
to run them separately.
2. They can be run in parallel with all of the other tests when using
"prove -j", etc.
-Peff
^ permalink raw reply
* Re: [PATCH v2] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Jeff King @ 2023-11-13 18:35 UTC (permalink / raw)
To: Arthur Chan via GitGitGadget; +Cc: git, Arthur Chan
In-Reply-To: <pull.1612.v2.git.1699892568344.gitgitgadget@gmail.com>
On Mon, Nov 13, 2023 at 04:22:48PM +0000, Arthur Chan via GitGitGadget wrote:
> + str = (char *)malloc(size + 1);
> + if (!str)
> + return 0;
> + memcpy(str, data, size);
> + str[size] = '\0';
Is it important that we avoid calling die() if the malloc fails here?
The usual way to write this in our code base is just:
str = xmemdupz(data, size);
It's not entirely a style thing; we sometimes audit the code base
looking for computations on malloc sizes (for integer overflows) as well
as sites that should be using xmalloc and are not. Obviously we can
exclude oss-fuzz/ from such audits, but if there's no reason not to
prefer our usual style, it's one less thing to worry about.
-Peff
^ permalink raw reply
* Re: [PATCH v4 4/4] rebase: rewrite --(no-)autosquash documentation
From: Phillip Wood @ 2023-11-13 17:02 UTC (permalink / raw)
To: Andy Koppe, git; +Cc: gitster, newren
In-Reply-To: <20231111132720.78877-6-andy.koppe@gmail.com>
Hi Andy
On 11/11/2023 13:27, Andy Koppe wrote:
> Rewrite the description of the rebase --(no-)autosquash options to try
> to make it a bit clearer. Don't use "the '...'" to refer to part of a
> commit message, mention how --interactive can be used to review the
> todo list, and add a bit more detail on commit --squash/amend.
This version looks good to me, thanks for working on it
Best Wishes
Phillip
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
> Documentation/git-rebase.txt | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 10548e715c..1dd6555f66 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -589,21 +589,27 @@ See also INCOMPATIBLE OPTIONS below.
>
> --autosquash::
> --no-autosquash::
> - When the commit log message begins with "squash! ..." or "fixup! ..."
> - or "amend! ...", and there is already a commit in the todo list that
> - matches the same `...`, automatically modify the todo list of
> - `rebase`, so that the commit marked for squashing comes right after
> - the commit to be modified, and change the action of the moved commit
> - from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
> - matches the `...` if the commit subject matches, or if the `...` refers
> - to the commit's hash. As a fall-back, partial matches of the commit
> - subject work, too. The recommended way to create fixup/amend/squash
> - commits is by using the `--fixup`, `--fixup=amend:` or `--fixup=reword:`
> - and `--squash` options respectively of linkgit:git-commit[1].
> + Automatically squash commits with specially formatted messages into
> + previous commits being rebased. If a commit message starts with
> + "squash! ", "fixup! " or "amend! ", the remainder of the subject line
> + is taken as a commit specifier, which matches a previous commit if it
> + matches the subject line or the hash of that commit. If no commit
> + matches fully, matches of the specifier with the start of commit
> + subjects are considered.
> +
> -If the `--autosquash` option is enabled by default using the
> -configuration variable `rebase.autoSquash`, this option can be
> -used to override and disable this setting.
> +In the rebase todo list, the actions of squash, fixup and amend commits are
> +changed from `pick` to `squash`, `fixup` or `fixup -C`, respectively, and they
> +are moved right after the commit they modify. The `--interactive` option can
> +be used to review and edit the todo list before proceeding.
> ++
> +The recommended way to create commits with squash markers is by using the
> +`--squash`, `--fixup`, `--fixup=amend:` or `--fixup=reword:` options of
> +linkgit:git-commit[1], which take the target commit as an argument and
> +automatically fill in the subject line of the new commit from that.
> ++
> +Settting configuration variable `rebase.autoSquash` to true enables
> +auto-squashing by default for interactive rebase. The `--no-autosquash`
> +option can be used to override that setting.
> +
> See also INCOMPATIBLE OPTIONS below.
>
^ permalink raw reply
* Re: [PATCH v4 3/4] rebase: test autosquash with and without -i
From: Phillip Wood @ 2023-11-13 17:02 UTC (permalink / raw)
To: Andy Koppe, git; +Cc: gitster, newren
In-Reply-To: <20231111132720.78877-4-andy.koppe@gmail.com>
On 11/11/2023 13:27, Andy Koppe wrote:
> Amend t3415-rebase-autosquash.sh to test the --autosquash option and
> rebase.autoSquash config with and without -i.
Thanks for adding these tests. I'd be happy to see this squashed into
the previous commit, though that is probably not worth a re-roll on its own.
Best Wishes
Phillip
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
> t/t3415-rebase-autosquash.sh | 38 ++++++++++++++++++++++++++----------
> 1 file changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index a364530d76..fcc40d6fe1 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -43,7 +43,7 @@ test_auto_fixup () {
>
> git tag $1 &&
> test_tick &&
> - git rebase $2 -i HEAD^^^ &&
> + git rebase $2 HEAD^^^ &&
> git log --oneline >actual &&
> if test -n "$no_squash"
> then
> @@ -61,15 +61,24 @@ test_auto_fixup () {
> }
>
> test_expect_success 'auto fixup (option)' '
> - test_auto_fixup final-fixup-option --autosquash
> + test_auto_fixup fixup-option --autosquash &&
> + test_auto_fixup fixup-option-i "--autosquash -i"
> '
>
> -test_expect_success 'auto fixup (config)' '
> +test_expect_success 'auto fixup (config true)' '
> git config rebase.autosquash true &&
> - test_auto_fixup final-fixup-config-true &&
> + test_auto_fixup ! fixup-config-true &&
> + test_auto_fixup fixup-config-true-i -i &&
> test_auto_fixup ! fixup-config-true-no --no-autosquash &&
> + test_auto_fixup ! fixup-config-true-i-no "-i --no-autosquash"
> +'
> +
> +test_expect_success 'auto fixup (config false)' '
> git config rebase.autosquash false &&
> - test_auto_fixup ! final-fixup-config-false
> + test_auto_fixup ! fixup-config-false &&
> + test_auto_fixup ! fixup-config-false-i -i &&
> + test_auto_fixup fixup-config-false-yes --autosquash &&
> + test_auto_fixup fixup-config-false-i-yes "-i --autosquash"
> '
>
> test_auto_squash () {
> @@ -87,7 +96,7 @@ test_auto_squash () {
> git commit -m "squash! first" -m "extra para for first" &&
> git tag $1 &&
> test_tick &&
> - git rebase $2 -i HEAD^^^ &&
> + git rebase $2 HEAD^^^ &&
> git log --oneline >actual &&
> if test -n "$no_squash"
> then
> @@ -105,15 +114,24 @@ test_auto_squash () {
> }
>
> test_expect_success 'auto squash (option)' '
> - test_auto_squash final-squash --autosquash
> + test_auto_squash squash-option --autosquash &&
> + test_auto_squash squash-option-i "--autosquash -i"
> '
>
> -test_expect_success 'auto squash (config)' '
> +test_expect_success 'auto squash (config true)' '
> git config rebase.autosquash true &&
> - test_auto_squash final-squash-config-true &&
> + test_auto_squash ! squash-config-true &&
> + test_auto_squash squash-config-true-i -i &&
> test_auto_squash ! squash-config-true-no --no-autosquash &&
> + test_auto_squash ! squash-config-true-i-no "-i --no-autosquash"
> +'
> +
> +test_expect_success 'auto squash (config false)' '
> git config rebase.autosquash false &&
> - test_auto_squash ! final-squash-config-false
> + test_auto_squash ! squash-config-false &&
> + test_auto_squash ! squash-config-false-i -i &&
> + test_auto_squash squash-config-false-yes --autosquash &&
> + test_auto_squash squash-config-false-i-yes "-i --autosquash"
> '
>
> test_expect_success 'misspelled auto squash' '
^ permalink raw reply
* Re: [PATCH v4 2/4] rebase: support --autosquash without -i
From: Phillip Wood @ 2023-11-13 17:01 UTC (permalink / raw)
To: Andy Koppe, git; +Cc: gitster, newren
In-Reply-To: <20231111132720.78877-3-andy.koppe@gmail.com>
Hi Andy
On 11/11/2023 13:27, Andy Koppe wrote:
> The --autosquash option prevents preemptive fast-forwarding and triggers
> conflicts with amend backend options,
"amend" should be "apply". While this sentence is true I'm not quite
sure how it relates to the change in this commit.
> yet it only actually performs
> auto-squashing when combined with the --interactive (or -i) option.
>
> Remove the latter restriction and tweak the --autosquash description
> accordingly.
This seems like a reasonable change to me, thanks for working on it.
Best Wishes
Phillip
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
> Documentation/git-rebase.txt | 2 +-
> builtin/rebase.c | 4 +---
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index b4526ca246..10548e715c 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -592,7 +592,7 @@ See also INCOMPATIBLE OPTIONS below.
> When the commit log message begins with "squash! ..." or "fixup! ..."
> or "amend! ...", and there is already a commit in the todo list that
> matches the same `...`, automatically modify the todo list of
> - `rebase -i`, so that the commit marked for squashing comes right after
> + `rebase`, so that the commit marked for squashing comes right after
> the commit to be modified, and change the action of the moved commit
> from `pick` to `squash` or `fixup` or `fixup -C` respectively. A commit
> matches the `...` if the commit subject matches, or if the `...` refers
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index a73de7892b..9f8192e0a5 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -710,10 +710,8 @@ static int run_specific_rebase(struct rebase_options *opts)
> if (opts->type == REBASE_MERGE) {
> /* Run sequencer-based rebase */
> setenv("GIT_CHERRY_PICK_HELP", resolvemsg, 1);
> - if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
> + if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT))
> setenv("GIT_SEQUENCE_EDITOR", ":", 1);
> - opts->autosquash = 0;
> - }
> if (opts->gpg_sign_opt) {
> /* remove the leading "-S" */
> char *tmp = xstrdup(opts->gpg_sign_opt + 2);
^ permalink raw reply
* Re: [PATCH v4 1/4] rebase: fully ignore rebase.autoSquash without -i
From: Phillip Wood @ 2023-11-13 17:01 UTC (permalink / raw)
To: Andy Koppe, git; +Cc: gitster, newren
In-Reply-To: <20231111132720.78877-2-andy.koppe@gmail.com>
Hi Andy
On 11/11/2023 13:27, Andy Koppe wrote:
> Setting the rebase.autoSquash config variable to true implies a couple
> of restrictions: it prevents preemptive fast-forwarding and it triggers
> conflicts with amend backend options. However, it only actually results
> in auto-squashing when combined with the --interactive (or -i) option,
> due to code in run_specific_rebase() that disables auto-squashing unless
> the REBASE_INTERACTIVE_EXPLICIT flag is set.
>
> Doing autosquashing for rebase.autoSquash without --interactive would be
> problematic in terms of backward compatibility, but conversely, there is
> no need for the aforementioned restrictions without --interactive.
>
> So drop the options.config_autosquash check from the conditions for
> clearing allow_preemptive_ff, as the case where it is combined with
> --interactive is already covered by the REBASE_INTERACTIVE_EXPLICIT
> flag check above it.
>
> Also drop the "apply options are incompatible with rebase.autoSquash"
> error, because it is unreachable if it is restricted to --interactive,
> as apply options already cause an error when used with --interactive.
> Drop the tests for the error from t3422-rebase-incompatible-options.sh,
> which has separate tests for the conflicts of --interactive with apply
> options.
>
> When neither --autosquash nor --no-autosquash are given, only set
> options.autosquash to true if rebase.autosquash is combined with
> --interactive.
>
> Don't initialize options.config_autosquash to -1, as there is no need to
> distinguish between rebase.autoSquash being unset or explicitly set to
> false.
>
> Finally, amend the rebase.autoSquash documentation to say it only
> affects interactive mode.
Thanks for the well reasoned explanation of the changes. I think
documenting rebase.autosquash as only applying to interactive rebases is
a good way forward. The code changes all look sensible to me.
Best Wishes
Phillip
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
> Documentation/config/rebase.txt | 4 +++-
> builtin/rebase.c | 13 ++++++-------
> t/t3422-rebase-incompatible-options.sh | 12 ------------
> 3 files changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index 9c248accec..d59576dbb2 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -9,7 +9,9 @@ rebase.stat::
> rebase. False by default.
>
> rebase.autoSquash::
> - If set to true enable `--autosquash` option by default.
> + If set to true, enable the `--autosquash` option of
> + linkgit:git-rebase[1] by default for interactive mode.
> + This can be overridden with the `--no-autosquash` option.
>
> rebase.autoStash::
> When set to true, automatically create a temporary stash entry
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 043c65dccd..a73de7892b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -149,7 +149,6 @@ struct rebase_options {
> .reapply_cherry_picks = -1, \
> .allow_empty_message = 1, \
> .autosquash = -1, \
> - .config_autosquash = -1, \
> .rebase_merges = -1, \
> .config_rebase_merges = -1, \
> .update_refs = -1, \
> @@ -1405,7 +1404,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||
> (options.action != ACTION_NONE) ||
> (options.exec.nr > 0) ||
> - (options.autosquash == -1 && options.config_autosquash == 1) ||
> options.autosquash == 1) {
> allow_preemptive_ff = 0;
> }
> @@ -1508,8 +1506,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (is_merge(&options))
> die(_("apply options and merge options "
> "cannot be used together"));
> - else if (options.autosquash == -1 && options.config_autosquash == 1)
> - die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
> else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
> else if (options.update_refs == -1 && options.config_update_refs == 1)
> @@ -1529,10 +1525,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
> ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
>
> - if (options.autosquash == 1)
> + if (options.autosquash == 1) {
> imply_merge(&options, "--autosquash");
> - options.autosquash = (options.autosquash >= 0) ? options.autosquash :
> - ((options.config_autosquash >= 0) ? options.config_autosquash : 0);
> + } else if (options.autosquash == -1) {
> + options.autosquash =
> + options.config_autosquash &&
> + (options.flags & REBASE_INTERACTIVE_EXPLICIT);
> + }
>
> if (options.type == REBASE_UNSPECIFIED) {
> if (!strcmp(options.default_backend, "merge"))
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 2eba00bdf5..b40f26250b 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -100,12 +100,6 @@ test_rebase_am_only () {
> test_must_fail git rebase $opt --root A
> "
>
> - test_expect_success "$opt incompatible with rebase.autosquash" "
> - git checkout B^0 &&
> - test_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&
> - grep -e --no-autosquash err
> - "
> -
> test_expect_success "$opt incompatible with rebase.rebaseMerges" "
> git checkout B^0 &&
> test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
> @@ -118,12 +112,6 @@ test_rebase_am_only () {
> grep -e --no-update-refs err
> "
>
> - test_expect_success "$opt okay with overridden rebase.autosquash" "
> - test_when_finished \"git reset --hard B^0\" &&
> - git checkout B^0 &&
> - git -c rebase.autosquash=true rebase --no-autosquash $opt A
> - "
> -
> test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
> test_when_finished \"git reset --hard B^0\" &&
> git checkout B^0 &&
^ permalink raw reply
* [PATCH] ci: avoid running the test suite _twice_
From: Johannes Schindelin via GitGitGadget @ 2023-11-13 17:00 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
This is a late amendment of 19ec39aab54 (ci: stop linking the `prove`
cache, 2022-07-10), fixing a bug that had been hidden so far.
The bug is that the `.prove` cache stores information about previous
`prove` runs (`save`) and uses them (`slow`, to run the tests in the
order from longer-running to shorter ones).
This bug can cause some surprising behavior: when the Prove cache
contains a reference to a test script, subsequent `prove` runs (with
`--state=slow`) will run the same test script again even if said script
is not specified on the `prove` command-line!
So far, this bug did not matter. Right until d8f416bbb87c (ci: run unit
tests in CI, 2023-11-09) it did not matter.
But starting with that commit, we run `prove` _twice_ in CI, and with
completely different sets of tests to run. Due to the bug, the second
invocation re-runs all of the tests that were already run as part of the
first invocation. This not only wastes build minutes, it also frequently
causes the `osx-*` jobs to fail because they already take a long time
and now are likely to run into a timeout.
The worst part about it is that there is actually no benefit to keep
running with `--state=slow,save`, ever since we decided no longer to
try to reuse the Prove cache between CI runs.
So let's just drop that Prove option and live happily ever after.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
ci: avoid running the test suite twice
This
[https://github.com/git/git/actions/runs/6845537889/job/18614840166] is
an example of a osx-* job that times out. Here
[https://github.com/git/git/actions/runs/6845537889/job/18614840166#step:4:839],
it is running t0013, and here
[https://github.com/git/git/actions/runs/6845537889/job/18614840166#step:4:2765],
it is run again (in the middle of the entire test suite, as part of make
unit-tests).
While this tries to fix a bug uncovered by js/doc-unit-tests, to avoid
merge conflicts, this is based on ps/ci-gitlab.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1613%2Fdscho%2Favoid-running-test-suite-twice-in-ci-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1613/dscho/avoid-running-test-suite-twice-in-ci-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1613
ci/lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 6dfc90d7f53..307a8df0b5a 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -281,7 +281,7 @@ else
fi
MAKEFLAGS="$MAKEFLAGS --jobs=$JOBS"
-GIT_PROVE_OPTS="--timer --jobs $JOBS --state=failed,slow,save"
+GIT_PROVE_OPTS="--timer --jobs $JOBS"
GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
case "$CI_OS_NAME" in
base-commit: 0e3b67e2aa25edb7e1a5c999c87b52a7b3a7649a
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 1/1] attr: add native file mode values support
From: Torsten Bögershausen @ 2023-11-13 16:50 UTC (permalink / raw)
To: Joanna Wang; +Cc: git
In-Reply-To: <20231111040309.2848560-1-jojwang@google.com>
On Sat, Nov 11, 2023 at 04:03:08AM +0000, Joanna Wang wrote:
Some thoughts and comments inline...
> Gives all paths inherent 'mode' attribute values based on the paths'
> modes (one of 100644, 100755, 120000, 040000, 160000). Users may use
> this feature to filter by file types. For example a pathspec such as
> ':(attr:mode=160000)' could filter for submodules without needing
My spontanous feeling is that filetype may be another choice:
> ':(attr:filetype=160000)' could filter for submodules without needing
And having written this, we can think using something borrowed from
`find . -type f`
:(attr:filetype=f)' or :(attr:filetype=x)' (for executable)
> `mode=160000` to actually be specified for each subdmoule path in
> .gitattributes. The native mode values are also reflected in
> `git check-attr` results.
But then I missed the point why we need an attribute here?
The mode is already defined by the the file system (and Git),
is there a special reason that the user can define or re-define the
value here ?
May be there is, when working with pathspec.
But then "pathspec=" could be a better construction.
Since "mode" could make a reader think that Git does somewhat with the file
when checking out.
My personal hope reading "mode=100755" in .gitattributes would
be that Git makes it executable when checking out, if if it is
recorded in Git as 100644, probably coming from a file-system that
doesn't support the executable bit in a Unix way.
> If there is any existing mode attribute for a path (e.g. there is
> !mode, -mode, mode, mode=<value> in .gitattributes) that setting will
> take precedence over the native mode value.
>
> ---
>
> I went with 'mode' because that is the word used in documentation
> (e.g. https://git-scm.com/book/sv/v2/Git-Internals-Git-Objects)
> and in the code (e.g. `ce_mode`). Please let me know what you think
> of this UX.
>
> The implementation happens within git_check_attr() method which is
> the common mathod called for getting a pathspec attribute value.
>
> The previous discussed idea did not work with `git check-attr`.
> (https://lore.kernel.org/all/CAMmZTi8swsSMcLUcW+YwUDg8GcrY_ks2+i35-nsHE3o9MNpsUQ@mail.gmail.com/).
>
> There are no tests for excluding based on pathspec attrs in subdirectories
> due to an existing bug. Since it is not specific to native mode, I thought
> it would be better to fix separately.
> https://lore.kernel.org/all/CAMmZTi89Un+bsLXdEdYs44oT8eLNp8y=Pm8ywaurcQ7ccRKGdQ@mail.gmail.com/
Reading "pathspec attrs" above it feels better to call the new attribute pathspec.
But why should a user be allowed to overwrite what we have in the index ?
That is somewhat missing in the motivation for this change,
it would be good to have this explained in the commit message.
>
> Documentation/gitattributes.txt | 10 +++++++
> attr.c | 42 ++++++++++++++++++++++++--
> t/t0003-attributes.sh | 40 +++++++++++++++++++++++++
> t/t6135-pathspec-with-attrs.sh | 53 +++++++++++++++++++++++++++++++++
> 4 files changed, 143 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 8c1793c148..bb3c11f151 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -156,6 +156,16 @@ Unspecified::
> Any other value causes Git to act as if `text` has been left
> unspecified.
>
> +`mode`
> +^^^^^
> +
> +This attribute is for filtering files by their file bit modes
> +(40000, 120000, 160000, 100755, 100644) and has native support in git,
> +meaning values for this attribute are natively set (e.g. mode=100644) by
> +git without having to specify them in .gitattributes. However, if
> +'mode' is set in .gitattributest for some path, that value takes precendence,
> +whether it is 'set', 'unset', 'unspecified', or some other value.
> +
> `eol`
> ^^^^^
>
> diff --git a/attr.c b/attr.c
> index e62876dfd3..95dc1cf695 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -1240,20 +1240,58 @@ static struct object_id *default_attr_source(void)
> return &attr_source;
> }
>
> +
> +/*
> + * This implements native file mode attr support.
> + * We always return the current mode of the path, not the mode stored
> + * in the index, unless the path is a directory where we check the index
> + * to see if it is a GITLINK. It is ok to rely on the index for GITLINK
> + * modes because a path cannot become a GITLINK (which is a git concept only)
> + * without having it indexed with a GITLINK mode in git.
> + */
> +static unsigned int native_mode_attr(struct index_state *istate, const char *path)
> +{
> + struct stat st;
> + unsigned int mode;
> + if (lstat(path, &st))
> + die_errno(_("unable to stat '%s'"), path);
> + mode = canon_mode(st.st_mode);
> + if (S_ISDIR(mode)) {
> + int pos = index_name_pos(istate, path, strlen(path));
> + if (pos >= 0)
> + if (S_ISGITLINK(istate->cache[pos]->ce_mode))
> + return istate->cache[pos]->ce_mode;
> + }
> + return mode;
> +}
> +
> +
> void git_check_attr(struct index_state *istate,
> const char *path,
> struct attr_check *check)
> {
> int i;
> const struct object_id *tree_oid = default_attr_source();
> + struct strbuf sb = STRBUF_INIT;
>
> collect_some_attrs(istate, tree_oid, path, check);
>
> for (i = 0; i < check->nr; i++) {
> unsigned int n = check->items[i].attr->attr_nr;
> const char *value = check->all_attrs[n].value;
> - if (value == ATTR__UNKNOWN)
> - value = ATTR__UNSET;
> + if (value == ATTR__UNKNOWN){
> + if (strcmp(check->all_attrs[n].attr->name, "mode") == 0) {
> + /*
> + * If value is ATTR_UNKNOWN then it has not been set
> + * anywhere with -mode (ATTR_FALSE), !mode (ATTR_UNSET),
> + * mode (ATTR_TRUE), or an explicit value. We fill
> + * value with the native mode value.
> + */
> + strbuf_addf(&sb, "%06o", native_mode_attr(istate, path));
> + value = sb.buf;
> + } else
> + value = ATTR__UNSET;
> + }
> check->items[i].value = value;
> }
> }
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index aee2298f01..9c2603d8e2 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -19,6 +19,15 @@ attr_check () {
> test_must_be_empty err
> }
>
> +attr_check_mode () {
> + path="$1" expect="$2" git_opts="$3" &&
It would be easier to read, if each assignment is on it's on line
> +
> + git $git_opts check-attr mode -- "$path" >actual 2>err &&
> + echo "$path: mode: $expect" >expect &&
> + test_cmp expect actual &&
> + test_must_be_empty err
> +}
> +
> attr_check_quote () {
> path="$1" quoted_path="$2" expect="$3" &&
>
> @@ -558,4 +567,35 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '
> test_cmp expect err
> '
>
> +test_expect_success 'submodule with .git file' '
> + mkdir sub &&
> + (cd sub &&
> + git init &&
> + mv .git .real &&
> + echo "gitdir: .real" >.git &&
> + test_commit first) &&
> + git update-index --add -- sub
Style and indentation (Please use TAB for indenting)
Using sub-shells is good. Somewhat easier to read would be this:
mkdir sub &&
(
cd sub &&
git init &&
mv .git .real &&
echo "gitdir: .real" >.git &&
test_commit first
) &&
> +'
> +
> +test_expect_success 'native mode attributes work' '
> + >exec && chmod +x exec && attr_check_mode exec 100755 &&
> + >normal && attr_check_mode normal 100644 &&
> + mkdir dir && attr_check_mode dir 040000 &&
> + ln -s normal normal_sym && attr_check_mode normal_sym 120000 &&
> + attr_check_mode sub 160000
> +'
We need a precondition here:
test_expect_success SYMLINKS
> +
> +test_expect_success '.gitattributes mode values take precedence' '
> + (
> + echo "mode_value* mode=myownmode" &&
> + echo "mode_set* mode" &&
> + echo "mode_unset* -mode" &&
> + echo "mode_unspecified* !mode"
> + ) >.gitattributes &&
Can this be written using a
cat <<-\EOF >expect &&
EOF
expression ?
> + >mode_value && attr_check_mode mode_value myownmode &&
> + >mode_unset && attr_check_mode mode_unset unset &&
> + >mode_unspecified && attr_check_mode mode_unspecified unspecified &&
> + >mode_set && attr_check_mode mode_set set
> +'
> +
> test_done
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index a9c1e4e0ec..fd9569d39b 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -64,6 +64,9 @@ test_expect_success 'setup .gitattributes' '
> fileSetLabel label
> fileValue label=foo
> fileWrongLabel label☺
> + mode_set* mode=1234
> + mode_unset* -mode
> + mode_unspecified* !mode
> EOF
> echo fileSetLabel label1 >sub/.gitattributes &&
> git add .gitattributes sub/.gitattributes &&
> @@ -295,4 +298,54 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
> test_cmp expect actual
> '
>
> +test_expect_success 'mode attr is handled correctly for overriden values' '
> + >mode_set_1 &&
> + >mode_unset_1 &&
> + >mode_unspecified_1 &&
> + >mode_regular_file_1 &&
> +
> + git status -s ":(attr:mode=1234)mode*" >actual &&
> + cat <<-\EOF >expect &&
> + ?? mode_set_1
> + EOF
> + test_cmp expect actual &&
> +
> + git status -s ":(attr:-mode)mode*" >actual &&
> + echo ?? mode_unset_1 >expect &&
> + test_cmp expect actual &&
> +
> + git status -s ":(attr:!mode)mode*" >actual &&
> + echo ?? mode_unspecified_1 >expect &&
> + test_cmp expect actual &&
> +
> + git status -s ":(attr:mode=100644)mode*" >actual &&
> + echo ?? mode_regular_file_1 >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'mode attr values are current file modes, not indexed modes' '
> + >mode_exec_file_1 &&
> +
> + git status -s ":(attr:mode=100644)mode_exec_*" >actual &&
> + echo ?? mode_exec_file_1 >expect &&
> + test_cmp expect actual &&
> +
> + git add mode_exec_file_1 && chmod +x mode_exec_file_1 &&
> + git status -s ":(attr:mode=100755)mode_exec_*" >actual &&
> + echo AM mode_exec_file_1 >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'mode attr can be excluded' '
> + >mode_1_regular &&
> + >mode_1_exec && chmod +x mode_1_exec &&
> + git status -s ":(exclude,attr:mode=100644)" "mode_1_*" >actual &&
> + echo ?? mode_1_exec >expect &&
> + test_cmp expect actual &&
> +
> + git status -s ":(exclude,attr:mode=100755)" "mode_1_*" >actual &&
> + echo ?? mode_1_regular >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.42.0.869.gea05f2083d-goog
>
>
^ permalink raw reply
* [PATCH v2] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Arthur Chan via GitGitGadget @ 2023-11-13 16:22 UTC (permalink / raw)
To: git; +Cc: Arthur Chan, Arthur Chan
In-Reply-To: <pull.1612.git.1699724379458.gitgitgadget@gmail.com>
From: Arthur Chan <arthur.chan@adalogics.com>
Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
---
fuzz: add new oss-fuzz fuzzer for date.c / date.h
This patch is aimed to add a new oss-fuzz fuzzer to the oss-fuzz
directory for fuzzing date.c / date.h in the base directory.
The .gitignore of the oss-fuzz directory and the Makefile have been
modified to accommodate the new fuzzer fuzz-date.c.
Fixed the objects order in .gitignore and Makefiles and fixed some of
the logic and formatting for the fuzz-date.c fuzzer in v2.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1612%2Farthurscchan%2Fnew-fuzzer-date-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1612/arthurscchan/new-fuzzer-date-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1612
Range-diff vs v1:
1: d43724c19d0 ! 1: 2928e2b858d fuzz: add new oss-fuzz fuzzer for date.c / date.h
@@ Commit message
Signed-off-by: Arthur Chan <arthur.chan@adalogics.com>
## Makefile ##
-@@ Makefile: ETAGS_TARGET = TAGS
+@@ Makefile: SCRIPTS = $(SCRIPT_SH_GEN) \
+ ETAGS_TARGET = TAGS
+
FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
++FUZZ_OBJS += oss-fuzz/fuzz-date.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
-+FUZZ_OBJS += oss-fuzz/fuzz-date.o
.PHONY: fuzz-objs
- fuzz-objs: $(FUZZ_OBJS)
-
## oss-fuzz/.gitignore ##
@@
fuzz-commit-graph
++fuzz-date
fuzz-pack-headers
fuzz-pack-idx
-+fuzz-date
## oss-fuzz/fuzz-date.c (new) ##
@@
@@ oss-fuzz/fuzz-date.c (new)
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
-+ int type;
-+ int time;
++ int local;
+ int num;
++ uint16_t tz;
+ char *str;
+ timestamp_t ts;
+ enum date_mode_type dmtype;
+ struct date_mode *dm;
+
-+ if (size <= 8)
-+ {
++ if (size <= 4)
++ /*
++ * we use the first byte to fuzz dmtype and local,
++ * then the next three bytes to fuzz tz offset,
++ * and the remainder (at least one byte) is fed
++ * as end-user input to approxidate_careful().
++ */
+ return 0;
-+ }
+
-+ type = (*((int *)data)) % 8;
-+ data += 4;
-+ size -= 4;
++ local = !!(*data & 0x10);
++ dmtype = (enum date_mode_type)(*data % DATE_UNIX);
++ if (dmtype == DATE_STRFTIME)
++ /*
++ * Currently DATE_STRFTIME is not supported.
++ */
++ return 0;
++ data++;
++ size--;
+
-+ time = abs(*((int *)data));
-+ data += 4;
-+ size -= 4;
++ tz = *data++;
++ tz = (tz << 8) | *data++;
++ tz = (tz << 8) | *data++;
++ size -= 3;
+
-+ str = (char *)malloc(size+1);
++ str = (char *)malloc(size + 1);
+ if (!str)
-+ {
+ return 0;
-+ }
+ memcpy(str, data, size);
+ str[size] = '\0';
+
+ ts = approxidate_careful(str, &num);
+ free(str);
+
-+ switch(type)
-+ {
-+ case 0: default:
-+ dmtype = DATE_NORMAL;
-+ break;
-+ case 1:
-+ dmtype = DATE_HUMAN;
-+ break;
-+ case 2:
-+ dmtype = DATE_SHORT;
-+ break;
-+ case 3:
-+ dmtype = DATE_ISO8601;
-+ break;
-+ case 4:
-+ dmtype = DATE_ISO8601_STRICT;
-+ break;
-+ case 5:
-+ dmtype = DATE_RFC2822;
-+ break;
-+ case 6:
-+ dmtype = DATE_RAW;
-+ break;
-+ case 7:
-+ dmtype = DATE_UNIX;
-+ break;
-+ }
-+
+ dm = date_mode_from_type(dmtype);
-+ dm->local = 1;
-+ show_date(ts, time, dm);
++ dm->local = local;
++ show_date(ts, (int16_t)tz, dm);
+
+ date_mode_release(dm);
+
Makefile | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-date.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+)
create mode 100644 oss-fuzz/fuzz-date.c
diff --git a/Makefile b/Makefile
index 03adcb5a480..4b875ef6ce1 100644
--- a/Makefile
+++ b/Makefile
@@ -750,6 +750,7 @@ SCRIPTS = $(SCRIPT_SH_GEN) \
ETAGS_TARGET = TAGS
FUZZ_OBJS += oss-fuzz/fuzz-commit-graph.o
+FUZZ_OBJS += oss-fuzz/fuzz-date.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
.PHONY: fuzz-objs
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index 9acb74412ef..5b954088254 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -1,3 +1,4 @@
fuzz-commit-graph
+fuzz-date
fuzz-pack-headers
fuzz-pack-idx
diff --git a/oss-fuzz/fuzz-date.c b/oss-fuzz/fuzz-date.c
new file mode 100644
index 00000000000..a5c887bf11c
--- /dev/null
+++ b/oss-fuzz/fuzz-date.c
@@ -0,0 +1,56 @@
+#include "git-compat-util.h"
+#include "date.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ int local;
+ int num;
+ uint16_t tz;
+ char *str;
+ timestamp_t ts;
+ enum date_mode_type dmtype;
+ struct date_mode *dm;
+
+ if (size <= 4)
+ /*
+ * we use the first byte to fuzz dmtype and local,
+ * then the next three bytes to fuzz tz offset,
+ * and the remainder (at least one byte) is fed
+ * as end-user input to approxidate_careful().
+ */
+ return 0;
+
+ local = !!(*data & 0x10);
+ dmtype = (enum date_mode_type)(*data % DATE_UNIX);
+ if (dmtype == DATE_STRFTIME)
+ /*
+ * Currently DATE_STRFTIME is not supported.
+ */
+ return 0;
+ data++;
+ size--;
+
+ tz = *data++;
+ tz = (tz << 8) | *data++;
+ tz = (tz << 8) | *data++;
+ size -= 3;
+
+ str = (char *)malloc(size + 1);
+ if (!str)
+ return 0;
+ memcpy(str, data, size);
+ str[size] = '\0';
+
+ ts = approxidate_careful(str, &num);
+ free(str);
+
+ dm = date_mode_from_type(dmtype);
+ dm->local = local;
+ show_date(ts, (int16_t)tz, dm);
+
+ date_mode_release(dm);
+
+ return 0;
+}
base-commit: dadef801b365989099a9929e995589e455c51fed
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] checkout: add config variable checkout.autoDetach
From: Phillip Wood @ 2023-11-13 15:07 UTC (permalink / raw)
To: Junio C Hamano, Andy Koppe; +Cc: git, pclouds
In-Reply-To: <xmqqbkbzo6ba.fsf@gitster.g>
On 12/11/2023 06:04, Junio C Hamano wrote:
> Andy Koppe <andy.koppe@gmail.com> writes:
>
>> The git-checkout command without pathspecs automatically detaches HEAD
>> when switching to something other than a branch, whereas git-switch
>> requires the --detach option to do so.
>>
>> Add configuration variable checkout.autoDetach to choose the behavior
>> for both: true for automatic detaching, false for requiring --detach.
>>
>> Amend their documentation and tests accordingly.
>>
>> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
>> ---
>
> "switch" was meant to be an experimental command to sort out this
> kind of UI ideas, and I think the fact that it requires a more
> explicit "--detach", where experienced users might just say "git
> checkout that-branch^0", has established itself as a more friendly
> and good thing to help new users. I do not know how others react to
> this kind of proliferation of configuration variables, but I do not
> mind this particular variable existing.
I'm a bit wary of having a config variable that could break scripts
relying on the current behavior of "git checkout". As far as "git
switch" goes I don't particularly mind this config variable though I'm
not sure it is that hard to type "--detach" (especially with tab
completion) and I do worry that we're making the UI more complex each
time we add something like this.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH] remote-curl: avoid hang if curl asks for more data after eof
From: Jiří Hruška @ 2023-11-13 11:25 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan
It has been observed that under some circumstances, libcurl can call
our `CURLOPT_READFUNCTION` callback `rpc_out()` again even after
already getting a return value of zero (EOF) back once before.
This results in `rpc_read_from_out()` trying to read more from the
child process pipe, because `rpc->flush_read_but_not_sent` is reset to
false already the first time EOF is returned, and then the whole
operation gets stuck - the child process is already trying to read
a response back and will not write anything to the output pipe anymore,
while the parent/remote process is now blocked waiting to read more too
and never even finishes sending the request.
This commit addresses the bug with the following changes:
- Move resetting of the `flush_read_but_not_sent` flag to `post_rpc()`,
just before the `rpc_out()` function would be potentially used the
next time. This makes the callback return a zero any number of times
at the end of a finished upload, thus fixing the bug.
- Rename `flush_read_but_not_sent` to `read_from_out_done`, because the
flag is not as much about reading and sending the "flush" as about
the reading/sending phase being done and that `read_from_out()` must
not be called for this particular RPC exchange anymore.
- Simplify `post_rpc()` a bit by removing nested `if` statements, extra
logic and comments. This revealed a bug in `stateless_connect()`,
where `rpc.len` is reset to 0 after each request but not `rpc.pos`,
relying on it being reset elsewhere. This is being fixed as well.
- Update all related comments and add a few new ones to help the next
person understand how the chunked large request flow works easier.
- Rename the user data/context argument of libcurl streaming callbacks
to be consistently `userdata` everywhere, just like the official
documentation calls it. The current naming is confusing especially
where it is called `buffer_` now.
- Reset also `CURLOPT_POSTFIELDSIZE_LARGE` to its default value of -1
before reusing a handle in http.c / `get_active_slot()`. It can stay
set from a previous request, e.g. to 4 from an earlier probe "0000"
packet. Seems mostly harmless in practice, but looks confusing when
looking at libcurl state while debugging problems like this one.
Signed-off-by: Jiri Hruska <jirka@fud.cz>
---
Hi all. I would like to report a bug in Git's integration of libcurl for the
HTTP transport and propose a patch to fix it.
I have seen Git get stuck while fetching a large repo sometimes over the years,
but I have never paid too much attention to it, because just trying again made
it work. But this started to happen much more frequently to me a few months back
and pushed me to investigate better.
I get to work with a large repository with lots of commits, branches and overall
traffic and when I run `git fetch --tags --force` on it, there are a lot of refs
to synchronize even after just a few days. And Git happens to hang for me, like
50% of the time. After a lot of activity and network I/O, it just gets stuck.
I have dumped the relevant processes and found that it all hangs because there
is a deadlock between a parent "git-remote-https" (curl) process and a child
process handling a "fetch-pack" command. Both sides are blocked on reading more
from _their_ side of the IPC pipe, so neither gets to do anything.
The call stacks where both sides get stuck are as follows:
Parent: git-remote-https origin https://blah@dev.azure.com/blah/_git/meh
00 ntdll!ZwReadFile
01 KERNELBASE!ReadFile
02 msvcrt!_read_nolock
03 msvcrt!_read <<<<<<<<<<<<<<<<<<<<<<<< fd = child/client.out
04 git_remote_https!xread
05 git_remote_https!read_in_full
06 git_remote_https!get_packet_data
07 git_remote_https!packet_read_with_status
08 git_remote_https!rpc_read_from_out
09 git_remote_https!rpc_out
0a libcurl_4!CURLOPT_READFUNCTION callback
0b libcurl_4!...
0e git_remote_https!run_active_slot
0f git_remote_https!run_one_slot
10 git_remote_https!run_slot
11 git_remote_https!post_rpc
12 git_remote_https!rpc_service
13 git_remote_https!fetch_git
14 git_remote_https!fetch
15 git_remote_https!parse_fetch
16 git_remote_https!cmd_main
...
Child: git fetch-pack --stateless-rpc --stdin --lock-pack --thin
--no-progress https://blah@dev.azure.com/blah/_git/meh
00 ntdll!ZwReadFile
01 KERNELBASE!ReadFile
02 msvcrt!_read_nolock
03 msvcrt!_read <<<<<<<<<<<<<<<<<<<<<<<< fd = stdin (1)
04 git!xread
05 git!read_in_full
06 git!get_packet_data
07 git!packet_read_with_status
08 git!packet_reader_read
09 git!get_ack
0a git!find_common
0b git!do_fetch_patch
0c git!fetch_pack
0d git!cmd_fetch_pack
...
That is, the child is already trying to read the _response_ of an incremental
common commits negotiation, but the parent, orchestrating the whole RPC
interaction, has not finished sending the _request_ data yet and wants more.
I looked at the packets passed between the two processes, I read through the
whole rpc_service() - post_rpc() - rpc_out() flow many times, but I could not
see how it could all get into this blocked state. It was clear that everything
worked fine as long as the request data fit into one packet, but once the "large
request" code path was taken, things broke and got stuck. But I could not see
any weirdness in the implementation on the Git side that could trigger this.
Which was a correct observation, because the weirdness happens in libcurl!
It turns out that curl's function readwrite_upload() in lib/transfer.c [1] can
call the configured CURLOPT_READFUNCTION callback multiple times even if already
at EOF, even if the user callback has previously returned a zero to signal the
end of the data to upload before. It does not happen every time, only if the
next component in curl (OS socket, http/2 library, ...) is not able to send all
the data right away (and readwrite_upload() then runs again with offset > 0).
[1]: https://github.com/curl/curl/blob/ad051e1cbec68b2456a22661bf0800d49d085294/lib/transfer.c#L810
This is harmless when using the default fread() implementation, which the whole
CURLOPT_READFUNCTION is based on. But Git's rpc_out() function is not safe in
this regard. rpc->flush_read_but_not_sent is reset to false already before
returning a zero (EOF), so the next invocation goes to rpc_read_from_out()
again, reads from the silent pipe and blocks the upload from ever finishing.
A trivial solution would be to just take the line which resets the flag
/*
* The line length either does not need to be sent at
* all or has already been completely sent. Now we can
* return 0, indicating EOF, meaning that the flush has
* been fully sent.
*/
- rpc->flush_read_but_not_sent = 0;
return 0;
from rpc_out() and reset it only in post_rpc(), before the next time a large
request is being sent out and rpc_out() will go into play again:
if (large_request) {
...
rpc->initial_buffer = 1;
+ rpc->flush_read_but_not_sent = 0;
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
This way the CURLOPT_READFUNCTION would be returning zeros at the end of the
upload as long as needed, just like fread() at the end of a real file.
Hence, the bug could be fixed with just that two-lines change.
But while trying to figure out the above, I noticed a few things that prolonged
the time I needed to understand what was going on, so I would like to propose
some more changes to make the code perhaps a bit easier to read for the next
person who comes to hack on it after me.
The description of the extra modifications is in the commit message. All of
these changes are obviously optional and naturally subjective. I think that we
can all agree on some points (less indentation = good), but naming is hard,
and so is balance between unclear and too verbose, or when to split all
non-functional changes to a separate commit. So let me know if there are things
to do differently and I will gladly obey, it is your codebase after all.
Which brings me to the next topic, testing.
Validating the fix would be trivial with a mocked libcurl, but turns out to be
much harder with the integration-level test suite of this project.
I have validated this solution against a patched version of libcurl:
diff --git a/lib/transfer.c b/lib/transfer.c
index ba0410fc5..82141b648 100644
--- a/lib/transfer.c
+++ b/lib/transfer.c
@@ -915,6 +915,9 @@ static CURLcode readwrite_upload(struct Curl_easy *data,
k->upload_fromhere += offset;
result = Curl_fillreadbuffer(data,
data->set.upload_buffer_size-offset,
&fillcount);
+ if(!result && fillcount == 0)
+ result = Curl_fillreadbuffer(data,
data->set.upload_buffer_size-offset,
+ &fillcount);
k->upload_fromhere -= offset;
if(result)
return result;
I validated this solution on Windows, in the real-life case of the repo which
triggers these problems, managing to always fetch it smoothly after the fix.
But outside of that, and as far as the bundled test suite goes, I have failed to
write a test that could validate this problem does not ever occur again.
For all I know, the special case when curl calls the callback again happens only
on some operating systems (Windows), only with some versions of libcurl, only
with some dependencies (libnghttp2, OpenSSL) only when talking to some remote
servers (Microsoft Azure DevOps repos) over some protocols (HTTP/2), and only
with some back luck (network speed, TCP/IP window size, moon phase, ...).
Actually, I found at least one report from the past (2017) on Linux [2], which
seems to show exactly the same symptoms. Hard to tell if the root cause was
really the same too, though. The Internet seems full of "git fetch/pull hangs"
stories and there are always helpful people who jump in with their proven
solution of "git config --global http.postBuffer 100000000000000"... If this
bug fix could stop some of that nonsense, that would be awesome.
[2]: https://public-inbox.org/git/xmqqefzoxeki.fsf@gitster.mtv.corp.google.com/T/
Anyway, when I enabled extra GIT_TRACE_PACKET and GIT_CURL_VERBOSE, I could not
reproduce it anymore. When I tried to play with some of the t/*http*.sh
testcases, I did not manage to reproduce it either. And even then, reproducing
it _sometimes_ is not the same as reproducing it _always_. A flaky test is
often worse than no test at all.
So, I do not know how to write a test for this. Any suggestions are welcome.
Based on the guidance from Documentation/SubmittingPatches, as a bug fix, the
patch is based on the current `maint` (2.42.1). But applies cleanly to `master`,
`next` and `seen` as of 2023-11-12 too.
I have added Jeff and Jonathan to CC based on some previous commits to similar
places in remote-curl.c. Sorry if not or no longer relevant.
Thank you for your kind consideration.
Jiri
http.c | 1 +
remote-curl.c | 93 +++++++++++++++++++++++++++++----------------------
2 files changed, 54 insertions(+), 40 deletions(-)
diff --git a/http.c b/http.c
index 8f71bf00d8..14f2fbb82e 100644
--- a/http.c
+++ b/http.c
@@ -1454,6 +1454,7 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL);
curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
+ curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, (curl_off_t)-1);
curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
diff --git a/remote-curl.c b/remote-curl.c
index ef05752ca5..c043836214 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -606,13 +606,14 @@ struct rpc_state {
unsigned write_line_lengths : 1;
/*
- * Used by rpc_out; initialize to 0. This is true if a flush has been
- * read, but the corresponding line length (if write_line_lengths is
- * true) and EOF have not been sent to libcurl. Since each flush marks
- * the end of a request, each flush must be completely sent before any
- * further reading occurs.
+ * Used by rpc_out; initialize to 0. This is true if a flush packet
+ * has been read from the child process, signaling the end of the
+ * current data to send. There might be still some bytes pending in
+ * 'buf' (e.g. the corresponding line length, if write_line_lengths
+ * is true), but no more reads can be performed on the 'out' pipe as
+ * part of the current RPC exchange.
*/
- unsigned flush_read_but_not_sent : 1;
+ unsigned read_from_out_done : 1;
};
#define RPC_STATE_INIT { 0 }
@@ -675,48 +676,51 @@ static int rpc_read_from_out(struct rpc_state
*rpc, int options,
return 1;
}
+/*
+ * CURLOPT_READFUNCTION callback, called by libcurl when it wants more data
+ * to send out. Used only if the request did not fit into just one buffer and
+ * data must be streamed as it comes.
+ * Has the same semantics as fread(), but reads packets from the pipe from
+ * the child process instead. A return value of 0 (EOF) finishes the upload.
+ */
static size_t rpc_out(void *ptr, size_t eltsize,
- size_t nmemb, void *buffer_)
+ size_t nmemb, void *userdata)
{
size_t max = eltsize * nmemb;
- struct rpc_state *rpc = buffer_;
+ struct rpc_state *rpc = userdata;
size_t avail = rpc->len - rpc->pos;
enum packet_read_status status;
- if (!avail) {
+ /*
+ * If there is no more data available in our buffer and the child
+ * process is not done sending yet, read the next packet.
+ */
+ if (!avail && !rpc->read_from_out_done) {
rpc->initial_buffer = 0;
rpc->len = 0;
rpc->pos = 0;
- if (!rpc->flush_read_but_not_sent) {
- if (!rpc_read_from_out(rpc, 0, &avail, &status))
- BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX");
- if (status == PACKET_READ_FLUSH)
- rpc->flush_read_but_not_sent = 1;
- }
+ if (!rpc_read_from_out(rpc, 0, &avail, &status))
+ BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX");
+
/*
- * If flush_read_but_not_sent is true, we have already read one
- * full request but have not fully sent it + EOF, which is why
- * we need to refrain from reading.
- */
- }
- if (rpc->flush_read_but_not_sent) {
- if (!avail) {
- /*
- * The line length either does not need to be sent at
- * all or has already been completely sent. Now we can
- * return 0, indicating EOF, meaning that the flush has
- * been fully sent.
- */
- rpc->flush_read_but_not_sent = 0;
- return 0;
- }
- /*
- * If avail is non-zero, the line length for the flush still
- * hasn't been fully sent. Proceed with sending the line
- * length.
+ * If a flush packet was read, it means the child process is
+ * done sending this request. The buffer might be fully empty
+ * at this point or contain a flush packet too, depending on
+ * rpc->write_line_lengths.
+ * In any case, we must refrain from reading any more, because
+ * the child process already expects to receive a response back
+ * instead. If both sides would try to read at once, they would
+ * just hang waiting for each other.
*/
+ rpc->read_from_out_done = (status == PACKET_READ_FLUSH);
}
+ /*
+ * Copy data to the provided buffer. If there is nothing more to send,
+ * nothing gets written and the return value is 0 (EOF).
+ * It is important to keep returning 0 as long as needed in that case,
+ * as libcurl invokes the callback multiple times at EOF sometimes.
+ */
if (max < avail)
avail = max;
memcpy(ptr, rpc->buf + rpc->pos, avail);
@@ -724,9 +728,16 @@ static size_t rpc_out(void *ptr, size_t eltsize,
return avail;
}
-static int rpc_seek(void *clientp, curl_off_t offset, int origin)
+/*
+ * CURLOPT_SEEKFUNCTION callback, called by libcurl when it wants to seek in
+ * the data being sent out. Used only if the request did not fit into just
+ * one buffer and data must be streamed as it comes.
+ * Has the same semantics as fseek(), but seeks in the buffered packet read
+ * from the pipe from the child process instead.
+ */
+static int rpc_seek(void *userdata, curl_off_t offset, int origin)
{
- struct rpc_state *rpc = clientp;
+ struct rpc_state *rpc = userdata;
if (origin != SEEK_SET)
BUG("rpc_seek only handles SEEK_SET, not %d", origin);
@@ -800,10 +811,10 @@ struct rpc_in_data {
* from ptr.
*/
static size_t rpc_in(char *ptr, size_t eltsize,
- size_t nmemb, void *buffer_)
+ size_t nmemb, void *userdata)
{
size_t size = eltsize * nmemb;
- struct rpc_in_data *data = buffer_;
+ struct rpc_in_data *data = userdata;
long response_code;
if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
@@ -963,6 +974,7 @@ static int post_rpc(struct rpc_state *rpc, int
stateless_connect, int flush_rece
*/
headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
rpc->initial_buffer = 1;
+ rpc->read_from_out_done = 0;
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
@@ -1482,7 +1494,7 @@ static int stateless_connect(const char *service_name)
rpc.gzip_request = 1;
rpc.initial_buffer = 0;
rpc.write_line_lengths = 1;
- rpc.flush_read_but_not_sent = 0;
+ rpc.read_from_out_done = 0;
/*
* Dump the capability listing that we got from the server earlier
@@ -1505,6 +1517,7 @@ static int stateless_connect(const char *service_name)
break;
/* Reset the buffer for next request */
rpc.len = 0;
+ rpc.pos = 0;
}
free(rpc.service_url);
--
2.42.1.1.g18afbdf3b0
^ permalink raw reply related
* [PATCH 13/13] Handled git pipeline errors - z/OS enable
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This PR has fixes to enable build on z/OS
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
convert.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/convert.c b/convert.c
index 4c034bb714c..c66f5b3ecec 100644
--- a/convert.c
+++ b/convert.c
@@ -1315,7 +1315,6 @@ static struct attr_check *check;
static void get_platform(char** result) {
struct utsname uname_info;
- *result = NULL;
if (uname(&uname_info) < 0)
die(_("uname() failed with error '%s' (%d)\n"),
@@ -1346,11 +1345,10 @@ void convert_attrs(struct index_state *istate,
{
struct attr_check_item *ccheck = NULL;
struct strbuf platform_working_tree_encoding = STRBUF_INIT;
- char* result=NULL;
+ char* result="Unknown";
get_platform(&result);
strbuf_addf(&platform_working_tree_encoding, "%s-working-tree-encoding", result);
- if (result != NULL)
- free (result);
+ free (result);
if (!check) {
check = attr_check_initl("crlf", "ident", "filter",
--
gitgitgadget
^ permalink raw reply related
* [PATCH 12/13] Handled git pipeline errors - Memory leak
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This PR has fixes to enable build on z/OS
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
convert.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/convert.c b/convert.c
index 16173a1caf6..4c034bb714c 100644
--- a/convert.c
+++ b/convert.c
@@ -1313,9 +1313,9 @@ static int git_path_check_ident(struct attr_check_item *check)
static struct attr_check *check;
-static const char* get_platform(void) {
+static void get_platform(char** result) {
struct utsname uname_info;
- char *result = NULL;
+ *result = NULL;
if (uname(&uname_info) < 0)
die(_("uname() failed with error '%s' (%d)\n"),
@@ -1325,10 +1325,10 @@ static const char* get_platform(void) {
if(*uname_info.sysname != '\0')
{
int index=0;
- result = (char *)malloc(strlen(uname_info.sysname)+1);
+ *result = (char *)malloc(strlen(uname_info.sysname)+1);
while(index <= strlen(uname_info.sysname))
{
- result[index] = uname_info.sysname[index];
+ (*result)[index] = uname_info.sysname[index];
index++;
}
}
@@ -1336,9 +1336,8 @@ static const char* get_platform(void) {
#ifdef __MVS__
if (!strcmp(uname_info.sysname, "OS/390"))
- result="zos";
+ *result = "zos";
#endif
- return (char*)result;
}
@@ -1347,8 +1346,11 @@ void convert_attrs(struct index_state *istate,
{
struct attr_check_item *ccheck = NULL;
struct strbuf platform_working_tree_encoding = STRBUF_INIT;
-
- strbuf_addf(&platform_working_tree_encoding, "%s-working-tree-encoding", get_platform());
+ char* result=NULL;
+ get_platform(&result);
+ strbuf_addf(&platform_working_tree_encoding, "%s-working-tree-encoding", result);
+ if (result != NULL)
+ free (result);
if (!check) {
check = attr_check_initl("crlf", "ident", "filter",
--
gitgitgadget
^ permalink raw reply related
* [PATCH 11/13] strncpy fix Handled git pipeline errors
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This PR has fixes to enable build on z/OS
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
convert.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/convert.c b/convert.c
index ef44e6429da..16173a1caf6 100644
--- a/convert.c
+++ b/convert.c
@@ -1326,8 +1326,9 @@ static const char* get_platform(void) {
{
int index=0;
result = (char *)malloc(strlen(uname_info.sysname)+1);
- while(result[index] = uname_info.sysname[index])
+ while(index <= strlen(uname_info.sysname))
{
+ result[index] = uname_info.sysname[index];
index++;
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 10/13] strncpy fix Handled git pipeline errors
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This PR has fixes to enable build on z/OS
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
convert.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/convert.c b/convert.c
index 78403de422d..ef44e6429da 100644
--- a/convert.c
+++ b/convert.c
@@ -1326,13 +1326,10 @@ static const char* get_platform(void) {
{
int index=0;
result = (char *)malloc(strlen(uname_info.sysname)+1);
- while(index <= strlen(uname_info.sysname))
+ while(result[index] = uname_info.sysname[index])
{
- *result = uname_info.sysname[index];
- ++result;
- ++index;
+ index++;
}
- *result = '\0';
}
#endif
@@ -1340,8 +1337,7 @@ static const char* get_platform(void) {
if (!strcmp(uname_info.sysname, "OS/390"))
result="zos";
#endif
-
- return result;
+ return (char*)result;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 09/13] strncpy fix Handled git pipeline errors
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This PR has fixes to enable build on z/OS
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
convert.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/convert.c b/convert.c
index 9cd0c1382ac..78403de422d 100644
--- a/convert.c
+++ b/convert.c
@@ -1326,7 +1326,13 @@ static const char* get_platform(void) {
{
int index=0;
result = (char *)malloc(strlen(uname_info.sysname)+1);
- strncpy(result, uname_info.sysname, strlen(uname_info.sysname));
+ while(index <= strlen(uname_info.sysname))
+ {
+ *result = uname_info.sysname[index];
+ ++result;
+ ++index;
+ }
+ *result = '\0';
}
#endif
--
gitgitgadget
^ permalink raw reply related
* [PATCH 08/13] platform_name fix Handled git pipeline errors
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This PR has fixes to enable build on z/OS
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
convert.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/convert.c b/convert.c
index c8d30011458..9cd0c1382ac 100644
--- a/convert.c
+++ b/convert.c
@@ -1321,19 +1321,14 @@ static const char* get_platform(void) {
die(_("uname() failed with error '%s' (%d)\n"),
strerror(errno),
errno);
-
+#ifndef __MVS__
if(*uname_info.sysname != '\0')
{
int index=0;
result = (char *)malloc(strlen(uname_info.sysname)+1);
- while(index <= strlen(uname_info.sysname))
- {
- *result = uname_info.sysname[index];
- ++result;
- ++index;
- }
- *result = '\0';
+ strncpy(result, uname_info.sysname, strlen(uname_info.sysname));
}
+#endif
#ifdef __MVS__
if (!strcmp(uname_info.sysname, "OS/390"))
--
gitgitgadget
^ permalink raw reply related
* [PATCH 07/13] spaces and errors fix Handled git pipeline errors
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This PR has fixes to enable build on z/OS
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
convert.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/convert.c b/convert.c
index 7fe107710ec..c8d30011458 100644
--- a/convert.c
+++ b/convert.c
@@ -1314,9 +1314,15 @@ static int git_path_check_ident(struct attr_check_item *check)
static struct attr_check *check;
static const char* get_platform(void) {
- struct utsname uname_info = {0};
+ struct utsname uname_info;
char *result = NULL;
- if(!uname_info.sysname[0])
+
+ if (uname(&uname_info) < 0)
+ die(_("uname() failed with error '%s' (%d)\n"),
+ strerror(errno),
+ errno);
+
+ if(*uname_info.sysname != '\0')
{
int index=0;
result = (char *)malloc(strlen(uname_info.sysname)+1);
@@ -1326,15 +1332,13 @@ static const char* get_platform(void) {
++result;
++index;
}
+ *result = '\0';
}
- if (uname(&uname_info))
- die(_("uname() failed with error '%s' (%d)\n"),
- strerror(errno),
- errno);
-
+#ifdef __MVS__
if (!strcmp(uname_info.sysname, "OS/390"))
result="zos";
+#endif
return result;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 02/13] Enable builds for z/OS.
From: Haritha D via GitGitGadget @ 2023-11-13 10:24 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
In-Reply-To: <pull.1537.git.git.1699871056.gitgitgadget@gmail.com>
From: Haritha D <harithamma.d@ibm.com>
This commit enables git to build on z/OS.
It takes advantage of enahanced ASCII
services on z/OS to auto-convert input
files to ASCII
It also adds support for
[platform]-working-tree-encoding.
Platform is substituted with uname_info.sysname,
so it will only apply to the given platform.
Also adds support for scripts that are not in
standard locations so that /bin/env bash
can be specified.
Signed-off-by: Harithamma D <harithamma.d@ibm.com>
---
Makefile | 21 +++++++++---
builtin.h | 3 ++
builtin/archive.c | 6 ++++
builtin/hash-object.c | 28 +++++++++++++++
combine-diff.c | 4 +++
config.c | 7 ++++
configure.ac | 3 ++
convert.c | 44 ++++++++++++++++++++----
copy.c | 3 ++
diff.c | 11 ++++++
entry.c | 26 ++++++++++++++
environment.c | 3 ++
git-compat-util.h | 8 +++++
negotiator/default.c | 4 +--
negotiator/noop.c | 4 +--
negotiator/skipping.c | 4 +--
object-file.c | 80 ++++++++++++++++++++++++++++++++++++++++++-
read-cache.c | 3 ++
utf8.c | 11 ++++++
19 files changed, 255 insertions(+), 18 deletions(-)
diff --git a/Makefile b/Makefile
index 9c6a2f125f8..30aa76da4f4 100644
--- a/Makefile
+++ b/Makefile
@@ -20,6 +20,8 @@ include shared.mak
#
# Define SHELL_PATH to a POSIX shell if your /bin/sh is broken.
#
+# Define SHELL_PATH_FOR_SCRIPTS to a POSIX shell if your /bin/sh is broken.
+#
# Define SANE_TOOL_PATH to a colon-separated list of paths to prepend
# to PATH if your tools in /usr/bin are broken.
#
@@ -215,6 +217,8 @@ include shared.mak
#
# Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).
#
+# Define PERL_PATH_FOR_SCRIPTS to a Perl binary if your /usr/bin/perl is broken.
+#
# Define NO_PERL if you do not want Perl scripts or libraries at all.
#
# Define NO_PERL_CPAN_FALLBACKS if you do not want to install bundled
@@ -903,9 +907,15 @@ BINDIR_PROGRAMS_NO_X += git-cvsserver
ifndef SHELL_PATH
SHELL_PATH = /bin/sh
endif
+ifndef SHELL_PATH_FOR_SCRIPTS
+ SHELL_PATH_FOR_SCRIPTS = /bin/sh
+endif
ifndef PERL_PATH
PERL_PATH = /usr/bin/perl
endif
+ifndef PERL_PATH_FOR_SCRIPTS
+ PERL_PATH_FOR_SCRIPTS = /usr/bin/perl
+endif
ifndef PYTHON_PATH
PYTHON_PATH = /usr/bin/python
endif
@@ -1336,7 +1346,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
# xdiff and reftable libs may in turn depend on what is in libgit.a
GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
-EXTLIBS =
+EXTLIBS = $(ZOPEN_EXTRA_LIBS)
GIT_USER_AGENT = git/$(GIT_VERSION)
@@ -2226,9 +2236,10 @@ perllibdir_relative_SQ = $(subst ','\'',$(perllibdir_relative))
gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
gitwebstaticdir_SQ = $(subst ','\'',$(gitwebstaticdir))
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH_FOR_SCRIPTS))
TEST_SHELL_PATH_SQ = $(subst ','\'',$(TEST_SHELL_PATH))
PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
+PERL_PATH_FOR_SCRIPTS_SQ = $(subst ','\'',$(PERL_PATH_FOR_SCRIPTS))
PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
DIFF_SQ = $(subst ','\'',$(DIFF))
@@ -2448,7 +2459,7 @@ hook-list.h: generate-hooklist.sh Documentation/githooks.txt
SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):\
$(localedir_SQ):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
- $(gitwebdir_SQ):$(PERL_PATH_SQ):$(PAGER_ENV):\
+ $(gitwebdir_SQ):$(PERL_PATH_FOR_SCRIPTS_SQ):$(PAGER_ENV):\
$(perllibdir_SQ)
GIT-SCRIPT-DEFINES: FORCE
@FLAGS='$(SCRIPT_DEFINES)'; \
@@ -2465,7 +2476,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
-e $(BROKEN_PATH_FIX) \
-e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
- -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
+ -e 's|@@PERL@@|$(PERL_PATH_FOR_SCRIPTS_SQ)|g' \
-e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
$@.sh >$@+
endef
@@ -2519,7 +2530,7 @@ PERL_DEFINES += $(gitexecdir) $(perllibdir) $(localedir)
$(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
$(QUIET_GEN) \
sed -e '1{' \
- -e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \
+ -e ' s|#!.*perl|#!$(PERL_PATH_FOR_SCRIPTS_SQ)|' \
-e ' r GIT-PERL-HEADER' \
-e ' G' \
-e '}' \
diff --git a/builtin.h b/builtin.h
index d560baa6618..806af1a262d 100644
--- a/builtin.h
+++ b/builtin.h
@@ -250,5 +250,8 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix);
int cmd_show_ref(int argc, const char **argv, const char *prefix);
int cmd_pack_refs(int argc, const char **argv, const char *prefix);
int cmd_replace(int argc, const char **argv, const char *prefix);
+#ifdef __MVS__
+ extern int setbinaryfd(int);
+#endif
#endif
diff --git a/builtin/archive.c b/builtin/archive.c
index 90761fdfee0..53ec794356f 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -14,6 +14,12 @@
static void create_output_file(const char *output_file)
{
int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
+#ifdef __MVS__
+ #if (__CHARSET_LIB == 1)
+ if (setbinaryfd(output_fd))
+ die_errno(_("could not tag archive file '%s'"), output_file);
+ #endif
+#endif
if (output_fd != 1) {
if (dup2(output_fd, 1) < 0)
die_errno(_("could not redirect output"));
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 5ffec99dcea..b33b32ff977 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -57,11 +57,39 @@ static void hash_fd(int fd, const char *type, const char *path, unsigned flags,
maybe_flush_or_die(stdout, "hash to stdout");
}
+#ifdef __MVS__
+# if (__CHARSET_LIB == 1)
+# include <stdio.h>
+# include <stdlib.h>
+
+ int setbinaryfd(int fd)
+ {
+ attrib_t attr;
+ int rc;
+
+ memset(&attr, 0, sizeof(attr));
+ attr.att_filetagchg = 1;
+ attr.att_filetag.ft_ccsid = FT_BINARY;
+ attr.att_filetag.ft_txtflag = 0;
+
+ rc = __fchattr(fd, &attr, sizeof(attr));
+ return rc;
+ }
+# endif
+#endif
+
+
static void hash_object(const char *path, const char *type, const char *vpath,
unsigned flags, int literally)
{
int fd;
fd = xopen(path, O_RDONLY);
+#ifdef __MVS__
+# if (__CHARSET_LIB == 1)
+ if (setbinaryfd(fd))
+ die_errno("Cannot set to binary '%s'", path);
+# endif
+#endif
hash_fd(fd, type, vpath, flags, literally);
}
diff --git a/combine-diff.c b/combine-diff.c
index f90f4424829..73445a517c7 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1082,6 +1082,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
ssize_t done;
int is_file, i;
+#ifdef __MVS__
+ __disableautocvt(fd);
+#endif
+
elem->mode = canon_mode(st.st_mode);
/* if symlinks don't work, assume symlink if all parents
* are symlinks
diff --git a/config.c b/config.c
index f9a1cca4e8a..37c124a37c0 100644
--- a/config.c
+++ b/config.c
@@ -1521,6 +1521,13 @@ static int git_default_core_config(const char *var, const char *value,
return 0;
}
+ #ifdef __MVS__
+ if (!strcmp(var, "core.ignorefiletags")) {
+ ignore_file_tags = git_config_bool(var, value);
+ return 0;
+ }
+ #endif
+
if (!strcmp(var, "core.safecrlf")) {
int eol_rndtrp_die;
if (value && !strcasecmp(value, "warn")) {
diff --git a/configure.ac b/configure.ac
index 276593cd9dd..ed380504be6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -463,6 +463,9 @@ else
CC_LD_DYNPATH=-Wl,+b,
else
CC_LD_DYNPATH=
+ if test "$(uname -s)" = "OS/390"; then
+ CC_LD_DYNPATH=-L
+ fi
AC_MSG_WARN([linker does not support runtime path to dynamic libraries])
fi
fi
diff --git a/convert.c b/convert.c
index a8870baff36..4f14ff6f1ed 100644
--- a/convert.c
+++ b/convert.c
@@ -377,12 +377,15 @@ static int check_roundtrip(const char *enc_name)
static const char *default_encoding = "UTF-8";
static int encode_to_git(const char *path, const char *src, size_t src_len,
- struct strbuf *buf, const char *enc, int conv_flags)
+ struct strbuf *buf, const char *enc, enum convert_crlf_action attr_action, int conv_flags)
{
char *dst;
size_t dst_len;
int die_on_error = conv_flags & CONV_WRITE_OBJECT;
+ if (attr_action == CRLF_BINARY) {
+ return 0;
+ }
/*
* No encoding is specified or there is nothing to encode.
* Tell the caller that the content was not modified.
@@ -403,6 +406,11 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,
return 0;
trace_encoding("source", path, enc, src, src_len);
+#ifdef __MVS__
+ // Don't convert ISO8859-1 on z/OS
+ if (strcasecmp("ISO8859-1", enc) == 0)
+ return 0;
+#endif
dst = reencode_string_len(src, src_len, default_encoding, enc,
&dst_len);
if (!dst) {
@@ -468,11 +476,14 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,
}
static int encode_to_worktree(const char *path, const char *src, size_t src_len,
- struct strbuf *buf, const char *enc)
+ struct strbuf *buf, enum convert_crlf_action attr_action, const char *enc)
{
char *dst;
size_t dst_len;
+ if (attr_action == CRLF_BINARY) {
+ return 0;
+ }
/*
* No encoding is specified or there is nothing to encode.
* Tell the caller that the content was not modified.
@@ -1302,18 +1313,37 @@ static int git_path_check_ident(struct attr_check_item *check)
static struct attr_check *check;
+static const char* get_platform() {
+ struct utsname uname_info;
+
+ if (uname(&uname_info))
+ die(_("uname() failed with error '%s' (%d)\n"),
+ strerror(errno),
+ errno);
+
+ if (!strcmp(uname_info.sysname, "OS/390"))
+ return "zos";
+ return uname_info.sysname;
+}
+
+
void convert_attrs(struct index_state *istate,
struct conv_attrs *ca, const char *path)
{
struct attr_check_item *ccheck = NULL;
+ struct strbuf platform_working_tree_encoding = STRBUF_INIT;
+
+ strbuf_addf(&platform_working_tree_encoding, "%s-working-tree-encoding", get_platform());
+
if (!check) {
check = attr_check_initl("crlf", "ident", "filter",
- "eol", "text", "working-tree-encoding",
+ "eol", "text", "working-tree-encoding", platform_working_tree_encoding.buf,
NULL);
user_convert_tail = &user_convert;
git_config(read_convert_config, NULL);
}
+ strbuf_release(&platform_working_tree_encoding);
git_check_attr(istate, path, check);
ccheck = check->items;
@@ -1334,6 +1364,8 @@ void convert_attrs(struct index_state *istate,
ca->crlf_action = CRLF_TEXT_CRLF;
}
ca->working_tree_encoding = git_path_check_encoding(ccheck + 5);
+ if (git_path_check_encoding(ccheck + 6))
+ ca->working_tree_encoding = git_path_check_encoding(ccheck + 6);
/* Save attr and make a decision for action */
ca->attr_action = ca->crlf_action;
@@ -1427,7 +1459,7 @@ int convert_to_git(struct index_state *istate,
len = dst->len;
}
- ret |= encode_to_git(path, src, len, dst, ca.working_tree_encoding, conv_flags);
+ ret |= encode_to_git(path, src, len, dst, ca.working_tree_encoding, ca.attr_action, conv_flags);
if (ret && dst) {
src = dst->buf;
len = dst->len;
@@ -1455,7 +1487,7 @@ void convert_to_git_filter_fd(struct index_state *istate,
if (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN, NULL, NULL))
die(_("%s: clean filter '%s' failed"), path, ca.drv->name);
- encode_to_git(path, dst->buf, dst->len, dst, ca.working_tree_encoding, conv_flags);
+ encode_to_git(path, dst->buf, dst->len, dst, ca.working_tree_encoding, ca.attr_action, conv_flags);
crlf_to_git(istate, path, dst->buf, dst->len, dst, ca.crlf_action, conv_flags);
ident_to_git(dst->buf, dst->len, dst, ca.ident);
}
@@ -1487,7 +1519,7 @@ static int convert_to_working_tree_ca_internal(const struct conv_attrs *ca,
}
}
- ret |= encode_to_worktree(path, src, len, dst, ca->working_tree_encoding);
+ ret |= encode_to_worktree(path, src, len, dst, ca->attr_action, ca->working_tree_encoding);
if (ret) {
src = dst->buf;
len = dst->len;
diff --git a/copy.c b/copy.c
index 23d84c6c1db..63546aecf81 100644
--- a/copy.c
+++ b/copy.c
@@ -14,6 +14,9 @@ int copy_fd(int ifd, int ofd)
if (write_in_full(ofd, buffer, len) < 0)
return COPY_WRITE_ERROR;
}
+#ifdef __MVS__
+ __copyfdccsid(ifd, ofd);
+#endif
return 0;
}
diff --git a/diff.c b/diff.c
index 2c602df10a3..28b96d53dbc 100644
--- a/diff.c
+++ b/diff.c
@@ -4083,6 +4083,9 @@ int diff_populate_filespec(struct repository *r,
int check_binary = options ? options->check_binary : 0;
int err = 0;
int conv_flags = global_conv_flags_eol;
+#ifdef __MVS__
+ int autocvtToASCII;
+#endif
/*
* demote FAIL to WARN to allow inspecting the situation
* instead of refusing.
@@ -4155,9 +4158,17 @@ int diff_populate_filespec(struct repository *r,
s->is_binary = 1;
return 0;
}
+#ifdef __MVS__
+ validate_codeset(r->index, s->path, &autocvtToASCII);
+#endif
fd = open(s->path, O_RDONLY);
if (fd < 0)
goto err_empty;
+
+#ifdef __MVS__
+ if (!autocvtToASCII)
+ __disableautocvt(fd);
+#endif
s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
close(fd);
s->should_munmap = 1;
diff --git a/entry.c b/entry.c
index 076e97eb89c..df6feb2234b 100644
--- a/entry.c
+++ b/entry.c
@@ -126,6 +126,24 @@ int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st)
return 0;
}
+#ifdef __MVS__
+void tag_file_as_working_tree_encoding(struct index_state *istate, char* path, int fd) {
+ struct conv_attrs ca;
+ convert_attrs(istate, &ca, path);
+ if (ca.attr_action != CRLF_BINARY) {
+ if (ca.working_tree_encoding)
+ __chgfdcodeset(fd, ca.working_tree_encoding);
+ else
+ __setfdtext(fd);
+ }
+ else {
+ __setfdbinary(fd);
+ }
+
+ __disableautocvt(fd);
+}
+#endif
+
static int streaming_write_entry(const struct cache_entry *ce, char *path,
struct stream_filter *filter,
const struct checkout *state, int to_tempfile,
@@ -138,6 +156,10 @@ static int streaming_write_entry(const struct cache_entry *ce, char *path,
if (fd < 0)
return -1;
+#ifdef __MVS__
+ tag_file_as_working_tree_encoding(state->istate, path, fd);
+#endif
+
result |= stream_blob_to_fd(fd, &ce->oid, filter, 1);
*fstat_done = fstat_checkout_output(fd, state, statbuf);
result |= close(fd);
@@ -374,6 +396,10 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca
return error_errno("unable to create file %s", path);
}
+#ifdef __MVS__
+ tag_file_as_working_tree_encoding(state->istate, path, fd);
+#endif
+
wrote = write_in_full(fd, new_blob, size);
if (!to_tempfile)
fstat_done = fstat_checkout_output(fd, state, &st);
diff --git a/environment.c b/environment.c
index bb3c2a96a33..2e4d3a1e058 100644
--- a/environment.c
+++ b/environment.c
@@ -51,6 +51,9 @@ const char *git_hooks_path;
int zlib_compression_level = Z_BEST_SPEED;
int pack_compression_level = Z_DEFAULT_COMPRESSION;
int fsync_object_files = -1;
+#ifdef __MVS__
+int ignore_file_tags = 0;
+#endif
int use_fsync = -1;
enum fsync_method fsync_method = FSYNC_METHOD_DEFAULT;
enum fsync_component fsync_components = FSYNC_COMPONENTS_DEFAULT;
diff --git a/git-compat-util.h b/git-compat-util.h
index 3e7a59b5ff1..66e0abec24b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -223,7 +223,15 @@ struct strbuf;
#include <sys/stat.h>
#include <fcntl.h>
#include <stddef.h>
+#ifdef __MVS__
+#define release stdlib_release
+#define fetch stdlib_fetch
+#endif
#include <stdlib.h>
+#ifdef __MVS__
+#undef fetch
+#undef release
+#endif
#include <stdarg.h>
#include <string.h>
#ifdef HAVE_STRINGS_H
diff --git a/negotiator/default.c b/negotiator/default.c
index 9a5b6963272..b1f9f153372 100644
--- a/negotiator/default.c
+++ b/negotiator/default.c
@@ -174,7 +174,7 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
return known_to_be_common;
}
-static void release(struct fetch_negotiator *n)
+static void release_negotiator(struct fetch_negotiator *n)
{
clear_prio_queue(&((struct negotiation_state *)n->data)->rev_list);
FREE_AND_NULL(n->data);
@@ -187,7 +187,7 @@ void default_negotiator_init(struct fetch_negotiator *negotiator)
negotiator->add_tip = add_tip;
negotiator->next = next;
negotiator->ack = ack;
- negotiator->release = release;
+ negotiator->release_negotiator = release_negotiator;
negotiator->data = CALLOC_ARRAY(ns, 1);
ns->rev_list.compare = compare_commits_by_commit_date;
diff --git a/negotiator/noop.c b/negotiator/noop.c
index de39028ab7f..82089654d8b 100644
--- a/negotiator/noop.c
+++ b/negotiator/noop.c
@@ -30,7 +30,7 @@ static int ack(struct fetch_negotiator *n UNUSED, struct commit *c UNUSED)
return 0;
}
-static void release(struct fetch_negotiator *n UNUSED)
+static void release_negotiator(struct fetch_negotiator *n UNUSED)
{
/* nothing to release */
}
@@ -41,6 +41,6 @@ void noop_negotiator_init(struct fetch_negotiator *negotiator)
negotiator->add_tip = add_tip;
negotiator->next = next;
negotiator->ack = ack;
- negotiator->release = release;
+ negotiator->release_negotiator = release_negotiator;
negotiator->data = NULL;
}
diff --git a/negotiator/skipping.c b/negotiator/skipping.c
index 5b91520430c..783b3f27e63 100644
--- a/negotiator/skipping.c
+++ b/negotiator/skipping.c
@@ -243,7 +243,7 @@ static int ack(struct fetch_negotiator *n, struct commit *c)
return known_to_be_common;
}
-static void release(struct fetch_negotiator *n)
+static void release_negotiator(struct fetch_negotiator *n)
{
clear_prio_queue(&((struct data *)n->data)->rev_list);
FREE_AND_NULL(n->data);
@@ -256,7 +256,7 @@ void skipping_negotiator_init(struct fetch_negotiator *negotiator)
negotiator->add_tip = add_tip;
negotiator->next = next;
negotiator->ack = ack;
- negotiator->release = release;
+ negotiator->release_negotiator = release_negotiator;
negotiator->data = CALLOC_ARRAY(data, 1);
data->rev_list.compare = compare;
diff --git a/object-file.c b/object-file.c
index 7c7afe57936..28e69ed1e33 100644
--- a/object-file.c
+++ b/object-file.c
@@ -43,7 +43,9 @@
#include "setup.h"
#include "submodule.h"
#include "fsck.h"
-
+#ifdef __MVS__
+#include <_Ccsid.h>
+#endif
/* The maximum size for an object header. */
#define MAX_HEADER_LEN 32
@@ -2478,6 +2480,68 @@ int index_fd(struct index_state *istate, struct object_id *oid,
return ret;
}
+#ifdef __MVS__
+void validate_codeset(struct index_state *istate, const char *path, int* autoconvertToASCII) {
+ struct conv_attrs ca;
+ struct stat st;
+ unsigned short attr_ccsid;
+ unsigned short file_ccsid;
+
+ if (ignore_file_tags)
+ return;
+
+ *autoconvertToASCII = 0;
+ convert_attrs(istate, &ca, path);
+ if (ca.attr_action == CRLF_BINARY) {
+ attr_ccsid = FT_BINARY;
+ }
+ else if (ca.working_tree_encoding) {
+ attr_ccsid = __toCcsid(ca.working_tree_encoding);
+ }
+ else
+ attr_ccsid = 819;
+
+ if (stat(path, &st) < 0)
+ return;
+
+ file_ccsid = st.st_tag.ft_ccsid;
+
+ if (file_ccsid == FT_UNTAGGED) {
+ die("File %s is untagged, set the correct file tag (using the chtag command).", path);
+ }
+
+ if (attr_ccsid != file_ccsid) {
+ if (file_ccsid == 1047 && attr_ccsid == 819) {
+ *autoconvertToASCII = 1;
+ return;
+ }
+ // Allow tag mixing of 819 and 1208
+ if ((file_ccsid == 819 || file_ccsid == 1208) && (attr_ccsid == 1208 || attr_ccsid == 819)) {
+ return;
+ }
+ // Don't check for binary files, just add them
+ if (attr_ccsid == FT_BINARY)
+ return;
+
+ char attr_csname[_XOPEN_PATH_MAX] = {0};
+ char file_csname[_XOPEN_PATH_MAX] = {0};
+ if (attr_ccsid != FT_BINARY) {
+ __toCSName(attr_ccsid, attr_csname);
+ } else {
+ snprintf(attr_csname, _XOPEN_PATH_MAX, "%s", "binary");
+ }
+ if (file_ccsid != FT_BINARY) {
+ __toCSName(file_ccsid, file_csname);
+ } else {
+ snprintf(file_csname, _XOPEN_PATH_MAX, "%s", "binary");
+ }
+ die("%s added file: file tag (%s) does not match working-tree-encoding (%s)", path, file_csname, attr_csname);
+ }
+}
+#endif
+
+
+
int index_path(struct index_state *istate, struct object_id *oid,
const char *path, struct stat *st, unsigned flags)
{
@@ -2485,11 +2549,25 @@ int index_path(struct index_state *istate, struct object_id *oid,
struct strbuf sb = STRBUF_INIT;
int rc = 0;
+#ifdef __MVS__
+ struct conv_attrs ca;
+ int autocvtToASCII;
+#endif
+
switch (st->st_mode & S_IFMT) {
case S_IFREG:
+#ifdef __MVS__
+ validate_codeset(istate, path, &autocvtToASCII);
+#endif
fd = open(path, O_RDONLY);
if (fd < 0)
return error_errno("open(\"%s\")", path);
+
+#ifdef __MVS__
+ if (!autocvtToASCII)
+ __disableautocvt(fd);
+#endif
+
if (index_fd(istate, oid, fd, st, OBJ_BLOB, path, flags) < 0)
return error(_("%s: failed to insert into database"),
path);
diff --git a/read-cache.c b/read-cache.c
index 080bd39713b..75c06121302 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -205,6 +205,9 @@ static int ce_compare_data(struct index_state *istate,
int fd = git_open_cloexec(ce->name, O_RDONLY);
if (fd >= 0) {
+#ifdef __MVS__
+ __disableautocvt(fd);
+#endif
struct object_id oid;
if (!index_fd(istate, &oid, fd, st, OBJ_BLOB, ce->name, 0))
match = !oideq(&oid, &ce->oid);
diff --git a/utf8.c b/utf8.c
index 6a0dd25b0fe..b9cb56abf14 100644
--- a/utf8.c
+++ b/utf8.c
@@ -590,6 +590,17 @@ char *reencode_string_len(const char *in, size_t insz,
#endif
}
+#ifdef __MVS__
+ //HACK: For backwards compat, ISO8859-1 really means utf-8 in the z/OS world
+ if (strcasecmp("ISO8859-1", in_encoding) == 0) {
+ in_encoding = "UTF-8";
+ out_encoding = "UTF-8";
+ }
+ if (strcasecmp("ISO8859-1", out_encoding) == 0) {
+ in_encoding = "UTF-8";
+ out_encoding = "UTF-8";
+ }
+#endif
conv = iconv_open(out_encoding, in_encoding);
if (conv == (iconv_t) -1) {
in_encoding = fallback_encoding(in_encoding);
--
gitgitgadget
^ 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