* [PATCH 3/3] branch: rename orphan branches in any worktree
From: Rubén Justo @ 2023-01-16 0:04 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com>
In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)
we added support for renaming an orphan branch, skipping rename_ref() if
the branch to rename is an orphan branch, checking the current HEAD.
But the orphan branch to be renamed can be an orphan branch in a
different working tree than the current one, i.e. not the current HEAD.
Let's make "ishead_reject_rebase_or_bisect_branch()" return a flag
indicating if the returned value refers to an orphan branch, and use it
to extend what we did in cfaff3aac, to all HEADs in the repository.
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
builtin/branch.c | 5 ++---
t/t3200-branch.sh | 16 ++++++++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6bb5f50950..7e6baa291a 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -495,7 +495,7 @@ static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
struct worktree *wt = worktrees[i];
if (wt->head_ref && !strcmp(target, wt->head_ref))
- ret = 1;
+ ret = 1 + (is_null_oid(&wt->head_oid) ? 1 : 0);
if (!wt->is_detached)
continue;
@@ -560,8 +560,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
strbuf_addf(&logmsg, "Branch: renamed %s to %s",
oldref.buf, newref.buf);
- if (!copy &&
- (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&
+ if (!copy && !(ishead > 1) &&
rename_ref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch rename failed"));
if (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 5a169b68d6..966583dc7d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -279,6 +279,22 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '
test_cmp expect err
'
+test_expect_success 'git branch -m should work with orphan branches' '
+ test_when_finished git checkout - &&
+ test_when_finished git worktree remove -f wt &&
+ git worktree add wt --detach &&
+
+ # rename orphan in another worktreee
+ git -C wt checkout --orphan orphan-foo-wt &&
+ git branch -m orphan-foo-wt orphan-bar-wt &&
+ test orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&
+
+ # rename orphan in the current worktree
+ git checkout --orphan orphan-foo &&
+ git branch -m orphan-foo orphan-bar &&
+ test orphan-bar=$(git branch --show-current)
+'
+
test_expect_success 'git branch -d on orphan HEAD (merged)' '
test_when_finished git checkout main &&
git checkout --orphan orphan &&
--
2.39.0
^ permalink raw reply related
* [PATCH v2 2/3] branch: description for orphan branch errors
From: Rubén Justo @ 2023-01-16 0:02 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com>
In bcfc82bd48 (branch: description for non-existent branch errors,
2022-10-08) we check the current HEAD to detect if the branch to operate
with is an orphan branch, to avoid the confusing error: "No branch
named...".
If we are asked to operate with an orphan branch in a different working
tree than the current one, we need to check the HEAD in that different
working tree.
Let's extend the check we did in bcfc82db48, to all HEADs in the
repository, using the helper introduced in 31ad6b61bd (branch: add
branch_checked_out() helper, 2022-06-15)
"ishead_reject_rebase_or_bised_branch()" already returns whether the
branch to operate with is HEAD in any working tree in the repository.
Let's use this information in "copy_or_rename_branch()", instead of the
helper.
Note that we now call reject_rebase_or_bisect_branch() earlier, which
introduces a change in the error displayed when a combination of
unsupported conditions occur simultaneously: the desired destination
name is invalid, and the branch to operate on is being overrun or
bisected. With "foo" being rebased or bisected, this:
$ git branch -m foo HEAD
fatal: 'HEAD' is not a valid branch name.
... becomes:
$ git branch -m foo HEAD
fatal: Branch refs/heads/foo is being ...
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
builtin/branch.c | 25 +++++++++++--------------
t/t3202-show-branch.sh | 18 ++++++++++++++++++
2 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index a1ee728ca3..6bb5f50950 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -532,12 +532,12 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
die(_("Invalid branch name: '%s'"), oldname);
}
- if ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {
- if (copy && !strcmp(head, oldname))
- die(_("No commit on branch '%s' yet."), oldname);
- else
- die(_("No branch named '%s'."), oldname);
- }
+ ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
+
+ if ((copy || !ishead) && !ref_exists(oldref.buf))
+ die(ishead
+ ? _("No commit on branch '%s' yet.")
+ : _("No branch named '%s'."), oldname);
/*
* A command like "git branch -M currentbranch currentbranch" cannot
@@ -548,8 +548,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
else
validate_new_branchname(newname, &newref, force);
- ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
-
if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
!skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
BUG("expected prefix missing for refs");
@@ -810,7 +808,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
strbuf_addf(&branch_ref, "refs/heads/%s", branch_name);
if (!ref_exists(branch_ref.buf))
- error((!argc || !strcmp(head, branch_name))
+ error((!argc || branch_checked_out(branch_ref.buf))
? _("No commit on branch '%s' yet.")
: _("No branch named '%s'."),
branch_name);
@@ -854,11 +852,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
die(_("no such branch '%s'"), argv[0]);
}
- if (!ref_exists(branch->refname)) {
- if (!argc || !strcmp(head, branch->name))
- die(_("No commit on branch '%s' yet."), branch->name);
- die(_("branch '%s' does not exist"), branch->name);
- }
+ if (!ref_exists(branch->refname))
+ die((!argc || branch_checked_out(branch->refname))
+ ? _("No commit on branch '%s' yet.")
+ : _("branch '%s' does not exist"), branch->name);
dwim_and_setup_tracking(the_repository, branch->name,
new_upstream, BRANCH_TRACK_OVERRIDE,
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ea7cfd1951..be20ebe1d5 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '
test_cmp expect actual
'
+test_expect_success 'error descriptions on orphan branch' '
+ test_when_finished git worktree remove -f wt &&
+ git worktree add wt --detach &&
+ git -C wt checkout --orphan orphan-branch &&
+ test_branch_op_in_wt() {
+ test_orphan_error() {
+ test_must_fail git $* 2>actual &&
+ test_i18ngrep "No commit on branch .orphan-branch. yet.$" actual
+ } &&
+ test_orphan_error -C wt branch $1 $2 && # implicit branch
+ test_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch
+ test_orphan_error branch $1 orphan-branch $2 # different worktree
+ } &&
+ test_branch_op_in_wt --edit-description &&
+ test_branch_op_in_wt --set-upstream-to=ne &&
+ test_branch_op_in_wt -c new-branch
+'
+
test_done
--
2.39.0
^ permalink raw reply related
* [PATCH v2 1/3] avoid unnecessary worktrees traversing
From: Rubén Justo @ 2023-01-16 0:00 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com>
"reject_rebase_or_bisect_branch()" was introduced [1] to prevent a
branch under bisect or rebase from being renamed or copied. It
traverses all worktrees in the repository and dies if the specified
branch is being rebased or bisected in any them.
"replace_each_worktree_head_symref()" was introduced [2] to adjust the
HEAD in all worktrees after a branch rename succeeded. It traverses all
worktrees in the repository and if any of them have their HEAD pointing
to the renamed ref, it adjusts it.
If we could know in advance if the renamed branch is not HEAD in any
worktree we could avoid calling "replace_each_worktree_head_symref()".
Let's have "reject_rebase_or_bisect_branch()" check and return whether
the specified branch is HEAD in any worktree, and use this information
to avoid unnecessary calls to "replace_each_worktree_head_symref()".
1.- 14ace5b (branch: do not rename a branch under bisect or rebase,
2016-04-22)
2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
builtin/branch.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index f63fd45edb..a1ee728ca3 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -486,14 +486,17 @@ static void print_current_branch_name(void)
die(_("HEAD (%s) points outside of refs/heads/"), refname);
}
-static void reject_rebase_or_bisect_branch(const char *target)
+static int ishead_and_reject_rebase_or_bisect_branch(const char *target)
{
struct worktree **worktrees = get_worktrees();
- int i;
+ int i, ret = 0;
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
+ if (wt->head_ref && !strcmp(target, wt->head_ref))
+ ret = 1;
+
if (!wt->is_detached)
continue;
@@ -507,6 +510,7 @@ static void reject_rebase_or_bisect_branch(const char *target)
}
free_worktrees(worktrees);
+ return ret;
}
static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)
@@ -515,7 +519,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
const char *interpreted_oldname = NULL;
const char *interpreted_newname = NULL;
- int recovery = 0;
+ int recovery = 0, ishead;
if (strbuf_check_branch_ref(&oldref, oldname)) {
/*
@@ -544,7 +548,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
else
validate_new_branchname(newname, &newref, force);
- reject_rebase_or_bisect_branch(oldref.buf);
+ ishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);
if (!skip_prefix(oldref.buf, "refs/heads/", &interpreted_oldname) ||
!skip_prefix(newref.buf, "refs/heads/", &interpreted_newname)) {
@@ -574,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
interpreted_oldname);
}
- if (!copy &&
+ if (!copy && ishead &&
replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
--
2.39.0
^ permalink raw reply related
* [PATCH v2 0/3] branch: operations on orphan branches
From: Rubén Justo @ 2023-01-15 23:54 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
In-Reply-To: <ffd675e9-8a64-ae05-fc3b-36ae99092735@gmail.com>
Avoid some confusing errors operating on orphan branches when
working with worktrees.
Thanks.
Rubén Justo (3):
branch: avoid unnecessary worktrees traversing
branch: description for orphan branch errors
branch: rename orphan branches in any worktree
builtin/branch.c | 40 ++++++++++++++++++++--------------------
t/t3200-branch.sh | 16 ++++++++++++++++
t/t3202-show-branch.sh | 18 ++++++++++++++++++
3 files changed, 54 insertions(+), 20 deletions(-)
--
2.39.0
^ permalink raw reply
* Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
From: Jeff King @ 2023-01-15 23:49 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, git
In-Reply-To: <Y8SIf8AyGrNflBA7@coredump.intra.peff.net>
On Sun, Jan 15, 2023 at 06:13:04PM -0500, Jeff King wrote:
> On Sun, Jan 15, 2023 at 08:54:59PM +0000, Ramsay Jones wrote:
>
> > > - curl_easy_setopt(curl, CURLOPT_PUT, 1);
> > > + curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
> >
> > My version of this patch had '1L' rather than just '1' - but it
> > doesn't really matter (and was probably because all the curl
> > examples did so!).
>
> Yeah, I think it probably ought to be 1L because it's going to a
> variadic function (and it's been a while, but I think the regular
> integer promotions make small things into int, but not into long).
>
> So I don't mind fixing it, but I think we'd want it in a separate patch,
> since it's orthogonal to what's happening here.
I dug a little bit more and as far as I can tell, yes, "1" is
technically violating the standard but will often work depending on the
ABI and endianness of the platform.
That said, there are other cases besides this one, too, so if we want to
tackle it, let's make it a separate topic (but I'm happy to leave it if
nobody's platform is broken).
-Peff
^ permalink raw reply
* Re: ctrl-z ignored by git; creates blobs from non-existent repos
From: Jeff King @ 2023-01-15 23:33 UTC (permalink / raw)
To: Crls; +Cc: Theodore Ts'o, git
In-Reply-To: <Y8SCZvMu7DZZH1Pl@localhost.my.domain>
On Sun, Jan 15, 2023 at 05:47:02PM -0500, Crls wrote:
> > Now, when you type ^Z, the git processes are stopped --- but the
> > objects are created already.
>
> The directories exist not because ^Z is used, but because by the time
> git-clone prompts for a username, git is already set on what to do
> next. Correct? in other words, the process is shoved down my throat.
> Or the user's throat in this case. Or going by another analogy, it
> certainly sounds as if I meant: «Git, please git-clone such and such
> repo, but let me fix just a typo on the repo name before submitting
> it, pretty please» and then Git replies: «too late for that
> chick-a-doodle» and there it goes. It injects blobs all over (well,
> not all over but on the dir specified).
I don't know what you mean by "blobs" here, since we fail to download
any Git objects at all, let alone blobs. But yes, Git creates the local
repository, and then tries to fetch into it. So the directory is created
before it even contacts the remote server at all, and you will see the
same behavior whether the repository exists or not. And then if an error
occurs, it will rolls back by deleting the newly-created repository.
It _could_ be possible to contact the server first, and only when things
looked successful-ish, to create the local repository. But:
1. The clone command is simply not written that way, and converting it
now would be tricky. The clone command's view of the world is that
it makes a new repository, sets up config, etc, then fetches into
it.
2. It would not fix all cases anyway. At some point we have to say
"this looks close enough to success to create the directory", but
things can still go wrong after that.
Now if you have a problem with the rollback, there might be a bug there.
But it sounds like you are simply stopping the process and not letting
it finish. It should roll back even if it receives a signal death, but
^Z is stopping the job and putting it in limbo. If it hurts, don't do it
(or use "fg" or "bg" to let it finish).
> > So what's going on is that github.com is not returning a non-existent
> > repo error; it's prompting for a username/password, as _if_ the
> > repository exists. That's presumably to prevent disclosing
> > information as to whether or not a private repository exists or not.
>
> I agree with you there. Coincidentally speaking, why does a username
> warrants a prompt from git, is simply beyond me. I mean, that is
> certainly the more far-fetched reasoning of implementation I've read
> in a long long time.
>
> Can you git-clone a user? What about the user's settings? What about
> the remainder its gpg tokens and so forth? In other words, if a user's
> repo is not found, why even prompting for a username? The latter, that
> is, the user's repo, is redundant, when the prompt is clearly asking
> for a username, and not a repo.
Huh? GitHub cannot tell you whether you have access to a repository (or,
for privacy reasons of the owner of the hypothetical repository, whether
a repository of that name might exist) unless you authenticate. So it
returns an HTTP 401. Your authentication in this case requires a
username and password. Git asks for the username first, then the
password.
Is there something else you think GitHub should be returning there? Or
something else you think Git should do with an HTTP 401?
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
From: Jeff King @ 2023-01-15 23:22 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, git
In-Reply-To: <8f175d26-3d84-3019-031d-e358390f2de4@ramsayjones.plus.com>
On Sun, Jan 15, 2023 at 09:37:07PM +0000, Ramsay Jones wrote:
> > +/**
> > + * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,
> > + * released in August 2022.
> > + */
> > +#if LIBCURL_VERSION_NUM >= 0x075500
> > +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1
> > +#endif
>
> Ah, I haven't really grokked what this file is about ... but this
> looks simple enough. ;)
It's newish from the cleanups in e4ff3b67c2 (http: centralize the
accounting of libcurl dependencies, 2021-09-13). I mostly just
cargo-culted this part. ;)
> > +#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> > + {
> > + struct strbuf buf = STRBUF_INIT;
> > +
> > + get_curl_allowed_protocols(0, &buf);
> > + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);
> > + strbuf_reset(&buf);
> > +
> > + get_curl_allowed_protocols(-1, &buf);
> > + curl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);
> > + strbuf_release(&buf);
>
> I used two static char arrays to accumulate the strings before
> passing them to curl. I was unsure of the lifetime/ownership
> semantics - I still haven't got around to looking them up!
Really old versions of curl had lifetime issues, but for a long now
(since before the oldest version we'd support), the rule is generally
that curl will copy any opt strings as necessary.
The allocations do feel heavyweight for setting an option. And I think
this get_curl_handle() is really called once per request, so we _could_
probably just generate them once and cache the result. But in general
I've been trying to avoid hidden static variables, etc, as they make
later libification efforts harder. And an extra malloc() on top of an
HTTP request is probably not noticeable.
> > +#else
> > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> > - get_curl_allowed_protocols(0));
> > + get_curl_allowed_protocols(0, NULL));
> > curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> > - get_curl_allowed_protocols(-1));
> > + get_curl_allowed_protocols(-1, NULL));
> > +#endif
> > +
> > if (getenv("GIT_CURL_VERBOSE"))
> > http_trace_curl_no_data();
> > setup_curl_trace(result);
>
> (another reason for not completing these patches - I don't
> know what the test coverage is like for these changes; are
> more tests required? dunno).
I had wondered that, too. ;) It's covered by t5812 (my quick and dirty
check was to just drop these lines and see what broke in the test
suite).
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
From: Jeff King @ 2023-01-15 23:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
In-Reply-To: <xmqqilh7fp4s.fsf@gitster.g>
On Sun, Jan 15, 2023 at 01:45:39PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The IOCTLFUNCTION option has been deprecated, and generates a compiler
> > warning in recent versions of curl. We can switch to using SEEKFUNCTION
> > instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
> > indicates we require at least curl 7.19.4.
> > ...
> > + if (offset < 0 || offset >= buffer->buf.len) {
> > + error("curl seek would be outside of buffer");
> > + return CURL_SEEKFUNC_FAIL;
> > }
> > +
> > + buffer->posn = offset;
> > + return CURL_SEEKFUNC_OK;
> > }
>
> Now we depend on having at least cURL 7.19.5 because
> CURL_SEEKFUNC_{FAIL,OK} are not available before that version.
>
> cf. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions#L127
Ugh. I did not even check that, because why would anyone introduce the
return values in a separate version! ;)
> Which is fine, as 7.19.5 is from May 2009 that is old enough. We
> just need to update the place where you got the 7.19.4 above from.
OK, if we are all right with bumping the version, I can squash that in.
The other option is to fall back to the obvious 0/1 for OK/FAIL, which
is what curl does. But that feels awfully hacky, and it would only be
used if you were at _exactly_ 7.19.4. So I prefer the bump if it's
acceptable.
diff --git a/INSTALL b/INSTALL
index 3344788397..d5694f8c47 100644
--- a/INSTALL
+++ b/INSTALL
@@ -139,7 +139,7 @@ Issues of note:
not need that functionality, use NO_CURL to build without
it.
- Git requires version "7.19.4" or later of "libcurl" to build
+ Git requires version "7.19.5" or later of "libcurl" to build
without NO_CURL. This version requirement may be bumped in
the future.
-Peff
^ permalink raw reply related
* Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
From: Jeff King @ 2023-01-15 23:13 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, git
In-Reply-To: <c77892ba-e5cd-1192-ceba-d49edcb95da8@ramsayjones.plus.com>
On Sun, Jan 15, 2023 at 08:54:59PM +0000, Ramsay Jones wrote:
> > - curl_easy_setopt(curl, CURLOPT_PUT, 1);
> > + curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
>
> My version of this patch had '1L' rather than just '1' - but it
> doesn't really matter (and was probably because all the curl
> examples did so!).
Yeah, I think it probably ought to be 1L because it's going to a
variadic function (and it's been a while, but I think the regular
integer promotions make small things into int, but not into long).
So I don't mind fixing it, but I think we'd want it in a separate patch,
since it's orthogonal to what's happening here.
-Peff
^ permalink raw reply
* Re: ctrl-z ignored by git; creates blobs from non-existent repos
From: Crls @ 2023-01-15 22:47 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: git
In-Reply-To: <Y8Qfj32h89hq5UD6@mit.edu>
On Sun, Jan 15, 2023 at 10:45:19AM -0500, Theodore Ts'o wrote:
> On Fri, Jan 13, 2023 at 05:01:01PM -0500, Crls wrote:
> > Ctrl-Z is ignored by git; Git-clone injects blobs even with non-existent
> > repos
> >
> > Steps to reproduce 1- git clone github whateverrepo/whatevernonexistentrepo
> > or 1- git clone gitlab whateverrepo/whatevernonexistentrepo 2= Git prompts
> > for a username
>
> % git clone github whateverrepo/whatevernonexistentrepo
> fatal: repository 'github' does not exist
>
> I think what you meant was:
>
> % git clone https://github.com/whateverrepo/whatevernonexistentrepo
> Cloning into 'whatevernonexistentrepo'...
> Username for 'https://github.com':
>
Yes. That's what I meant… thank you. I was having a problem with the formatting while sending the message to the mailing list
Content-Policy reject msg: The message contains HTML subpart, therefore we consider it SPAM or Outlook Virus. TEXT/PLAIN is accepted.! BF:; S229631AbjAMVJQ (in reply to end of DATA command)
> > 3- Press Ctrl-Z to stop *git* from running either on the virtual console/tty
> > *git* automatically creates blobs with directories and disregards
>
> So it's not that Control-Z is being ignored. It's that by the time
> you see the prompt for "Username for 'https://github.com': ", the
> directories already exist. Try looking at
> whatevernonexistentrepo/.git as soon as the prompt shows up. You'll
> see that the .git directory has been greated.
>
> Now, when you type ^Z, the git processes are stopped --- but the
> objects are created already.
The directories exist not because ^Z is used, but because by the time git-clone prompts for a username, git is already set on what to do next. Correct? in other words, the process is shoved down my throat. Or the user's throat in this case. Or going by another analogy, it certainly sounds as if I meant: «Git, please git-clone such and such repo, but let me fix just a typo on the repo name before submitting it, pretty please» and then Git replies: «too late for that chick-a-doodle» and there it goes. It injects blobs all over (well, not all over but on the dir specified).
>
> Username for 'https://github.com': ^Z
> [1]+ Stopped git clone https://github.com/whateverrepo/whatevernonexistentrepo
> % ps aux | grep git
> tytso 5097 0.0 0.0 9736 4480 pts/0 T 10:41 0:00 git clone https://github.com/wha
> tytso 5098 0.0 0.0 9736 3992 pts/0 T 10:41 0:00 /usr/lib/git-core/git remote-htt
> tytso 5099 0.0 0.1 102332 16104 pts/0 T 10:41 0:00 /usr/lib/git-core/git-remote-htt
> tytso 5140 0.0 0.0 6332 2072 pts/0 S+ 10:43 0:00 grep git
>
>
> The 'T' means that the processes are stopped.
>
> > Expected: The same issue does not happen with other non-existent repos e.g.,
> > git clone git.zx2c4/ it returns the message of fatal repo not found
>
> So what's going on is that github.com is not returning a non-existent
> repo error; it's prompting for a username/password, as _if_ the
> repository exists. That's presumably to prevent disclosing
> information as to whether or not a private repository exists or not.
I agree with you there. Coincidentally speaking, why does a username warrants a prompt from git, is simply beyond me. I mean, that is certainly the more far-fetched reasoning of implementation I've read in a long long time.
Can you git-clone a user? What about the user's settings? What about the remainder its gpg tokens and so forth? In other words, if a user's repo is not found, why even prompting for a username? The latter, that is, the user's repo, is redundant, when the prompt is clearly asking for a username, and not a repo.
>
> Once the authentication fails, git will remove the partially created
> repro, so it's really not a problem in practice.
>
> Cheers,
>
> - Ted
Not true. Preventing the disclosure of information has nothing to do with the issue here. If anything seems clear to me, is that prompting for a username, does indeed disclose usernames, private, public and whatnot from either github or gitlab.
And Technically speaking, yes, I agree with you in that if ^Z main operation is to suspend the process, there's not much further ado. Right?
p.s
I consulted this issue with Thomas Dickey, before sending an email through this interface, and he acknowledges that the operation from git occurs before. Thus confirming some of your statements, and also… that by the time git exits and it's done with, in whatever dir of my choosing, ^Z couldn't do anything else about it. But I beg to differ. While ^Z may not do anything about it, then git should instead. Or so I think.
running stty -a returns:
swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W;
take care Ted
Carlos
--
Seems a computer engineer, a systems analyst, and a programmer were
driving down a mountain when the brakes gave out. They screamed down the
mountain, gaining speed, but finally managed to grind to a halt, more by
luck than anything else, just inches from a thousand foot drop to jagged
rocks. They all got out of the car:
The computer engineer said, "I think I can fix it."
The systems analyst said, "No, no, I think we should take it
into town and have a specialist look at it."
The programmer said, "OK, but first I think we should get back
in and see if it does it again."
^ permalink raw reply
* Re: [PATCH v4 5/5] notes.c: introduce "--separator" option
From: Eric Sunshine @ 2023-01-15 22:15 UTC (permalink / raw)
To: Teng Long; +Cc: avarab, git, tenglong.tl
In-Reply-To: <f7edbd0e508243ab55c13721a21b78bf50278a21.1673490953.git.dyroneteng@gmail.com>
On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:
> When appending to a given notes object and the appended note is not
> empty too, we will insert a blank line at first which separates the
> existing note and the appended one, which as the separator.
> [...]
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -24,6 +24,8 @@
> +static char *separator = "\n";
If you are initializing `separator` to "\n"...
> @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> +static void insert_separator(struct strbuf *message)
> +{
> + if (!separator)
> + separator = "\n";
... then these lines are not needed (and are effectively dead-code).
> + if (*separator == '\0')
> + /* separator is empty; use as-is (no blank line) */
> + return;
> + else if (separator[strlen(separator) - 1] == '\n')
> + /* user supplied newline; use as-is */
> + insert = separator;
> + else
> + /* separator lacks newline; add it ourselves */
> + insert = xstrfmt("%s%s", separator,"\n");
> + strbuf_insertstr(message, 0, insert);
> +}
> git format-patch a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1 --thread -v 4 --output-directory=outgoing/git-notes-append/v4 --cover-letter --range-diff 196e80358ediff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
There's some weird malformation going on here... this should just be a
"diff --git ..." line.
> @@ -521,6 +521,65 @@ test_expect_success 'listing non-existing notes fails' '
> +test_expect_success 'append: specify an empty separator' '
> + test_when_finished git notes remove HEAD &&
> + cat >expect <<-\EOF &&
> + notes-1
> + notes-2
> + EOF
Style nit: We don't normally give extra indentation to the body of the
here-doc. Instead:
cat >expect <<-\EOF &&
notes-1
notes-2
EOF
> + git notes add -m "notes-1" &&
> + git notes append --separator="" -m "notes-2" &&
> + git notes show >actual &&
> + test_cmp expect actual
> +
> +'
Style nit: drop the unnecessary blank line before the closing quote.
> +test_expect_success 'append: specify separatoro with line break' '
s/separatoro/separator/
> + test_when_finished git notes remove HEAD &&
> + cat >expect <<-\EOF &&
> + notes-1
> + -------
> + notes-2
> + EOF
> +
> + git notes add -m "notes-1" &&
> + separator=$(printf "%s\n" "-------") &&
> + git notes append --separator="$separator" -m "notes-2" &&
It might be easier to drop the `separator` variable and write this simply as:
git notes append --separator="-------$LF" -m "notes-2" &&
LF is defined in t/test-lib.sh as "\n".
> + git notes show >actual &&
> + test_cmp expect actual
> +'
^ permalink raw reply
* Re: [PATCH v4 5/5] notes.c: introduce "--separator" option
From: Eric Sunshine @ 2023-01-15 22:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Teng Long, git, tenglong.tl
In-Reply-To: <230112.86y1q812y4.gmgdl@evledraar.gmail.com>
On Thu, Jan 12, 2023 at 5:07 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Jan 12 2023, Teng Long wrote:
> > When appending to a given notes object and the appended note is not
> > empty too, we will insert a blank line at first which separates the
> > existing note and the appended one, which as the separator.
> > [...]
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> > @@ -159,6 +163,16 @@ OPTIONS
> > +--separator <text>::
> > + Specify the <text> to be inserted between existing note and appended
> > + message, the <text> acts as a separator.
>
> Maybe let's use '<string>' or '<separator>' here instead? e.g.:
> Specifies the <string> ...
> Maybe "<text>" just looks odd to me.
>
> More generally, let's say something like:
> When invoking "git notes append", specify the...
> I.e. this is only for "append", but nothing here says so.
Agreed on these points.
> > + If <text> is empty (`--separator=''`), will append the message to
> > + existing note directly without insert any separator.
> > + If <text> is nonempty, will use as-is. One thing to notice is if
> > + the <text> lacks newline charactor, will add the newline automatically.
> > + If not specify this option, a blank line will be inserted as the
> > + separator.
>
> We're spending a lot of text here on a pretty simple concept if I
> understand it correctly, I.e. just (pseudocode):
>
> int sep_extra_nl = 0;
> const char *sep = opt_sep ? opt_sep : "\n";
> if (!strstr(sep, '\n'))
> sep_extra_nl = 1;
> [...]
>
> Except that was written after I read your explanation, but looking at
> the code it's incorrect, it's whether the "*last*" character contains a
> newline or not.
>
> So all in all, I think we should just say:
>
> --separator <separator>:
> The '<separator>' inserted between the note and message
> by 'append', "\n" by default. A custom separator can be
> provided, if it doesn't end in a "\n" one will be added
> implicitly .
Unfortunately, this misses the point. The original reason Teng Long
started on this patch series was to be able to _suppress_ the blank
line added unconditionally between notes. In the original submission,
this was done via a --no-blankline option, but that met with
resistance from some reviewers as being potentially confusing and too
specialized. (The commit message of this patch should probably do a
better job of explaining that one purpose of this change is to support
the case of no-separator.)
A generalized --separator= option was suggested[1] as a possibly more
palatable alternative, with which an empty string (meaning "no
separator") would cover the case for which the original --no-blankline
was meant to handle. So, at the very least, the documentation needs to
call out the empty string as being a special case for which automatic
appending of "\n" does not occur.
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > +static void insert_separator(struct strbuf *message)
> > +{
> > + const char *insert;
> > +
> > + if (!separator)
> > + separator = "\n";
> > + if (*separator == '\0')
>
> Style: Don't compare to 0, NULL, '\0' etc. Just use !*separator.
My fault[2]. Your suggestion is indeed more appropriate in this codebase.
> > + /* separator is empty; use as-is (no blank line) */
> > + return;
> > + else if (separator[strlen(separator) - 1] == '\n')
> > + /* user supplied newline; use as-is */
> > + insert = separator;
> > + else
> > + /* separator lacks newline; add it ourselves */
> > + insert = xstrfmt("%s%s", separator,"\n");
>
> We're leaking memor here, and making it hard to fix that by conflating a
> const "insert" with this allocated version.
>
> I haven't read the whole context, but this seems really complex per the
> doc feedback above. Why can't we just keep track of if we're using the
> default value or not? I.e. just have the "--separator" option default to
> NULL, if it's not set y ou don't need to do this "\n" check, and just
> use the default, otherwise append etc.
That wouldn't work for the reason given above. The idea outlined in
[2] is that an empty separator is treated specially as meaning
"nothing-between-notes, not even a blank line".
> > + strbuf_insertstr(message, 0, insert);
>
> Maybe you were trying to get around using a more complex strbuf_splice()
> here, but let's just avoid teh xstrfmt() and splice() that "\n" in, if
> needed?
The code example I gave in [2] was meant to illustrate the suggested
behavior as clearly as possible, not necessarily to be copied
verbatim. Being able to do this without leaking memory should
certainly be possible.
> Do we mean to strbuf_stripspace() here over the whole buffer, or just
> what we're appending?
That's a very good question. The note being appended might indeed have
leading whitespace gunk which ought to be removed before the append
operation. (Plus, it's a reasonable assumption that the existing note
text has already been "stripspaced".)
[1]: https://lore.kernel.org/git/CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cTFBVAL2gd3LqQEzS--cXqJXR+1OVerii-D6JqFvJwXqQ@mail.gmail.com/
^ permalink raw reply
* Re: Segmentation fault within git read-tree
From: Junio C Hamano @ 2023-01-15 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: Christian Couder, Frédéric Fort, git
In-Reply-To: <Y8RHfjFFLRdW3WRh@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm not sure if Frédéric is seeing another segfault in practice (when
> not using --debug-unpack), but yeah, it is very easy to trigger this
> segfault. It does not even have to do with sparse checkouts, etc. Here's
> an even more minimal example:
>
> git init repo
> cd repo
> touch file
> git add file
> git commit -m added
> git read-tree --debug-unpack --prefix=subtree HEAD
>
> I was going to bisect, but it looks like it was broken all the way back
> to Junio's ba655da537 (read-tree --debug-unpack, 2009-09-14).
As "git read-tree --help" does not even metnion "--debug-unpack", I
have no idea what it does, or how useful the debug information it
produces is. As long as the same command without --debug-unpack
works OK, I'd throw this into "does it hurt? don't do it, then" bin.
;-)
^ permalink raw reply
* Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
From: Junio C Hamano @ 2023-01-15 21:45 UTC (permalink / raw)
To: Jeff King; +Cc: git, Ramsay Jones
In-Reply-To: <Y8Rd0KXYcHKykvjq@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The IOCTLFUNCTION option has been deprecated, and generates a compiler
> warning in recent versions of curl. We can switch to using SEEKFUNCTION
> instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
> indicates we require at least curl 7.19.4.
> ...
> + if (offset < 0 || offset >= buffer->buf.len) {
> + error("curl seek would be outside of buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
> +
> + buffer->posn = offset;
> + return CURL_SEEKFUNC_OK;
> }
Now we depend on having at least cURL 7.19.5 because
CURL_SEEKFUNC_{FAIL,OK} are not available before that version.
cf. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions#L127
Which is fine, as 7.19.5 is from May 2009 that is old enough. We
just need to update the place where you got the 7.19.4 above from.
Thanks.
^ permalink raw reply
* Re: [PATCH] ci: do not die on deprecated-declarations warning
From: Junio C Hamano @ 2023-01-15 21:38 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, git
In-Reply-To: <Y8RdOt02JTvDnLiX@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I do think the IOCTL/SEEK one is old enough that we can do, though. The
> deprecation is newer, but the SEEK interface was added in an old enough
> version.
Yes, that one is old enough (SEEKFUNCTION and frieds are from 7.18.0
and 7.19.5, IOCTL was deprecated at 7.18.0, released 15 years ago).
A problematic one is REDIR_PROTOCOLS that was deprecated in 7.85.0
(Aug 2022) whose replacement appeared in the same 7.85.0 version.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
From: Ramsay Jones @ 2023-01-15 21:37 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <Y8ReHbGWetJHQcI1@coredump.intra.peff.net>
On 15/01/2023 20:12, Jeff King wrote:
> The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was
> deprecated in curl 7.85.0, and using it generate compiler warnings as of
> curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we
> can't just do so unilaterally, as it was only introduced less than a
> year ago in 7.85.0.
Yep, I hadn't quite finished my version of this patch yet, but you
would probably not be shocked to learn that I had two separate sets
of functions #ifdef-ed by curl version number! What you have here
looks *much* better!
>
> Until that version becomes ubiquitous, we have to either disable the
> deprecation warning or conditionally use the "STR" variant on newer
> versions of libcurl. This patch switches to the new variant, which is
> nice for two reasons:
>
> - we don't have to worry that silencing curl's deprecation warnings
> might cause us to miss other more useful ones
>
> - we'd eventually want to move to the new variant anyway, so this gets
> us set up (albeit with some extra ugly boilerplate for the
> conditional)
>
> There are a lot of ways to split up the two cases. One way would be to
> abstract the storage type (strbuf versus a long), how to append
> (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,
> and so on. But the resulting code looks pretty magical:
>
> GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;
> if (...http is allowed...)
> GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP);
>
> and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than
> actual code.
>
> On the other end of the spectrum, we could just implement two separate
> functions, one that handles a string list and one that handles bits. But
> then we end up repeating our list of protocols (http, https, ftp, ftp).
>
> This patch takes the middle ground. The run-time code is always there to
> handle both types, and we just choose which one to feed to curl.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> git-curl-compat.h | 8 ++++++++
> http.c | 41 ++++++++++++++++++++++++++++++++++-------
> 2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/git-curl-compat.h b/git-curl-compat.h
> index 56a83b6bbd..fd96b3cdff 100644
> --- a/git-curl-compat.h
> +++ b/git-curl-compat.h
> @@ -126,4 +126,12 @@
> #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
> #endif
>
> +/**
> + * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,
> + * released in August 2022.
> + */
> +#if LIBCURL_VERSION_NUM >= 0x075500
> +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1
> +#endif
Ah, I haven't really grokked what this file is about ... but this
looks simple enough. ;)
> +
> #endif
> diff --git a/http.c b/http.c
> index ca0fe80ddb..e529ebc993 100644
> --- a/http.c
> +++ b/http.c
> @@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)
> curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
> }
>
> -static long get_curl_allowed_protocols(int from_user)
> +static void proto_list_append(struct strbuf *list_str, const char *proto_str,
> + long *list_bits, long proto_bits)
> +{
> + *list_bits |= proto_bits;
> + if (list_str) {
> + if (list_str->len)
> + strbuf_addch(list_str, ',');
> + strbuf_addstr(list_str, proto_str);
> + }
> +}
> +
> +static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
> {
> long allowed_protocols = 0;
>
> if (is_transport_allowed("http", from_user))
> - allowed_protocols |= CURLPROTO_HTTP;
> + proto_list_append(list, "http", &allowed_protocols, CURLPROTO_HTTP);
> if (is_transport_allowed("https", from_user))
> - allowed_protocols |= CURLPROTO_HTTPS;
> + proto_list_append(list, "https", &allowed_protocols, CURLPROTO_HTTPS);
> if (is_transport_allowed("ftp", from_user))
> - allowed_protocols |= CURLPROTO_FTP;
> + proto_list_append(list, "ftp", &allowed_protocols, CURLPROTO_FTP);
> if (is_transport_allowed("ftps", from_user))
> - allowed_protocols |= CURLPROTO_FTPS;
> + proto_list_append(list, "ftps", &allowed_protocols, CURLPROTO_FTPS);
>
> return allowed_protocols;
> }
> @@ -921,10 +932,26 @@ static CURL *get_curl_handle(void)
>
> curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
> curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
> +
> +#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> + {
> + struct strbuf buf = STRBUF_INIT;
> +
> + get_curl_allowed_protocols(0, &buf);
> + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);
> + strbuf_reset(&buf);
> +
> + get_curl_allowed_protocols(-1, &buf);
> + curl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);
> + strbuf_release(&buf);
I used two static char arrays to accumulate the strings before
passing them to curl. I was unsure of the lifetime/ownership
semantics - I still haven't got around to looking them up!
> + }
> +#else
> curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> - get_curl_allowed_protocols(0));
> + get_curl_allowed_protocols(0, NULL));
> curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> - get_curl_allowed_protocols(-1));
> + get_curl_allowed_protocols(-1, NULL));
> +#endif
> +
> if (getenv("GIT_CURL_VERBOSE"))
> http_trace_curl_no_data();
> setup_curl_trace(result);
(another reason for not completing these patches - I don't
know what the test coverage is like for these changes; are
more tests required? dunno).
For what it's worth, this LGTM.
Thanks!
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v4 4/5] notes.c: provide tips when target and append note are both empty
From: Eric Sunshine @ 2023-01-15 21:28 UTC (permalink / raw)
To: Teng Long; +Cc: avarab, git, tenglong.tl
In-Reply-To: <d41ba140505aa3459330afd652ff6a0f456222a0.1673490953.git.dyroneteng@gmail.com>
On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:
> When "git notes append <object>" is executed, if there is no note in
> the given object and the appended note is empty too, we could print
> the exact tips to end-user.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> + } else if (!d.buf.len && !note)
> + fprintf(stderr,
> + _("Both original and appended notes are empty in %s, do nothing\n"),
> + oid_to_hex(&object));
My knee-jerk reaction is between "meh" and "thumbs down". The commit
message says we "can do this" but doesn't explain "why we should do
this". Is this condition important enough to break the Unix maxim of
"Rule of Silence" (or "Silence is Golden")?
I also wonder if this change is going to cause problems (or at least
annoyance) for automated tooling. At present, tooling doesn't have to
worry whether or not the existing or new note is empty; everything
just works as expected without complaint. However, following this
change, such tooling may now be greeted with an unexpected diagnostic
message.
^ permalink raw reply
* Re: [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
From: Ramsay Jones @ 2023-01-15 21:11 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <Y8Rd0KXYcHKykvjq@coredump.intra.peff.net>
On 15/01/2023 20:10, Jeff King wrote:
> The IOCTLFUNCTION option has been deprecated, and generates a compiler
> warning in recent versions of curl. We can switch to using SEEKFUNCTION
> instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
> indicates we require at least curl 7.19.4.
Ah, I didn't even think about this! I knew they were implemented in 7.18.0
and (as I mentioned previously) used the curl version number to switch
between the two sets of 'options'/implementations. Duh! :(
This is much better!
> We have to rewrite the ioctl functions into seek functions. In some ways
> they are simpler (seeking is the only operation), but in some ways more
> complex (the ioctl allowed only a full rewind, but now we can seek to
> arbitrary offsets).
>
> Curl will only ever use SEEK_SET (per their documentation), so I didn't
> bother implementing anything else, since it would naturally be
> completely untested. This seems unlikely to change, but I added an
> assertion just in case.
>
> Likewise, I doubt curl will ever try to seek outside of the buffer sizes
> we've told it, but I erred on the defensive side here, rather than do an
> out-of-bounds read.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> http-push.c | 4 ++--
> http.c | 20 +++++++++-----------
> http.h | 2 +-
> remote-curl.c | 28 +++++++++++++---------------
> 4 files changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index 1b18e775d0..7f71316456 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,
> curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
> curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
> curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
> - curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
> - curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
> + curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);
> + curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);
> curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
> curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
> curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
> diff --git a/http.c b/http.c
> index 8a5ba3f477..ca0fe80ddb 100644
> --- a/http.c
> +++ b/http.c
> @@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> return size / eltsize;
> }
>
> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
> +int seek_buffer(void *clientp, curl_off_t offset, int origin)
> {
> struct buffer *buffer = clientp;
>
> - switch (cmd) {
> - case CURLIOCMD_NOP:
> - return CURLIOE_OK;
> -
> - case CURLIOCMD_RESTARTREAD:
> - buffer->posn = 0;
> - return CURLIOE_OK;
> -
> - default:
> - return CURLIOE_UNKNOWNCMD;
> + if (origin != SEEK_SET)
> + BUG("seek_buffer only handles SEEK_SET");
I didn't even think to check this; as you say, the documentation
claims only to send SEEK_SET, so ... (but this is obviously a
good idea).
> + if (offset < 0 || offset >= buffer->buf.len) {
> + error("curl seek would be outside of buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
I did at least do this! :)
> +
> + buffer->posn = offset;
> + return CURL_SEEKFUNC_OK;
> }
>
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> diff --git a/http.h b/http.h
> index 3c94c47910..77c042706c 100644
> --- a/http.h
> +++ b/http.h
> @@ -40,7 +40,7 @@ struct buffer {
> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
> +int seek_buffer(void *clientp, curl_off_t offset, int origin);
>
> /* Slot lifecycle functions */
> struct active_request_slot *get_active_slot(void);
> diff --git a/remote-curl.c b/remote-curl.c
> index 72dfb8fb86..a76b6405eb 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,
> return avail;
> }
>
> -static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
> +static int rpc_seek(void *clientp, curl_off_t offset, int origin)
> {
> struct rpc_state *rpc = clientp;
>
> - switch (cmd) {
> - case CURLIOCMD_NOP:
> - return CURLIOE_OK;
> + if (origin != SEEK_SET)
> + BUG("rpc_seek only handles SEEK_SET, not %d", origin);
>
> - case CURLIOCMD_RESTARTREAD:
> - if (rpc->initial_buffer) {
> - rpc->pos = 0;
> - return CURLIOE_OK;
> + if (rpc->initial_buffer) {
> + if (offset < 0 || offset > rpc->len) {
> + error("curl seek would be outside of rpc buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
> - error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
> - return CURLIOE_FAILRESTART;
> -
> - default:
> - return CURLIOE_UNKNOWNCMD;
> + rpc->pos = offset;
> + return CURL_SEEKFUNC_OK;
> }
> + error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
> + return CURL_SEEKFUNC_FAIL;
> }
>
> struct check_pktline_state {
> @@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
> rpc->initial_buffer = 1;
> curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
> curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
> - curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
> - curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
> + curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
> + curl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);
> if (options.verbosity > 1) {
> fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
> fflush(stderr);
It looks so much better without #ifdef's (or having to worry about
the git-curl-compat.h header file)!
LGTM
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'
From: Eric Sunshine @ 2023-01-15 21:10 UTC (permalink / raw)
To: Teng Long; +Cc: avarab, git, tenglong.tl
In-Reply-To: <CAPig+cR5s3XzmY+L_jDW2g_PEgi5E791x0GuV+VPkxFA_6sB7A@mail.gmail.com>
On Sun, Jan 15, 2023 at 3:59 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:
> > - } else {
> > - fprintf(stderr, _("Removing note for object %s\n"),
> > - oid_to_hex(&object));
> > - remove_note(t, object.hash);
> > - logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> > + commit_notes(the_repository, t, logmsg);
> > }
> > - commit_notes(the_repository, t, logmsg);
>
> This change breaks removal of notes using "git notes edit". Prior to
> this change, if you delete the content of a note using "git notes
> edit", then the note is removed. Following this change, the note
> remains, which contradicts documented[1] behavior.
Aside from suggesting that this patch ought to be dropped, implicit in
the above comment is that test coverage is not as thorough as it
should be, otherwise this regression would have been caught early.
This suggests that the test coverage, at least for "git notes edit",
needs some beefing up, though doing so is probably outside the scope
of this series; it's something which could/should be done separately,
and it doesn't necessarily need to be done by you.
^ permalink raw reply
* Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'
From: Eric Sunshine @ 2023-01-15 20:59 UTC (permalink / raw)
To: Teng Long; +Cc: avarab, git, tenglong.tl
In-Reply-To: <7b756b4c605e148f6938fee74882091661382173.1673490953.git.dyroneteng@gmail.com>
On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:
> Situation of note "removing" shouldn't happen in 'append_edit()',
> unless it's a bug. So, let's drop the unreachable "else" code
> in "append_edit()".
>
> The notes operation "append" is different with "add", the latter
> supports to overwrite the existing note then let the "removing"
> happen (e.g. execute `git notes add -f -F /dev/null` on an existing
> note), but the former will not because it only does "appends" but
> not doing "overwrites".
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> if (add_note(t, &object, &new_note, combine_notes_overwrite))
> BUG("combine_notes_overwrite failed");
> logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
> - } else {
> - fprintf(stderr, _("Removing note for object %s\n"),
> - oid_to_hex(&object));
> - remove_note(t, object.hash);
> - logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> + commit_notes(the_repository, t, logmsg);
> }
> - commit_notes(the_repository, t, logmsg);
This change breaks removal of notes using "git notes edit". Prior to
this change, if you delete the content of a note using "git notes
edit", then the note is removed. Following this change, the note
remains, which contradicts documented[1] behavior.
[1]: Unfortunately, perhaps, this behavior is documented under the
"remove" subcommand rather than the "edit" subcommand, but it is
nevertheless documented and has worked this way for ages, so this
patch causes a regression.
^ permalink raw reply
* Re: [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
From: Ramsay Jones @ 2023-01-15 20:54 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <Y8RduNqadAfaOgs1@coredump.intra.peff.net>
On 15/01/2023 20:10, Jeff King wrote:
> The two options do exactly the same thing, but the latter has been
> deprecated and in recent versions of curl may produce a compiler
> warning. Since the UPLOAD form is available everywhere (it was
> introduced in the year 2000 by curl 7.1), we can just switch to it.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> http-push.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/http-push.c b/http-push.c
> index 5f4340a36e..1b18e775d0 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,
> const char *custom_req, struct buffer *buffer,
> curl_write_callback write_fn)
> {
> - curl_easy_setopt(curl, CURLOPT_PUT, 1);
> + curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
My version of this patch had '1L' rather than just '1' - but it
doesn't really matter (and was probably because all the curl
examples did so!).
LGTM
ATB,
Ramsay Jones
> curl_easy_setopt(curl, CURLOPT_URL, url);
> curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
> curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
^ permalink raw reply
* [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR
From: Jeff King @ 2023-01-15 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
In-Reply-To: <Y8RddcM9Vr71ljp4@coredump.intra.peff.net>
The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was
deprecated in curl 7.85.0, and using it generate compiler warnings as of
curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we
can't just do so unilaterally, as it was only introduced less than a
year ago in 7.85.0.
Until that version becomes ubiquitous, we have to either disable the
deprecation warning or conditionally use the "STR" variant on newer
versions of libcurl. This patch switches to the new variant, which is
nice for two reasons:
- we don't have to worry that silencing curl's deprecation warnings
might cause us to miss other more useful ones
- we'd eventually want to move to the new variant anyway, so this gets
us set up (albeit with some extra ugly boilerplate for the
conditional)
There are a lot of ways to split up the two cases. One way would be to
abstract the storage type (strbuf versus a long), how to append
(strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use,
and so on. But the resulting code looks pretty magical:
GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT;
if (...http is allowed...)
GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP);
and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than
actual code.
On the other end of the spectrum, we could just implement two separate
functions, one that handles a string list and one that handles bits. But
then we end up repeating our list of protocols (http, https, ftp, ftp).
This patch takes the middle ground. The run-time code is always there to
handle both types, and we just choose which one to feed to curl.
Signed-off-by: Jeff King <peff@peff.net>
---
git-curl-compat.h | 8 ++++++++
http.c | 41 ++++++++++++++++++++++++++++++++++-------
2 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/git-curl-compat.h b/git-curl-compat.h
index 56a83b6bbd..fd96b3cdff 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -126,4 +126,12 @@
#define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
#endif
+/**
+ * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,
+ * released in August 2022.
+ */
+#if LIBCURL_VERSION_NUM >= 0x075500
+#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1
+#endif
+
#endif
diff --git a/http.c b/http.c
index ca0fe80ddb..e529ebc993 100644
--- a/http.c
+++ b/http.c
@@ -764,18 +764,29 @@ void setup_curl_trace(CURL *handle)
curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
}
-static long get_curl_allowed_protocols(int from_user)
+static void proto_list_append(struct strbuf *list_str, const char *proto_str,
+ long *list_bits, long proto_bits)
+{
+ *list_bits |= proto_bits;
+ if (list_str) {
+ if (list_str->len)
+ strbuf_addch(list_str, ',');
+ strbuf_addstr(list_str, proto_str);
+ }
+}
+
+static long get_curl_allowed_protocols(int from_user, struct strbuf *list)
{
long allowed_protocols = 0;
if (is_transport_allowed("http", from_user))
- allowed_protocols |= CURLPROTO_HTTP;
+ proto_list_append(list, "http", &allowed_protocols, CURLPROTO_HTTP);
if (is_transport_allowed("https", from_user))
- allowed_protocols |= CURLPROTO_HTTPS;
+ proto_list_append(list, "https", &allowed_protocols, CURLPROTO_HTTPS);
if (is_transport_allowed("ftp", from_user))
- allowed_protocols |= CURLPROTO_FTP;
+ proto_list_append(list, "ftp", &allowed_protocols, CURLPROTO_FTP);
if (is_transport_allowed("ftps", from_user))
- allowed_protocols |= CURLPROTO_FTPS;
+ proto_list_append(list, "ftps", &allowed_protocols, CURLPROTO_FTPS);
return allowed_protocols;
}
@@ -921,10 +932,26 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
+
+#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
+ {
+ struct strbuf buf = STRBUF_INIT;
+
+ get_curl_allowed_protocols(0, &buf);
+ curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS_STR, buf.buf);
+ strbuf_reset(&buf);
+
+ get_curl_allowed_protocols(-1, &buf);
+ curl_easy_setopt(result, CURLOPT_PROTOCOLS_STR, buf.buf);
+ strbuf_release(&buf);
+ }
+#else
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
- get_curl_allowed_protocols(0));
+ get_curl_allowed_protocols(0, NULL));
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
- get_curl_allowed_protocols(-1));
+ get_curl_allowed_protocols(-1, NULL));
+#endif
+
if (getenv("GIT_CURL_VERBOSE"))
http_trace_curl_no_data();
setup_curl_trace(result);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related
* [PATCH 2/3] http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
From: Jeff King @ 2023-01-15 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
In-Reply-To: <Y8RddcM9Vr71ljp4@coredump.intra.peff.net>
The IOCTLFUNCTION option has been deprecated, and generates a compiler
warning in recent versions of curl. We can switch to using SEEKFUNCTION
instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
indicates we require at least curl 7.19.4.
We have to rewrite the ioctl functions into seek functions. In some ways
they are simpler (seeking is the only operation), but in some ways more
complex (the ioctl allowed only a full rewind, but now we can seek to
arbitrary offsets).
Curl will only ever use SEEK_SET (per their documentation), so I didn't
bother implementing anything else, since it would naturally be
completely untested. This seems unlikely to change, but I added an
assertion just in case.
Likewise, I doubt curl will ever try to seek outside of the buffer sizes
we've told it, but I erred on the defensive side here, rather than do an
out-of-bounds read.
Signed-off-by: Jeff King <peff@peff.net>
---
http-push.c | 4 ++--
http.c | 20 +++++++++-----------
http.h | 2 +-
remote-curl.c | 28 +++++++++++++---------------
4 files changed, 25 insertions(+), 29 deletions(-)
diff --git a/http-push.c b/http-push.c
index 1b18e775d0..7f71316456 100644
--- a/http-push.c
+++ b/http-push.c
@@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
- curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
- curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
+ curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);
+ curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
diff --git a/http.c b/http.c
index 8a5ba3f477..ca0fe80ddb 100644
--- a/http.c
+++ b/http.c
@@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
return size / eltsize;
}
-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
+int seek_buffer(void *clientp, curl_off_t offset, int origin)
{
struct buffer *buffer = clientp;
- switch (cmd) {
- case CURLIOCMD_NOP:
- return CURLIOE_OK;
-
- case CURLIOCMD_RESTARTREAD:
- buffer->posn = 0;
- return CURLIOE_OK;
-
- default:
- return CURLIOE_UNKNOWNCMD;
+ if (origin != SEEK_SET)
+ BUG("seek_buffer only handles SEEK_SET");
+ if (offset < 0 || offset >= buffer->buf.len) {
+ error("curl seek would be outside of buffer");
+ return CURL_SEEKFUNC_FAIL;
}
+
+ buffer->posn = offset;
+ return CURL_SEEKFUNC_OK;
}
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
diff --git a/http.h b/http.h
index 3c94c47910..77c042706c 100644
--- a/http.h
+++ b/http.h
@@ -40,7 +40,7 @@ struct buffer {
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
-curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
+int seek_buffer(void *clientp, curl_off_t offset, int origin);
/* Slot lifecycle functions */
struct active_request_slot *get_active_slot(void);
diff --git a/remote-curl.c b/remote-curl.c
index 72dfb8fb86..a76b6405eb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,
return avail;
}
-static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
+static int rpc_seek(void *clientp, curl_off_t offset, int origin)
{
struct rpc_state *rpc = clientp;
- switch (cmd) {
- case CURLIOCMD_NOP:
- return CURLIOE_OK;
+ if (origin != SEEK_SET)
+ BUG("rpc_seek only handles SEEK_SET, not %d", origin);
- case CURLIOCMD_RESTARTREAD:
- if (rpc->initial_buffer) {
- rpc->pos = 0;
- return CURLIOE_OK;
+ if (rpc->initial_buffer) {
+ if (offset < 0 || offset > rpc->len) {
+ error("curl seek would be outside of rpc buffer");
+ return CURL_SEEKFUNC_FAIL;
}
- error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
- return CURLIOE_FAILRESTART;
-
- default:
- return CURLIOE_UNKNOWNCMD;
+ rpc->pos = offset;
+ return CURL_SEEKFUNC_OK;
}
+ error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
+ return CURL_SEEKFUNC_FAIL;
}
struct check_pktline_state {
@@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
rpc->initial_buffer = 1;
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
- curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
- curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
+ curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
+ curl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);
if (options.verbosity > 1) {
fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
fflush(stderr);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related
* [PATCH 1/3] http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
From: Jeff King @ 2023-01-15 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
In-Reply-To: <Y8RddcM9Vr71ljp4@coredump.intra.peff.net>
The two options do exactly the same thing, but the latter has been
deprecated and in recent versions of curl may produce a compiler
warning. Since the UPLOAD form is available everywhere (it was
introduced in the year 2000 by curl 7.1), we can just switch to it.
Signed-off-by: Jeff King <peff@peff.net>
---
http-push.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/http-push.c b/http-push.c
index 5f4340a36e..1b18e775d0 100644
--- a/http-push.c
+++ b/http-push.c
@@ -198,7 +198,7 @@ static void curl_setup_http(CURL *curl, const char *url,
const char *custom_req, struct buffer *buffer,
curl_write_callback write_fn)
{
- curl_easy_setopt(curl, CURLOPT_PUT, 1);
+ curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
curl_easy_setopt(curl, CURLOPT_URL, url);
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
--
2.39.0.513.g00e40dbe01
^ permalink raw reply related
* Re: [PATCH] ci: do not die on deprecated-declarations warning
From: Jeff King @ 2023-01-15 20:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
In-Reply-To: <xmqqzgakgu0n.fsf@gitster.g>
On Sat, Jan 14, 2023 at 11:02:32PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it
> > goes.
>
> The "DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations" version
> was merged to 'next', only because I wanted to see the commit
> cleanly pass the tests (and it does), but I do think in the longer
> term (like, before the topic hits 'master'), it probably is better
> to do this for everybody, not just for those who use DEVELOPER=Yes.
>
> So, further patches on top are very much welcomed.
So I took a look at just dropping the deprecated bits, and it wasn't
_too_ bad. Here's that series. The first two I hope are obviously good.
The third one is _ugly_, but at least it punts on the whole "how should
we silence this" argument, and it takes us in the direction we'd
ultimately want to go.
[1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
[2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
[3/3]: http: support CURLOPT_PROTOCOLS_STR
git-curl-compat.h | 8 +++++++
http-push.c | 6 ++---
http.c | 61 +++++++++++++++++++++++++++++++++--------------
http.h | 2 +-
remote-curl.c | 28 ++++++++++------------
5 files changed, 68 insertions(+), 37 deletions(-)
^ 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