* Re: [Potential Bug] Test t0301.34 hangs - Git v2.43.0-rc2
From: Todd Zullinger @ 2023-11-14 21:10 UTC (permalink / raw)
To: rsbecker; +Cc: 'Junio C Hamano', git
In-Reply-To: <053501da173c$ac5b4470$0511cd50$@nexbridge.com>
rsbecker@nexbridge.com wrote:
> When running the full suite, I found that t0301.34 hangs
> on NonStop x86 (Big Endian). No details at this point -
> will rerun this, but this is a regression from rc1.
FWIW, this test ran fine on Fedora's s390x architecture.
That's little solace, I know, but may help rule out some
potential causes.
t0301-credential-cache.sh ..........................
...
ok 34 - helper (cache) can forget user
...
# passed all 44 test(s)
The build log is available here (for a few weeks or so -- it
was only a test build):
https://kojipkgs.fedoraproject.org//work/tasks/4976/109024976/build.log
--
Todd
^ permalink raw reply
* Re: [PATCH] ci: avoid running the test suite _twice_
From: Josh Steadmon @ 2023-11-14 21:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Johannes Schindelin via GitGitGadget, Phillip Wood,
git, Johannes Schindelin
In-Reply-To: <xmqq4jhp438y.fsf@gitster.g>
On 2023.11.14 08:55, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > 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.
>
> Very good points. Josh?
In short, the last time I tried to add something to CI, it was not well
received, so I've been perhaps overly cautious in keeping the unit-tests
well-separated from other targets. But I can send a follow-up patch to
fold them into `make test`. Or would you prefer that I send a v11 of
js/doc-unit-tests instead?
^ permalink raw reply
* [PATCH v5 0/3] rebase: support --autosquash without -i
From: Andy Koppe @ 2023-11-14 21:43 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood, Andy Koppe
In-Reply-To: <20231111132720.78877-1-andy.koppe@gmail.com>
Make rebase --autosquash work without --interactive, but limit
rebase.autoSquash's effects to interactive mode, and improve testing
and documentation.
Changes from v4:
- Fix amend vs apply backend thinko in commit messages.
- Squash patch 3 for testing into patch 2 and improve the commit
message.
- No source changes.
Thanks again to Junio and Phillip for their reviews.
Andy Koppe (3):
rebase: fully ignore rebase.autoSquash without -i
rebase: support --autosquash without -i
rebase: rewrite --(no-)autosquash documentation
Documentation/config/rebase.txt | 4 ++-
Documentation/git-rebase.txt | 34 +++++++++++++----------
builtin/rebase.c | 17 +++++-------
t/t3415-rebase-autosquash.sh | 38 +++++++++++++++++++-------
t/t3422-rebase-incompatible-options.sh | 12 --------
5 files changed, 58 insertions(+), 47 deletions(-)
--
2.43.0-rc2
^ permalink raw reply
* [PATCH v5 1/3] rebase: fully ignore rebase.autoSquash without -i
From: Andy Koppe @ 2023-11-14 21:43 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood, Andy Koppe
In-Reply-To: <20231114214339.10925-1-andy.koppe@gmail.com>
Setting the rebase.autoSquash config variable to true implies a couple
of restrictions: it prevents preemptive fast-forwarding and it triggers
conflicts with apply 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.
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 &&
--
2.43.0-rc2
^ permalink raw reply related
* [PATCH v5 2/3] rebase: support --autosquash without -i
From: Andy Koppe @ 2023-11-14 21:43 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood, Andy Koppe
In-Reply-To: <20231114214339.10925-1-andy.koppe@gmail.com>
The rebase --autosquash option is quietly ignored when used without
--interactive (apart from preventing preemptive fast-forwarding and
triggering conflicts with apply backend options).
Change that to support --autosquash without --interactive, by dropping
its restriction to REBASE_INTERACTIVE_EXCPLICIT mode. When used this
way, auto-squashing is done without opening the todo list editor.
Drop the -i requirement from the --autosquash description, and amend
t3415-rebase-autosquash.sh to test the option and the rebase.autoSquash
config variable with and without -i.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/git-rebase.txt | 2 +-
builtin/rebase.c | 4 +---
t/t3415-rebase-autosquash.sh | 38 ++++++++++++++++++++++++++----------
3 files changed, 30 insertions(+), 14 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);
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' '
--
2.43.0-rc2
^ permalink raw reply related
* [PATCH v5 3/3] rebase: rewrite --(no-)autosquash documentation
From: Andy Koppe @ 2023-11-14 21:43 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood, Andy Koppe
In-Reply-To: <20231114214339.10925-1-andy.koppe@gmail.com>
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.
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.
--
2.43.0-rc2
^ permalink raw reply related
* [PATCH 1/1] attr: add builtin objectmode values support
From: Joanna Wang @ 2023-11-14 21:49 UTC (permalink / raw)
To: gitster; +Cc: git, jojwang, tboegi
In-Reply-To: <xmqqbkbx11x2.fsf@gitster.g>
Gives all paths builtin objectmode 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:builtin_objectmode=160000)' could filter for submodules without
needing to have `builtin_objectmode=160000` to be set in .gitattributes
for every submodule path.
These values are also reflected in `git check-attr` results.
If the git_attr_direction is set to GIT_ATTR_INDEX or GIT_ATTR_CHECKIN
and a path is not found in the index, the value will be unspecified.
This patch also reserves the builtin_* attribute namespace for objectmode
and any future builtin attributes. Any user defined attributes using this
reserved namespace will result in a warning. This is a breaking change for
any existing builtin_* attributes.
Pathspecs with some builtin_* attribute name (excluding builtin_objectmode)
will behave like any attribute where there are no user specified values.
Signed-off-by: Joanna Wang <jojwang@google.com>
---
>But the usual practice is *not* to cover all paths with explicit
> attribute definition, isn't it?
Got it, the breaking change is that we're giving mode a value when
user may expect no value.
>When checking things out of the index, the index should be the
>source of the truth. If something is not in the index, is there a
>point in falling back to the workint tree state to decide if the
>thing should be checked out of the index?
I don't think working tree objectmode fallback makes sense for
GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX. Updated patch implements
without fallback.
> static struct git_attr mode_attr;
>
> if (!mode_attr)
> mode_attr = git_attr("mode");
Is there an idiomatic way to check if this git_attr (struct or pointer)
has been initialized? I could not find an anything for C but maybe I've
missed a common way to do this in the codebase.
I also addressed the style nits. Thanks for the review/fedback so far.
Documentation/gitattributes.txt | 14 +++++++
attr.c | 67 +++++++++++++++++++++++++++++++--
t/t0003-attributes.sh | 44 ++++++++++++++++++++++
t/t6135-pathspec-with-attrs.sh | 25 ++++++++++++
4 files changed, 147 insertions(+), 3 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 8c1793c148..84bad3e741 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -100,6 +100,20 @@ for a path to `Unspecified` state. This can be done by listing
the name of the attribute prefixed with an exclamation point `!`.
+RESERVED BUILTIN_* ATTRIBUTES
+-----------------------------
+
+builtin_* is a reserved namespace for builtin attribute values. Any
+user defined attributes under this namespace will result in a warning.
+
+`builtin_objectmode`
+^^^^^^^^^^^^^^^^^^^^
+This attribute is for filtering files by their file bit modes (40000,
+120000, 160000, 100755, 100644). e.g. ':(attr:builtin_objectmode=160000)'.
+You may also check these values with `git check-attr builtin_objectmode -- <file>`.
+If the object is not in the index `git check-attr --cached` will return unspecified.
+
+
EFFECTS
-------
diff --git a/attr.c b/attr.c
index e62876dfd3..8493f2c5c0 100644
--- a/attr.c
+++ b/attr.c
@@ -17,6 +17,7 @@
#include "utf8.h"
#include "quote.h"
#include "read-cache-ll.h"
+#include "refs.h"
#include "revision.h"
#include "object-store-ll.h"
#include "setup.h"
@@ -183,6 +184,15 @@ static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
}
}
+/*
+ * Atribute name cannot begin with "builtin_" which
+ * is a reserved namespace for built in attributes values.
+ */
+static int attr_name_reserved(const char *name)
+{
+ return !strncmp(name, "builtin_", strlen("builtin_"));
+}
+
static int attr_name_valid(const char *name, size_t namelen)
{
/*
@@ -315,7 +325,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
cp++;
len--;
}
- if (!attr_name_valid(cp, len)) {
+ if (!attr_name_valid(cp, len) || attr_name_reserved(cp)) {
report_invalid_attr(cp, len, src, lineno);
return NULL;
}
@@ -379,7 +389,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
name += strlen(ATTRIBUTE_MACRO_PREFIX);
name += strspn(name, blank);
namelen = strcspn(name, blank);
- if (!attr_name_valid(name, namelen)) {
+ if (!attr_name_valid(name, namelen) || attr_name_reserved(name)) {
report_invalid_attr(name, namelen, src, lineno);
goto fail_return;
}
@@ -1240,6 +1250,57 @@ static struct object_id *default_attr_source(void)
return &attr_source;
}
+static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)
+{
+ unsigned int mode;
+ struct strbuf sb = STRBUF_INIT;
+
+ if (direction == GIT_ATTR_CHECKIN) {
+ struct object_id oid;
+ struct stat st;
+ if (lstat(path, &st))
+ die_errno(_("unable to stat '%s'"), path);
+ mode = canon_mode(st.st_mode);
+ if (S_ISDIR(mode)) {
+ /* `path` is either a directory or it is a submodule,
+ * in which case it is already indexed as submodule
+ * or it does not exist in the index yet and we need to
+ * check if we can resolve to a ref.
+ */
+ int pos = index_name_pos(istate, path, strlen(path));
+ if (pos >= 0) {
+ if (S_ISGITLINK(istate->cache[pos]->ce_mode))
+ mode = istate->cache[pos]->ce_mode;
+ } else if (resolve_gitlink_ref(path, "HEAD", &oid) == 0)
+ mode = S_IFGITLINK;
+ }
+ } else {
+ /*
+ * For GIT_ATTR_CHECKOUT and GIT_ATTR_INDEX we only check
+ * for mode in the index.
+ */
+ int pos = index_name_pos(istate, path, strlen(path));
+ if (pos >= 0)
+ mode = istate->cache[pos]->ce_mode;
+ else
+ return ATTR__UNSET;
+ }
+ strbuf_addf(&sb, "%06o", mode);
+ return sb.buf;
+
+}
+
+
+static const char *compute_builtin_attr(struct index_state *istate,
+ const char *path,
+ const struct git_attr *attr) {
+ const struct git_attr *object_mode_attr = git_attr("builtin_objectmode");
+
+ if (attr == object_mode_attr)
+ return builtin_object_mode_attr(istate, path);
+ return ATTR__UNSET;
+}
+
void git_check_attr(struct index_state *istate,
const char *path,
struct attr_check *check)
@@ -1253,7 +1314,7 @@ void git_check_attr(struct index_state *istate,
unsigned int n = check->items[i].attr->attr_nr;
const char *value = check->all_attrs[n].value;
if (value == ATTR__UNKNOWN)
- value = ATTR__UNSET;
+ value = compute_builtin_attr(istate, path, check->all_attrs[n].attr);
check->items[i].value = value;
}
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index aee2298f01..25aa3fbd05 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -19,6 +19,16 @@ attr_check () {
test_must_be_empty err
}
+attr_check_object_mode () {
+ path="$1" &&
+ expect="$2" &&
+ check_opts="$3" &&
+ git check-attr $check_opts builtin_objectmode -- "$path" >actual 2>err &&
+ echo "$path: builtin_objectmode: $expect" >expect &&
+ test_cmp expect actual
+ test_must_be_empty err
+}
+
attr_check_quote () {
path="$1" quoted_path="$2" expect="$3" &&
@@ -558,4 +568,38 @@ test_expect_success EXPENSIVE 'large attributes file ignored in index' '
test_cmp expect err
'
+test_expect_success 'native object mode attributes work' '
+ >exec && chmod +x exec && attr_check_object_mode exec 100755 &&
+ >normal && attr_check_object_mode normal 100644 &&
+ mkdir dir && attr_check_object_mode dir 040000 &&
+ >to_sym ln -s to_sym sym && attr_check_object_mode sym 120000
+'
+
+test_expect_success 'native object mode attributes work with --cached' '
+ >normal && attr_check_object_mode normal unspecified --cached &&
+ git add normal && attr_check_object_mode normal 100644 --cached
+'
+
+test_expect_success 'check object mode attributes work for submodules' '
+ mkdir sub &&
+ (
+ cd sub &&
+ git init &&
+ mv .git .real &&
+ echo "gitdir: .real" >.git &&
+ test_commit first
+ ) &&
+ attr_check_object_mode sub 160000 &&
+ attr_check_object_mode sub unspecified --cached &&
+ git add sub &&
+ attr_check_object_mode sub 160000 --cached
+'
+
+test_expect_success 'we do not allow user defined builtin_* attributes' '
+ echo "foo* builtin_foo" >.gitattributes &&
+ git add .gitattributes 2>actual &&
+ echo "builtin_foo is not a valid attribute name: .gitattributes:1" >expect &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index a9c1e4e0ec..b08a32ea68 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -295,4 +295,29 @@ test_expect_success 'reading from .gitattributes in a subdirectory (3)' '
test_cmp expect actual
'
+test_expect_success 'pathspec with builtin_objectmode attr can be used' '
+ >mode_exec_file_1 &&
+
+ git status -s ":(attr:builtin_objectmode=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:builtin_objectmode=100755)mode_exec_*" >actual &&
+ echo AM mode_exec_file_1 >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'builtin_objectmode attr can be excluded' '
+ >mode_1_regular &&
+ >mode_1_exec && chmod +x mode_1_exec &&
+ git status -s ":(exclude,attr:builtin_objectmode=100644)" "mode_1_*" >actual &&
+ echo ?? mode_1_exec >expect &&
+ test_cmp expect actual &&
+
+ git status -s ":(exclude,attr:builtin_objectmode=100755)" "mode_1_*" >actual &&
+ echo ?? mode_1_regular >expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.43.0.rc0.421.g78406f8d94-goog
^ permalink raw reply related
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Jeff King @ 2023-11-14 21:53 UTC (permalink / raw)
To: Elijah Newren
Cc: Taylor Blau, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <CABPp-BEV8Mxu=4=TFN=0o9n+o69kKQfNZd_Rhr1swxqgKwd90g@mail.gmail.com>
On Mon, Nov 13, 2023 at 06:50:08PM -0800, Elijah Newren wrote:
> merge-tree is the same as far as hooks; I'd rather just not have them,
> but if we did, they'd be a post-operation hook.
>
> In both cases, that makes hooks not much of a sticking point.
>
> External merge drivers, however...
I suspect external merge drivers are OK, in the sense that they should
not be looking up arbitrary objects anyway. We should hand the driver
what it needs via tempfiles, etc.
That said, I'm also OK with the notion that "--write-pack" is optional,
and if some features don't work with it, that's OK. It's nice if we can
flag that explicitly (rather than having things break in weird and
subtle ways), but I think we might not even know about an implicit
assumption until somebody runs across it in practice.
-Peff
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Jeff King @ 2023-11-14 21:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: Taylor Blau, Elijah Newren, git, Eric W. Biederman,
Patrick Steinhardt
In-Reply-To: <xmqqpm0d1591.fsf@gitster.g>
On Tue, Nov 14, 2023 at 10:40:58AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I posted an alternative in response to Elijah; the general idea being to
> > allow the usual object-lookup code to access the in-progress pack. That
> > would keep us limited to a single pack.
>
> If such a mechanism is done in a generic way, would we be able to
> simplify fast-import a lot, I wonder? IIRC, it had quite a lot of
> code to remember what it has written to its output to work around
> the exact issue your alternative tries to solve. In fact, maybe we
> could make fast-import a thin wrapper around the bulk checkin
> infrastructure?
I suspect that the implementation could be shared with fast-import. I'm
not sure it would save all that much code, though. There's a lot going
on in fast-import besides keeping track of which objects we wrote into a
pack. ;)
The bigger issue, though, is that fast-import does generate some deltas
and the bulk checkin code does not. And I'm not sure how the bulk
checkin interface would expose that API (you need the caller to say
"...and I suspect this object might be a good delta base").
-Peff
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Jeff King @ 2023-11-14 22:04 UTC (permalink / raw)
To: Taylor Blau
Cc: Elijah Newren, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <ZVKkgpiFaOwwDcdw@nand.local>
On Mon, Nov 13, 2023 at 05:34:42PM -0500, Taylor Blau wrote:
> > It might not be too hard to just let in-process callers access the
> > objects we've written. A quick and dirty patch is below, which works
> > with the test you suggested (the test still fails because it finds a
> > conflict, but it gets past the "woah, I can't find that sha1" part).
>
> That's a very slick idea, and I think that this series has some legs to
> stand on now as a result.
I'm glad people seem to like it. ;)
I was mostly throwing it out there as an illustration, and I don't plan
on polishing it up further. But in case somebody else wants to work on
it, here are random extra thoughts on the topic:
- rather than teach packfile.c about bulk checkin, I think it might be
cleaner to let packed_git structs have a function pointer for
accessing the index (and if NULL, naturally we'd open the .idx file
in the usual way). And then bulk checkin could just register the
"fake" packed_git
- the flip side, though, is: would it be weird for other parts of the
code to ever see the fake bulk packed_git in the list? It doesn't
represent a real packfile the way the other ones do. So maybe it is
better to keep it separate
- I suspect this scheme may violate some bounds-checking of the
packfiles. For example, we haven't written the hashfile trailer yet in
the still-open packfile. But I think use_pack() assumes we always
have an extra the_hash_algo->rawsz bytes of padding at the end. I'm
not sure if that would ever cause us to accidentally read past the
end of the file.
- as you mentioned, an oidmap would be an obvious replacement for the
existing unsorted list
- the existing already_written() function faces the same problem. I
don't think anybody cared because we'd usually only bulk checkin a
few huge objects. But with --write-pack, we might write a lot of
such objects, and the linear scan in already_written() makes it
accidentally quadratic.
- speaking of already_written(): it calls repo_has_object_file() to
see if we can elide the write. It probably should be using
freshen_*_object() in the usual way. Again, this is an existing bug
but I suspect nobody noticed because the bulk checkin code path
hardly ever kicks in.
-Peff
^ permalink raw reply
* RE: [Potential Bug] Test t0301.34 hangs - Git v2.43.0-rc2
From: rsbecker @ 2023-11-14 22:18 UTC (permalink / raw)
To: 'Todd Zullinger'; +Cc: 'Junio C Hamano', git
In-Reply-To: <ZVPiQKhem7ew8o_8@pobox.com>
On Tuesday, November 14, 2023 4:10 PM, Todd Zullinger wrote:
>rsbecker@nexbridge.com wrote:
>> When running the full suite, I found that t0301.34 hangs on NonStop
>> x86 (Big Endian). No details at this point - will rerun this, but this
>> is a regression from rc1.
>
>FWIW, this test ran fine on Fedora's s390x architecture.
>That's little solace, I know, but may help rule out some potential causes.
>
> t0301-credential-cache.sh ..........................
> ...
> ok 34 - helper (cache) can forget user
> ...
> # passed all 44 test(s)
>
>The build log is available here (for a few weeks or so -- it was only a
test build):
>
>https://kojipkgs.fedoraproject.org//work/tasks/4976/109024976/build.log
Well... it looks transient. I reran the test with and without verbose in ksh
and bash without any difficulty. It might just be a timing issue.
I guess we can ignore this one for now unless it becomes persistent.
^ permalink raw reply
* [PATCH] RelNotes: tweak 2.43.0 release notes
From: Andy Koppe @ 2023-11-14 22:31 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
Add some more detail on the $(decorate) log format placeholder and tweak
the wording on some other points.
---
Documentation/RelNotes/2.43.0.txt | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/Documentation/RelNotes/2.43.0.txt b/Documentation/RelNotes/2.43.0.txt
index 770543c464..9a68074a4a 100644
--- a/Documentation/RelNotes/2.43.0.txt
+++ b/Documentation/RelNotes/2.43.0.txt
@@ -10,8 +10,8 @@ Backward Compatibility Notes
prefix. If you are negatively affected by this change, please use
"--subject-prefix=PATCH --rfc" as a replacement.
- * "git rev-list --stdin" learned to take non-revisions (like "--not")
- recently from the standard input, but the way such a "--not" was
+ * "git rev-list --stdin" recently learned to take non-revisions (like
+ "--not") from the standard input, but the way such a "--not" was
handled was quite confusing, which has been rethought. The updated
rule is that "--not" given from the command line only affects revs
given from the command line that comes but not revs read from the
@@ -43,10 +43,10 @@ UI, Workflows & Features
* Git GUI updates.
- * "git format-patch" learns a way to feed cover letter description,
- that (1) can be used on detached HEAD where there is no branch
- description available, and (2) also can override the branch
- description if there is one.
+ * "git format-patch" learns option "--description-file" to feed in a
+ cover letter description that can be used when no branch description
+ is available, or that can override the branch description if there is
+ one.
* Use of --max-pack-size to allow multiple packfiles to be created is
now supported even when we are sending unreachable objects to cruft
@@ -56,7 +56,9 @@ UI, Workflows & Features
"--subject-prefix" option and used "[RFC PATCH]"; now we will add
"RFC" prefix to whatever subject prefix is specified.
- * "git log --format" has been taught the %(decorate) placeholder.
+ * "git log" format strings now support a "%(decorate)" placeholder that
+ can be used to customize the symbols and the tag prefix used in ref
+ decorations.
* The default log message created by "git revert", when reverting a
commit that records a revert, has been tweaked, to encourage people
@@ -99,7 +101,7 @@ UI, Workflows & Features
* The attribute subsystem learned to honor `attr.tree` configuration
that specifies which tree to read the .gitattributes files from.
- * "git merge-file" learns a mode to read three contents to be merged
+ * "git merge-file" learns a mode to read three files to be merged
from blob objects.
@@ -127,7 +129,7 @@ Performance, Internal Implementation, Development Support etc.
* The code to keep track of existing packs in the repository while
repacking has been refactored.
- * The "streaming" interface used for bulk-checkin codepath has been
+ * The "streaming" interface used for the bulk-checkin codepath has been
narrowed to take only blob objects for now, with no real loss of
functionality.
--
2.43.0-rc2
^ permalink raw reply related
* Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof
From: Jiří Hruška @ 2023-11-14 23:16 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Jeff King
In-Reply-To: <20231113212243.1495815-1-jonathantanmy@google.com>
Thank you for the comments,
> Yes, please split non-functional changes into a separate commit
> (preferably one for each concern). I do envision reviewers saying "let's
> put patches X, Y, and Z in, but not patches A, B, and C", so splitting
> would make it easier to decide what's worthwhile to have.
Sure, will do
^ permalink raw reply
* Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof
From: Jiří Hruška @ 2023-11-14 23:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git, Jeff King
In-Reply-To: <xmqqv8a515ge.fsf@gitster.g>
> Is this a bug on our side, or cURL library calling us when it should not?
I thought the same and yes, I suppose it _might_ be potentially considered
a bug on cURL side in the first place. But then also
1/ it is not mandated anywhere in the API that the callback will never be
called again after already getting an EOF once,
2/ I looked at libcurl code and it was not entirely clear to me that the
behavior would be accidental, that it could be clearly called as a bug,
3/ anything that follows how fread() works would never be affected,
but git-remote-curl is, because it does something differently,
4/ even if it gets fixed in libcurl today, people might be building Git with
whatever old versions of the library for years to come,
so worth fixing here in any case (imho).
But I'll reach out to curl-library and get their opinion, so that we have
a full picture here. Thanks
^ permalink raw reply
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Junio C Hamano @ 2023-11-15 0:44 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git, Karthik Nayak
In-Reply-To: <20231114194310.GC2092538@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> What I'm getting as it that I think we have three options for v2.43:
>
> 1. Ship what has been in the release candidates, which has a known
> performance regression (though the severity is up for debate).
>
> 2. Flip the default to "0" (i.e., Patrick's patch in this thread). We
> know that loosens some cases versus 2.42, which may be considered a
> regression.
>
> 3. Sort it out before the release. We're getting pretty close to do
> a lot new work there, but I think the changes should be small-ish.
> The nuclear option is ejecting the topic and re-doing it in the
> next cycle.
>
> I don't have a really strong preference between the three.
I've been (naively) assuming that #1 is everybody's preference,
simply because #2 does introduce a regression in the correctness
department (as opposed to a possible performance regression caused
by #1), and because #3 has a high risk of screwing up.
As long as the performance regression is known and on our radar,
I'd say that working on a maintenance release after Thanksgiving
would be sufficient.
I might be underestimating the impact of the loss of performance,
though, in which case I'd consider that nuclear one, which is the
simplest and least risky.
Thanks.
^ permalink raw reply
* Re: [PATCH] send-email: avoid duplicate specification warnings
From: Junio C Hamano @ 2023-11-15 0:48 UTC (permalink / raw)
To: Todd Zullinger
Cc: Jeff King, git, Ævar Arnfjörð Bjarmason,
Ondřej Pohořelský
In-Reply-To: <ZVPfvjoXyGVlKqvr@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> Since this isn't anything new with 2.43, it doesn't need to
> be fixed with much urgency.
True. Unless the new version of Getopt::Long is quickly spreading
through our user base, that is.
> Thanks both,
Thanks for spotting the issue and acting on it quickly.
^ permalink raw reply
* Re: [PATCH] ci: avoid running the test suite _twice_
From: Junio C Hamano @ 2023-11-15 1:00 UTC (permalink / raw)
To: Josh Steadmon
Cc: Jeff King, Johannes Schindelin via GitGitGadget, Phillip Wood,
git, Johannes Schindelin
In-Reply-To: <ZVPm0qn6XsbLL8eM@google.com>
Josh Steadmon <steadmon@google.com> writes:
> On 2023.11.14 08:55, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>>
>> > 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.
>>
>> Very good points. Josh?
>
> In short, the last time I tried to add something to CI, it was not well
> received, so I've been perhaps overly cautious in keeping the unit-tests
> well-separated from other targets. But I can send a follow-up patch to
> fold them into `make test`. Or would you prefer that I send a v11 of
> js/doc-unit-tests instead?
Incremental patches to update what is in 'next' would let us try out
the new arragement to drive the tests from the main "make test"
eaarlier. Post release, a new iteration could replace the series
wholesale as we will have an opportunity to rebuild 'next', but it
would be nice for the end states to match, if you were to do both.
Thanks.
^ permalink raw reply
* Re: [PATCH] commit-graph: disable GIT_COMMIT_GRAPH_PARANOIA by default
From: Jeff King @ 2023-11-15 1:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git, Karthik Nayak
In-Reply-To: <xmqq4jhnyhe1.fsf@gitster.g>
On Wed, Nov 15, 2023 at 09:44:38AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > What I'm getting as it that I think we have three options for v2.43:
> >
> > 1. Ship what has been in the release candidates, which has a known
> > performance regression (though the severity is up for debate).
> >
> > 2. Flip the default to "0" (i.e., Patrick's patch in this thread). We
> > know that loosens some cases versus 2.42, which may be considered a
> > regression.
> >
> > 3. Sort it out before the release. We're getting pretty close to do
> > a lot new work there, but I think the changes should be small-ish.
> > The nuclear option is ejecting the topic and re-doing it in the
> > next cycle.
> >
> > I don't have a really strong preference between the three.
>
> I've been (naively) assuming that #1 is everybody's preference,
> simply because #2 does introduce a regression in the correctness
> department (as opposed to a possible performance regression caused
> by #1), and because #3 has a high risk of screwing up.
>
> As long as the performance regression is known and on our radar,
> I'd say that working on a maintenance release after Thanksgiving
> would be sufficient.
>
> I might be underestimating the impact of the loss of performance,
> though, in which case I'd consider that nuclear one, which is the
> simplest and least risky.
I am fine with #1 for the release. Mostly I just wanted to understand
what the plan was (and if we needed to be hurrying to try to make the
non-nuclear #3 work).
-Peff
^ permalink raw reply
* [PATCH v2 0/5] Avoid hang if curl needs eof twice + minor related improvements
From: Jiří Hruška @ 2023-11-15 3:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Junio C Hamano
In-Reply-To: <CAGE_+C6DJMAO0bj5QHoKBBV3gMEMtZ-ajJ9ZnDGYq6eorr-oig@mail.gmail.com>
Proposed changes split into several commits for clarity
Jiri Hruska (5):
remote-curl: avoid hang if curl asks for more data after eof
remote-curl: improve readability of curl callbacks
remote-curl: simplify rpc_out() - remove superfluous ifs
remote-curl: simplify rpc_out() - less nesting and rename
http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests
http.c | 1 +
remote-curl.c | 99 +++++++++++++++++++++++++++++----------------------
2 files changed, 58 insertions(+), 42 deletions(-)
--
2.42.1.5.g2f21867bd5
^ permalink raw reply
* [PATCH v2 2/5] remote-curl: improve readability of curl callbacks
From: Jiří Hruška @ 2023-11-15 3:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Junio C Hamano
In-Reply-To: <20231115033121.939-1-jirka@fud.cz>
The "user data" or "context" argument of libcurl streaming callbacks
is sometimes called `clientp` and sometimes `buffer_`. The latter is
especially confusing, because there is an actual buffer pointer
argument passed to the same functions.
- Make the argument consistently named `userdata` everywhere, just
like the official cURL documentation calls it.
- Also add comments to all the callbacks, to make it easier to grasp
what is the "in" and what is the "out" direction in this code.
Signed-off-by: Jiri Hruska <jirka@fud.cz>
---
remote-curl.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 199c4615a5..428dd70aa1 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -675,11 +675,18 @@ 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;
@@ -725,9 +732,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);
@@ -797,14 +811,15 @@ struct rpc_in_data {
};
/*
- * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
- * from ptr.
+ * CURLOPT_WRITEFUNCTION callback, called when more received data has come in.
+ * Has the same semantics as fwrite(), but writes packets to the pipe to the
+ * child process instead. The return value is the bytes consumed 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,
--
2.42.1.5.g2f21867bd5
^ permalink raw reply related
* [PATCH v2 1/5] remote-curl: avoid hang if curl asks for more data after eof
From: Jiří Hruška @ 2023-11-15 3:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Junio C Hamano
In-Reply-To: <20231115033121.939-1-jirka@fud.cz>
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.
Because `rpc->flush_read_but_not_sent` is reset to false immediately
the first time an EOF is returned, the repeated call goes again to
`rpc_read_from_out()`, which tries to read more from the child process
pipe and 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.
The bug is fixed by moving the reset of the `flush_read_but_not_sent`
flag to `post_rpc()`, only before `rpc_out()` would be potentially used
the next time. This makes the callback behave like fread() and return
a zero any number of times at the end of a finished upload.
Signed-off-by: Jiri Hruska <jirka@fud.cz>
---
remote-curl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index ef05752ca5..199c4615a5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -705,9 +705,10 @@ static size_t rpc_out(void *ptr, size_t eltsize,
* 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.
+ * been fully sent. It is important to keep returning 0
+ * as long as needed in that case, as libcurl invokes
+ * the callback multiple times at EOF sometimes.
*/
- rpc->flush_read_but_not_sent = 0;
return 0;
}
/*
@@ -963,6 +964,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->flush_read_but_not_sent = 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);
--
2.42.1.5.g2f21867bd5
^ permalink raw reply related
* [PATCH v2 3/5] remote-curl: simplify rpc_out() - remove superfluous ifs
From: Jiří Hruška @ 2023-11-15 3:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Junio C Hamano
In-Reply-To: <20231115033121.939-1-jirka@fud.cz>
Remove the second set of nested `if` statements and an early `return`
in `rpc_out()`, because they accomplish nothing. The rest of the
function would behave the same without any branching.
Signed-off-by: Jiri Hruska <jirka@fud.cz>
---
remote-curl.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 428dd70aa1..690df2a43e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -706,25 +706,13 @@ static size_t rpc_out(void *ptr, size_t eltsize,
* 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. It is important to keep returning 0
- * as long as needed in that case, as libcurl invokes
- * the callback multiple times at EOF sometimes.
- */
- 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.
- */
- }
+ /*
+ * 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);
--
2.42.1.5.g2f21867bd5
^ permalink raw reply related
* [PATCH v2 4/5] remote-curl: simplify rpc_out() - less nesting and rename
From: Jiří Hruška @ 2023-11-15 3:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Junio C Hamano
In-Reply-To: <20231115033121.939-1-jirka@fud.cz>
- Remove one indentation level in `rpc_out()`, as the conditions can be
combined into just one `if` statement without losing readability.
Skipping the resetting of `initial_buffer`/`len`/`pos` revealed a bug
in `stateless_connect()`, where `rpc.len` is reset to 0 after each
request, but not `rpc.pos`. Relying on `rpc_out()` always doing this
before has never been safe (it might have not finished cleanly, for
example). So better reset it there, just like `rpc.len`.
- Rename `flush_read_but_not_sent` to `read_from_out_done`. The name is
slightly misleading, because the "flush" might never be really "sent"
(depends on `write_line_lengths`), and this is not the most important
part anyway. The primary role of the flag is rather to signal that
`read_from_out()` is "done" and must not be called for this particular
RPC exchange anymore.
- Update/add some related comments.
Signed-off-by: Jiri Hruska <jirka@fud.cz>
---
remote-curl.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 690df2a43e..d5aa66a44c 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 }
@@ -690,21 +691,29 @@ static size_t rpc_out(void *ptr, size_t eltsize,
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 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.
*/
+ if (status == PACKET_READ_FLUSH)
+ rpc->read_from_out_done = 1;
}
/*
@@ -967,7 +976,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->flush_read_but_not_sent = 0;
+ 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);
@@ -1487,7 +1496,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
@@ -1510,6 +1519,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.5.g2f21867bd5
^ permalink raw reply related
* [PATCH v2 5/5] http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests
From: Jiří Hruška @ 2023-11-15 3:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Jonathan Tan, Junio C Hamano
In-Reply-To: <20231115033121.939-1-jirka@fud.cz>
`get_active_slot()` makes sure that the reused cURL handles it gives
out are as good as fresh ones, by resetting all options that other code
might have set on them back to defaults.
But this does not apply to `CURLOPT_POSTFIELDSIZE_LARGE` yet, which can
stay set from a previous request. For example, an earlier probe request
with just a flush packet "0000" leaves it set to 4.
The problem seems harmless in practice, but it can be confusing to see
a negative amount of remaining bytes to upload when inspecting libcurl
internals while debugging networking-related issues, for example.
So reset also this option to its default value (which is -1, not 0).
Signed-off-by: Jiri Hruska <jirka@fud.cz>
---
http.c | 1 +
1 file changed, 1 insertion(+)
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);
--
2.42.1.5.g2f21867bd5
^ permalink raw reply related
* Re: [PATCH v2 5/5] http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests
From: Patrick Steinhardt @ 2023-11-15 6:44 UTC (permalink / raw)
To: Jiří Hruška; +Cc: git, Jeff King, Jonathan Tan, Junio C Hamano
In-Reply-To: <CAGE_+C5pnASOsrDr4ehNj-deYbSTr=pRgPcWqq5VSQs-Y08ttQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]
On Tue, Nov 14, 2023 at 07:34:55PM -0800, Jiří Hruška wrote:
> `get_active_slot()` makes sure that the reused cURL handles it gives
> out are as good as fresh ones, by resetting all options that other code
> might have set on them back to defaults.
>
> But this does not apply to `CURLOPT_POSTFIELDSIZE_LARGE` yet, which can
> stay set from a previous request. For example, an earlier probe request
> with just a flush packet "0000" leaves it set to 4.
>
> The problem seems harmless in practice, but it can be confusing to see
> a negative amount of remaining bytes to upload when inspecting libcurl
> internals while debugging networking-related issues, for example.
>
> So reset also this option to its default value (which is -1, not 0).
>
> Signed-off-by: Jiri Hruska <jirka@fud.cz>
> ---
> http.c | 1 +
> 1 file changed, 1 insertion(+)
>
> 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);
It feels quite easy for this list to grow stale whenever we start to set
a new option somewhere else. Is there a specific reason why we can't
instead use `curl_easy_reset()` here? Quoting its description:
> Re-initializes all options previously set on a specified CURL handle
> to the default values. This puts back the handle to the same state as
> it was in when it was just created with curl_easy_init.
>
> It does not change the following information kept in the handle: live
> connections, the Session ID cache, the DNS cache, the cookies, the
> shares or the alt-svc cache.
From my naive point of view it sounds like exactly what we're after.
Most of the code in question was introduced in 9094950d73 (http: prevent
segfault during curl handle reuse, 2006-05-31), where we used to support
libcurl at least back to v7.7. `curl_easy_reset()` on the other hand had
only been introduced with v7.12.1 of libcurl, so maybe that's the reason
why it's not used here?
I dunno, might as well be that there is a good reason why we don't use
it here. But if we can, then I'd argue it would be a great cleanup to
convert to `curl_easy_reset()` here instead of piling onto the list of
options.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox