* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-15 2:40 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce
In-Reply-To: <20161115004426.unheihlmftlw6ex7@sigill.intra.peff.net>
On Mon, Nov 14, 2016 at 07:44:26PM -0500, Jeff King wrote:
> > But it does seem to work. At least it doesn't seem to break anything in
> > the test suite, and it fixes the new tests you added. I'd worry that
> > there's some obscure case where the response isn't packetized in the
> > same way.
>
> Actually, I take it back. I think it works for a single round of ref
> negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
> fail t5551.
>
> I think I've probably made a mis-assumption on exactly when in the HTTP
> protocol we will see a flush packet (and perhaps that is a sign that
> this protocol-snooping approach is not a good one).
Yep, that is it. The server may end with an ACK or a NAK, not a flush
packet. So going this route really does mean teaching remote-curl a lot
more about the protocol, which is pretty nasty. I think trying to
somehow signal the end-of-response to fetch-pack would be a more
maintainable approach.
-Peff
^ permalink raw reply
* Re: [PATCH v7 16/17] branch: use ref-filter printing APIs
From: Jacob Keller @ 2016-11-15 1:36 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <CAOLa=ZQSQe=jfTpdyscZCZgi5p6cVLN23oi2eE-hqFTXgt2LEQ@mail.gmail.com>
On Mon, Nov 14, 2016 at 11:23 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Hello
>
> On Wed, Nov 9, 2016 at 5:44 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Nov 8, 2016 at 12:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Port branch.c to use ref-filter APIs for printing. This clears out
>>> most of the code used in branch.c for printing and replaces them with
>>> calls made to the ref-filter library.
>>
>> Nice. This looks correct based on checking against the current
>> branch.c implementation by hand. There was one minor change I
>> suggested but I'm not really sure it buys is that much.
>>
>
> Thanks for this review. More down.
>
>>> + if (filter->verbose > 1)
>>> + strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
>>> + "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
>>> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
>>
>> When we have extra verbose, we check whether we have an upstream, and
>> if so, we print the short name of that upstream inside brackets. If we
>> have tracking information, we print that without brackets, and then we
>> end this section. Finally we print the subject.
>>
>> We could almost re-use the code for the subject bits, but I'm not sure
>> it's worth it. Maybe drop the %contents:subject part and add it
>> afterwards since we always want it? It would remove some duplication
>> but overall not sure it's actually worth it.
>>
>
> If you see that's the last part we add to the 'local' strbuf in the
> verbose case.
> If we want to remove the duplication we'll end up adding one more
> strbuf_addf(...).
> So I guess its better this way.
>
Agreed, I think that it makes more sense to keep this as is. It is
relatively complicated and the strings do have some duplicate code,
but I think it's still ok.
Thanks,
Jake
> --
> Regards,
> Karthik Nayak
^ permalink raw reply
* [PATCH 2/2] push: fix --dry-run to not push submodules
From: Brandon Williams @ 2016-11-15 1:18 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1479172735-698-1-git-send-email-bmwill@google.com>
Teach push to respect the --dry-run option when configured to
recursively push submodules 'on-demand'. This is done by passing the
--dry-run flag to the child process which performs a push for a
submodules when performing a dry-run.
In order to preserve good user experience, the additional check for
unpushed submodules is skipped during a dry-run when
--recurse-submodules=on-demand. The check is skipped because the submodule
pushes were performed as dry-runs and this check would always fail as the
submodules would still need to be pushed.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
submodule.c | 13 ++++++++-----
submodule.h | 4 +++-
t/t5531-deep-submodule-push.sh | 2 +-
transport.c | 9 ++++++---
4 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/submodule.c b/submodule.c
index a05c2a3..7b9bed1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -676,16 +676,17 @@ int find_unpushed_submodules(struct sha1_array *hashes,
return needs_pushing->nr;
}
-static int push_submodule(const char *path)
+static int push_submodule(const char *path, int dry_run)
{
if (add_submodule_odb(path))
return 1;
if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
struct child_process cp = CHILD_PROCESS_INIT;
- const char *argv[] = {"push", NULL};
+ argv_array_push(&cp.args, "push");
+ if (dry_run)
+ argv_array_push(&cp.args, "--dry-run");
- cp.argv = argv;
prepare_submodule_repo_env(&cp.env_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
@@ -698,7 +699,9 @@ static int push_submodule(const char *path)
return 1;
}
-int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name)
+int push_unpushed_submodules(struct sha1_array *hashes,
+ const char *remotes_name,
+ int dry_run)
{
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
@@ -709,7 +712,7 @@ int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name
for (i = 0; i < needs_pushing.nr; i++) {
const char *path = needs_pushing.items[i].string;
fprintf(stderr, "Pushing submodule '%s'\n", path);
- if (!push_submodule(path)) {
+ if (!push_submodule(path, dry_run)) {
fprintf(stderr, "Unable to push submodule '%s'\n", path);
ret = 0;
}
diff --git a/submodule.h b/submodule.h
index 065b2f0..a38e0f3 100644
--- a/submodule.h
+++ b/submodule.h
@@ -65,7 +65,9 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c
const unsigned char a[20], const unsigned char b[20], int search);
int find_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name,
struct string_list *needs_pushing);
-int push_unpushed_submodules(struct sha1_array *hashes, const char *remotes_name);
+extern int push_unpushed_submodules(struct sha1_array *hashes,
+ const char *remotes_name,
+ int dry_run);
void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
int parallel_submodules(void);
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index e6ccc30..54f334c 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -431,7 +431,7 @@ test_expect_success 'push unpushable submodule recursively fails' '
git -C work reset --hard master^
'
-test_expect_failure 'push --dry-run does not recursively update submodules' '
+test_expect_success 'push --dry-run does not recursively update submodules' '
(
cd work &&
(
diff --git a/transport.c b/transport.c
index f6bec0d..4ad08d0 100644
--- a/transport.c
+++ b/transport.c
@@ -921,15 +921,18 @@ int transport_push(struct transport *transport,
if (!is_null_oid(&ref->new_oid))
sha1_array_append(&hashes, ref->new_oid.hash);
- if (!push_unpushed_submodules(&hashes, transport->remote->name)) {
+ if (!push_unpushed_submodules(&hashes,
+ transport->remote->name,
+ pretend)) {
sha1_array_clear(&hashes);
die ("Failed to push all needed submodules!");
}
sha1_array_clear(&hashes);
}
- if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |
- TRANSPORT_RECURSE_SUBMODULES_CHECK)) && !is_bare_repository()) {
+ if (((flags & TRANSPORT_RECURSE_SUBMODULES_CHECK) ||
+ ((flags & TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND) &&
+ !pretend)) && !is_bare_repository()) {
struct ref *ref = remote_refs;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
struct sha1_array hashes = SHA1_ARRAY_INIT;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand
From: Brandon Williams @ 2016-11-15 1:18 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
In-Reply-To: <1479172735-698-1-git-send-email-bmwill@google.com>
This patch adds a test to illustrate how push run with --dry-run doesn't
actually perform a dry-run when push is configured to push submodules
on-demand. Instead all submodules which need to be pushed are actually
pushed to their remotes while any updates for the superproject are
performed as a dry-run. This is a bug and not the intended behaviour of
a dry-run.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
t/t5531-deep-submodule-push.sh | 26 +++++++++++++++++++++++++-
1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 198ce84..e6ccc30 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' '
cd submodule.git &&
git rev-parse master >../actual
) &&
- test_cmp expected actual
+ test_cmp expected actual &&
+ git -C work reset --hard master^
+'
+
+test_expect_failure 'push --dry-run does not recursively update submodules' '
+ (
+ cd work &&
+ (
+ cd gar/bage &&
+ git checkout master &&
+ git rev-parse master >../../../expected_submodule &&
+ > junk9 &&
+ git add junk9 &&
+ git commit -m "Ninth junk"
+ ) &&
+ git checkout master &&
+ git rev-parse master >../expected_pub
+ git add gar/bage &&
+ git commit -m "Ninth commit for gar/bage" &&
+ git push --dry-run --recurse-submodules=on-demand ../pub.git master
+ ) &&
+ git -C submodule.git rev-parse master >actual_submodule &&
+ git -C pub.git rev-parse master >actual_pub &&
+ test_cmp expected_pub actual_pub &&
+ test_cmp expected_submodule actual_submodule
'
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH 0/2] bug fix with push --dry-run and submodules
From: Brandon Williams @ 2016-11-15 1:18 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
While trying to understand how the recursive pushing of submodules worked I
discovered that when push was instructed to do a dry-run, while also configured
to push unpushed submodules 'on-demand', that the submodule pushes weren't
configured to perform dry-runs, but actually performed the pushes. This
resulted in the submodules being pushed while leaving the superproject unpushed
which is undesirable behavior for a dry-run.
This series introduces a test to illustrate the bug as well as a patch to
correct this behavior by passing the --dry-run flag to the child processes
which perform the submodule pushes during a dry-run.
This series is based against 'origin/hv/submodule-not-yet-pushed-fix'
Brandon Williams (2):
push: --dry-run updates submodules when --recurse-submodules=on-demand
push: fix --dry-run to not push submodules
submodule.c | 13 ++++++++-----
submodule.h | 4 +++-
t/t5531-deep-submodule-push.sh | 26 +++++++++++++++++++++++++-
transport.c | 9 ++++++---
4 files changed, 42 insertions(+), 10 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: RFC: Enable delayed responses to Git clean/smudge filter requests
From: Eric Wong @ 2016-11-15 1:03 UTC (permalink / raw)
To: Lars Schneider; +Cc: git
In-Reply-To: <D10F7C47-14E8-465B-8B7A-A09A1B28A39F@gmail.com>
Lars Schneider <larsxschneider@gmail.com> wrote:
> Hi,
>
> Git always performs a clean/smudge filter on files in sequential order.
> Sometimes a filter operation can take a noticeable amount of time.
> This blocks the entire Git process.
I have the same problem in many places which aren't git :>
> I would like to give a filter process the possibility to answer Git with
> "I got your request, I am processing it, ask me for the result later!".
>
> I see the following way to realize this:
>
> In unpack-trees.c:check_updates() [1] we loop through the cache
> entries and "ask me later" could be an acceptable return value of the
> checkout_entry() call. The loop could run until all entries returned
> success or error.
>
> The filter machinery is triggered in various other places in Git and
> all places that want to support "ask me later" would need to be patched
> accordingly.
That all sounds reasonable.
The filter itself would need to be aware of parallelism
if it lives for multiple objects, right?
> Do you think this could be a viable approach?
It'd probably require a bit of work, but yes, I think it's viable.
We already do this with curl_multi requests for parallel
fetching from dumb HTTP servers, but that's driven by curl
internals operating with a select/poll loop.
Perhaps the curl API could be a good example for doing this.
> Do you see a better way?
Nope, I prefer non-blocking state machines to threads for
debuggability and determinism.
Anyways, I'll plan on doing something similar (in Perl) with the
synchronous parts of public-inbox which relies on "cat-file --batch"
at some point... (my rotational disks are sloooooooow :<)
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Junio C Hamano @ 2016-11-15 1:02 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, spearce
In-Reply-To: <20161115004426.unheihlmftlw6ex7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Actually, I take it back. I think it works for a single round of ref
> negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
> fail t5551.
>
> I think I've probably made a mis-assumption on exactly when in the HTTP
> protocol we will see a flush packet (and perhaps that is a sign that
> this protocol-snooping approach is not a good one).
Hmph. I think I tried David's original under GIT_TEST_LONG and saw
it got stuck; could be the same issue, I guess.
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-15 0:44 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce
In-Reply-To: <20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net>
On Mon, Nov 14, 2016 at 02:40:49PM -0500, Jeff King wrote:
> On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:
>
> > 2. Have remote-curl understand enough of the protocol that it can
> > abort rather than hang.
> >
> > I think that's effectively the approach of your patch, but for one
> > specific case. But could we, for example, make sure that everything
> > we proxy is a complete set of pktlines and ends with a flush? And
> > if not, then we hang up on fetch-pack.
> >
> > I _think_ that would work, because even the pack is always encased
> > in pktlines for smart-http.
>
> So something like this. It turned out to be a lot uglier than I had
> hoped because we get fed the data from curl in odd-sized chunks, so we
> need a state machine.
>
> But it does seem to work. At least it doesn't seem to break anything in
> the test suite, and it fixes the new tests you added. I'd worry that
> there's some obscure case where the response isn't packetized in the
> same way.
Actually, I take it back. I think it works for a single round of ref
negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
fail t5551.
I think I've probably made a mis-assumption on exactly when in the HTTP
protocol we will see a flush packet (and perhaps that is a sign that
this protocol-snooping approach is not a good one).
I don't have time to dig more on this tonight, and I'll be traveling for
the rest of the week. So if anybody is interested, please feel free to
dig into it.
-Peff
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-14 23:48 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org, spearce@spearce.org
In-Reply-To: <a57cc9c4a0a840baab5b8123fac9388b@exmbdft7.ad.twosigma.com>
On Mon, Nov 14, 2016 at 11:25:30PM +0000, David Turner wrote:
> > But it does seem to work. At least it doesn't seem to break anything
> > in the test suite, and it fixes the new tests you added. I'd worry
> > that there's some obscure case where the response isn't packetized
> > in the same way.
>
> Overall, this looks good to me. The state machine is pretty clean. I
> think I would have used a tiny buffer for the length field, and then I
> would have regretted it. Your way looks nicer than my unwritten patch
> would have looked.
Heh, I started it that way but you end up dealing with the same states
(they're just implicit in your "how big is my temp buffer" field).
> > +#define READ_ONE_HEX(shift) do { \
> > + int val = hexval(buf[0]); \
> > + if (val < 0) { \
> > + warning("error on %d", *buf); \
> > + rpc->pktline_state = RPC_PKTLINE_ERROR; \
> > + return; \
> > + } \
> > + rpc->pktline_len |= val << shift; \
>
> Nit: parenthesize shift here, since it is a parameter to a macro.
Yeah, I'm often a bit slack on these one-off inside-a-function macros.
But it does not hurt to to be careful.
I'll make that change and then try to wrap this up with a commit
message. I plan to steal your tests, if that's OK.
-Peff
^ permalink raw reply
* RE: [PATCH] remote-curl: don't hang when a server dies before any output
From: David Turner @ 2016-11-14 23:25 UTC (permalink / raw)
To: 'Jeff King'; +Cc: git@vger.kernel.org, spearce@spearce.org
In-Reply-To: <20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net>
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Monday, November 14, 2016 2:41 PM
> To: David Turner
> Cc: git@vger.kernel.org; spearce@spearce.org
> Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
> output
>
> On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:
>
> > 2. Have remote-curl understand enough of the protocol that it can
> > abort rather than hang.
> >
> > I think that's effectively the approach of your patch, but for one
> > specific case. But could we, for example, make sure that everything
> > we proxy is a complete set of pktlines and ends with a flush? And
> > if not, then we hang up on fetch-pack.
> >
> > I _think_ that would work, because even the pack is always encased
> > in pktlines for smart-http.
>
> So something like this. It turned out to be a lot uglier than I had hoped
> because we get fed the data from curl in odd-sized chunks, so we need a
> state machine.
>
> But it does seem to work. At least it doesn't seem to break anything in
> the test suite, and it fixes the new tests you added. I'd worry that
> there's some obscure case where the response isn't packetized in the same
> way.
Overall, this looks good to me. The state machine is pretty clean. I think I would have used a tiny buffer for the length field, and then I would have regretted it. Your way looks nicer than my unwritten patch would have looked.
> +#define READ_ONE_HEX(shift) do { \
> + int val = hexval(buf[0]); \
> + if (val < 0) { \
> + warning("error on %d", *buf); \
> + rpc->pktline_state = RPC_PKTLINE_ERROR; \
> + return; \
> + } \
> + rpc->pktline_len |= val << shift; \
Nit: parenthesize shift here, since it is a parameter to a macro.
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Matt McCutchen @ 2016-11-14 22:37 UTC (permalink / raw)
To: git
In-Reply-To: <1478814800.2878.10.camel@mattmccutchen.net>
On Thu, 2016-11-10 at 16:53 -0500, Matt McCutchen wrote:
> On Wed, 2016-10-26 at 19:07 -0400, Matt McCutchen wrote:
> >
> > Maybe we would never hit any of these problems in practice, but they
> > give me a bad enough feeling that I'm planning to write my own tool
> > that tracks the upstream commit ID in a file (like a submodule) and
> > doesn't generate any extra commits. Without generating extra commits,
> > the only place to store the upstream content in the superproject would
> > be in another subtree, which would take up disk space in every working
> > tree unless developers manually set skip-worktree. I think I prefer to
> > not store the upstream content and just have the tool fetch it from a
> > local subproject repository each time it's needed.
> >
> > I'll of course post the tool on the web and would be happy to see it
> > integrated into "git subtree" if that makes sense, but I don't know how
> > much time I'd be willing to put into making that happen.
>
> I have named my tool "git subtree-lite" and posted it here:
>
> https://mattmccutchen.net/utils/git-subtree-lite.git/
As I was doing additional research in preparation for adding git-
subtree-lite to the tools page
(https://git.wiki.kernel.org/index.php/Interfaces,_frontends,_and_tools),
by chance I found an existing tool, Braid
(http://cristibalan.github.io/braid/), whose design meets my
requirements. I have a few minor concerns, but assuming I'm able to
fix them without too much work and upstream accepts my patches, I plan
to switch to Braid.
I've made a properly marked section on the tools page for subproject
management tools:
https://git.wiki.kernel.org/index.php/Interfaces,_frontends,_and_tools#Subprojects_or_sets_of_repositories
in the hope that the next person with the same requirements as me finds
Braid. (I unfortunately didn't check that page before starting, but I
will the next time I need something.)
Matt
^ permalink raw reply
* Re: [PATCH v15 02/27] bisect: rewrite `check_term_format` shell function in C
From: Stephan Beyer @ 2016-11-14 22:20 UTC (permalink / raw)
To: Pranit Bauva; +Cc: git
In-Reply-To: <01020157c38b1a74-ad2bcaff-0c92-4af4-9aa0-72d98f4945fc-000000@eu-west-1.amazonses.com>
Hi,
I saw in the recent "What's cooking" mail that this is still waiting
for review, so I thought I could interfere and help reviewing it from a
non-git-developer point of view.
But only two commits for today. The first one seems fine. The second
one makes me write this mail ;-)
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> +static int check_term_format(const char *term, const char *orig_term)
> +{
[...]
> + if (one_of(term, "help", "start", "skip", "next", "reset",
> + "visualize", "replay", "log", "run", NULL))
[... vs ...]
> -check_term_format () {
> - term=$1
> - git check-ref-format refs/bisect/"$term" ||
> - die "$(eval_gettext "'\$term' is not a valid term")"
> - case "$term" in
> - help|start|terms|skip|next|reset|visualize|replay|log|run)
Is there a reasons why "terms" has been dropped from the list?
Best
Stephan
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-14 21:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Turner, git, spearce
In-Reply-To: <xmqqpolxwyh6.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 14, 2016 at 01:19:49PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So something like this. It turned out to be a lot uglier than I had
> > hoped because we get fed the data from curl in odd-sized chunks, so we
> > need a state machine.
>
> It is unfortunate that we have to snoop the protocol like this to
> infer an error, but I do not think we can do better than that
> approach. FWIW, I did not find the logic in update_pktline_state()
> you wrote ugly at all.
>
> Having to assume that the end of each round from the other end must
> be a FLUSH does feel somewhat ugly and brittle, though.
Yeah, I agree. The other option is to signal to fetch-pack that we saw
EOF on the server request, and then we do not have to snoop; it knows
where in the protocol state it is at.
But doing that is a little tricky. We could send our own flush packet,
but that doesn't quite work. When fetch-pack sees a flush it does not
know if it is the server's flush and it should wait for our flush, or if
the server hung up prematurely and the flush is from us. :)
The "elegant" solution is to just wrap the server's data in another set
of pktlines. So the server flush becomes "00080000" (8 bytes including
our pktline, and then 4 bytes for the flush packet). But wrapping each
pktline gets a little hairy. What happens when the server sends us a
pktline of LARGE_PACKET_MAX? We can't wrap that without sending our own
pktline that's LARGE_PACKET_MAX+4.
One solution is that pktlines 0001-0003 are unused and impossible for
data. So we could send the server pktlines as-is, and use pktline 0001
as our special signal for "end of http request". A server shouldn't ever
send that, and if they did (perhaps to try something malicious), it
would just cause fetch-pack to think they prematurely ended the
conversation.
Hmm. I suppose that doesn't quite work, though. One of the problems is
that the server sends a partial pktline, in which case our special flush
packet would get gobbled up as data for that broken pktline.
So you really do need framing or some other out-of-band communication,
or resolve yourself to snooping in remote-curl.
-Peff
^ permalink raw reply
* Re: [RFH] limiting ref advertisements
From: Jeff King @ 2016-11-14 21:21 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8DwKxz14Dow9dEKeXnBriMzN_OptnGM7nPigPcS_pHX9w@mail.gmail.com>
On Tue, Oct 25, 2016 at 06:46:21PM +0700, Duy Nguyen wrote:
> > So it seems like left-anchoring the refspecs can never be fully correct.
> > We can communicate "master" to the server, who can then look at every
> > ref it would advertise and ask "could this be called master"? But it
> > will be setting in stone the set of "could this be" patterns. Granted,
> > those haven't changed much over the history of git, but it seems awfully
> > fragile.
>
> The first thought that comes to mind is, if left anchoring does not
> work, let's support both left and right anchoring. I guess you
> considered and discarded this.
>
> If prefix matching does not work, and assuming "some-prefix" sent by
> client to be in fact "**/some-prefix" pattern at server side will set
> the "could this be" in stone, how about use wildmatch? It's flexible
> enough and we have full control over the pattern matching engine so C
> Git <-> C Git should be good regardless of platforms. I understand
> that wildmatch is still complicated enough that a re-implementation
> can easily divert in behavior. But a pattern with only '*', '/**',
> '/**/' and '**/' wildcards (in other words, no [] or ?) could make the
> engine a lot simpler and still fit our needs (and give some room for
> client-optimization).
Thanks for responding to this. I've been meaning to get back to it with
some code experiments, but they keep getting bumped down in priority. So
let me at least outline some of my thoughts, without code. :)
I was hoping to avoid right-anchoring because it's expensive to find all
of the right-anchored cases (assuming that ref storage is generally
hierarchical, which it is now and probably will be for future backends).
I also don't think it covers all cases. As bizarre as it is, I believe
you can currently do:
git fetch $remote origin
and find refs/remotes/origin/HEAD.
So I think the best we can ever do is have the server look at a specific
set of patterns. Those patterns could be expressed by wildmatch. I was
just a little nervous to turn to wildmatch, because it's complicated and
we may want to update it in the future in a slightly-incompatible way.
We also want to give some preference-order to the patterns. If I give
you "refs/heads/master", and that ref exists, you do not need to tell me
whether you also have "refs/heads/refs/heads/master". So you have to
provide multiple patterns for each possible ref. And you need to group
them as "show the first one that matches from this group".
The pattern the client is using really is the ref_rev_parse_rules. So I
think the solution is more like one of:
1. Specify the pattern set ahead of time, and then the server applies
it to each refname. We need some pattern language that can express
"fill in the thing in the middle". IOW, something like:
advertise-pattern=%s
advertise-pattern=refs/tags/%s
advertise-pattern=refs/heads/%s
advertise-lookup=master
advertise-lookup=v1.0
except that the thought of using snprintf() to handle formats
provided by the user is vaguely terrifying. We could make sure they
contain only a single "%s", but given the history there, it still
makes me nervous. I guess we could write our own pseudo-%s parser
that is much more careful and complains on bugs instead of
executing arbitrary code. ;)
I don't think wildmatch quite works for that, because it wants to
have the full pattern.
2. Declare the current set of ref_rev_parse_rules as "version 1", and
send:
advertise-lookup-v1=master
advertise-lookup-v1=v1.0
and the server would do the right thing. We could do a v2, but it
gets hairy. Let's imagine we add "refs/notes/%s" to the lookup
rules, and we'll call that v2.
But remember that these are "early capabilities", before the server
has spoken at all. So the client doesn't know if we can handle v2.
So we have to send _both_ (and v2-aware servers can ignore the v1).
advertise-lookup-v1=master
advertise-lookup-v2=master
But that's not quite enough. A v1 server won't look in refs/notes
at all. So we have to say that, too:
advertise-lookup-v1=refs/notes/master
And of course the v1 server has no idea that this isn't necessary
if we already found refs/heads/master.
So I think you really do need the client to be able to say "also
look at this pattern".
Of course we do still want left-anchoring, too. Wildcards like
"refs/heads/*" are always left-anchored. So I think we'd have two types,
and a full request for
git fetch origin +refs/heads/*:refs/remotes/origin/* master:foo
would look like:
(1) advertise-pattern-v1
(2) advertise-pattern=refs/notes/%s
(3) advertise-prefix=refs/heads
(4) advertise-lookup=master
where the lines mean:
1. Use the standard v1 patterns (we could spell them out, but this
just saves bandwidth. In fact, it could just be implicit that v1
patterns are included, and we could skip this line).
2. This is for our fictional future version where the client knows
added refs/notes/* to its DWIM but the server hasn't yet.
3. Give me all of refs/heads/*
4. Look up "master" using the advertise patterns and give me the first
one you find.
So given that we can omit (1), and that (2) is just an example for the
future, it could look like:
advertise-prefix=refs/heads
advertise-lookup=master
which is pretty reasonable. It's not _completely_ bulletproof in terms
of backwards compatibility. The "v1" thing means the client can't insert
a new pattern in the middle (remember they're ordered by priority). So
maybe it is better to spell them all out (one thing that makes me
hesitate is that these will probably end up as URL parameters for the
HTTP version, which means our URL can start to get a little long).
Anyway. That's the direction I'm thinking. I haven't written the code
yet. The trickiest thing will probably be that the server would want to
avoid advertising the same ref twice via two mechanisms (or perhaps the
client just be tolerant of duplicates; that relieves the server of any
duplicate-storage requirements).
-Peff
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Junio C Hamano @ 2016-11-14 21:19 UTC (permalink / raw)
To: Jeff King; +Cc: David Turner, git, spearce
In-Reply-To: <20161114194049.mktpsvgdhex2f4zv@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So something like this. It turned out to be a lot uglier than I had
> hoped because we get fed the data from curl in odd-sized chunks, so we
> need a state machine.
It is unfortunate that we have to snoop the protocol like this to
infer an error, but I do not think we can do better than that
approach. FWIW, I did not find the logic in update_pktline_state()
you wrote ugly at all.
Having to assume that the end of each round from the other end must
be a FLUSH does feel somewhat ugly and brittle, though.
> diff --git a/remote-curl.c b/remote-curl.c
> index f14c41f4c..605357d77 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -403,6 +403,18 @@ struct rpc_state {
> struct strbuf result;
> unsigned gzip_request : 1;
> unsigned initial_buffer : 1;
> +
> + enum {
> + RPC_PKTLINE_ERROR, /* bogus hex chars in length */
> + RPC_PKTLINE_INITIAL, /* no packets received yet */
> + RPC_PKTLINE_1, /* got one hex char */
> + RPC_PKTLINE_2, /* got two hex chars */
> + RPC_PKTLINE_3, /* got three hex chars */
> + RPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */
> + RPC_PKTLINE_END_OF_PACKET, /* last packet completed */
> + RPC_PKTLINE_FLUSH, /* last packet was flush */
> + } pktline_state;
> + size_t pktline_len;
> };
>
> static size_t rpc_out(void *ptr, size_t eltsize,
> @@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
> }
> #endif
>
> +static void update_pktline_state(struct rpc_state *rpc,
> + const char *buf, size_t len)
> +{
> +#define READ_ONE_HEX(shift) do { \
> + int val = hexval(buf[0]); \
> + if (val < 0) { \
> + warning("error on %d", *buf); \
> + rpc->pktline_state = RPC_PKTLINE_ERROR; \
> + return; \
> + } \
> + rpc->pktline_len |= val << shift; \
> + buf++; \
> + len--; \
> +} while(0)
> +
> + while (len > 0) {
> + switch (rpc->pktline_state) {
> + case RPC_PKTLINE_ERROR:
> + /* previous error; there is no recovery */
> + return;
> +
> + /* We can start a new pktline at any of these states */
> + case RPC_PKTLINE_INITIAL:
> + case RPC_PKTLINE_FLUSH:
> + case RPC_PKTLINE_END_OF_PACKET:
> + rpc->pktline_len = 0;
> + READ_ONE_HEX(12);
> + rpc->pktline_state = RPC_PKTLINE_1;
> + break;
> +
> + case RPC_PKTLINE_1:
> + READ_ONE_HEX(8);
> + rpc->pktline_state = RPC_PKTLINE_2;
> + break;
> +
> + case RPC_PKTLINE_2:
> + READ_ONE_HEX(4);
> + rpc->pktline_state = RPC_PKTLINE_3;
> + break;
> +
> + case RPC_PKTLINE_3:
> + READ_ONE_HEX(0);
> + if (rpc->pktline_len) {
> + rpc->pktline_state = RPC_PKTLINE_DATA;
> + rpc->pktline_len -= 4;
> + } else
> + rpc->pktline_state = RPC_PKTLINE_FLUSH;
> + break;
> +
> + case RPC_PKTLINE_DATA:
> + if (len < rpc->pktline_len) {
> + rpc->pktline_len -= len;
> + len = 0;
> + } else {
> + buf += rpc->pktline_len;
> + len -= rpc->pktline_len;
> + rpc->pktline_len = 0;
> + rpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;
> + }
> + break;
> + }
> + }
> +#undef READ_ONE_HEX
> +}
> +
> static size_t rpc_in(char *ptr, size_t eltsize,
> size_t nmemb, void *buffer_)
> {
> size_t size = eltsize * nmemb;
> struct rpc_state *rpc = buffer_;
> + update_pktline_state(rpc, ptr, size);
> write_or_die(rpc->in, ptr, size);
> return size;
> }
> @@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)
> curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
> curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
>
> + rpc->pktline_state = RPC_PKTLINE_INITIAL;
> +
> err = run_slot(slot, NULL);
> if (err == HTTP_REAUTH && !large_request) {
> credential_fill(&http_auth);
> @@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)
> if (err != HTTP_OK)
> err = -1;
>
> + if (rpc->pktline_state != RPC_PKTLINE_FLUSH) {
> + error("invalid or truncated response from http server");
> + err = -1;
> + }
> +
> curl_slist_free_all(headers);
> free(gzip_body);
> return err;
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Junio C Hamano @ 2016-11-14 21:10 UTC (permalink / raw)
To: Michael J Gruber
Cc: Johannes Schindelin, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <a3db4c55-550c-f2e8-83b8-46c2be86f7da@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Junio C Hamano venit, vidit, dixit 14.11.2016 19:01:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>>> *My* idea of --no-index was for it to behave as similar to the
>>> --index-version as possible, regarding formatting etc., and to be a good
>>> substitute for ordinary diff. The proposed patch achieves exactly that -
>> ...
> It's not clear to me what you are saying here - 1/2 makes git diff
> follow symbolic links, yes, just like ordinary diff.
Yes, which can be seen as deviating from your earlier "as similar to
the --index version as possible" goal, which I think was where Dscho's
complaint comes from.
I _think_ the no-index mode was primarily for those who want to use
our diff as a replacement for GNU and other diffs, and from that
point of view, I'd favour not doing the "comparing symbolic link?
We'll show the difference between the link contents, not target"
under no-index mode myself. That is a lot closer to the diff other
people implemented, not ours. Hence the knee-jerk reaction I gave
in
http://public-inbox.org/git/xmqqinrt1zcx.fsf@gitster.mtv.corp.google.com
^ permalink raw reply
* RFC: Enable delayed responses to Git clean/smudge filter requests
From: Lars Schneider @ 2016-11-14 21:09 UTC (permalink / raw)
To: Git Mailing List
Hi,
Git always performs a clean/smudge filter on files in sequential order.
Sometimes a filter operation can take a noticeable amount of time.
This blocks the entire Git process.
I would like to give a filter process the possibility to answer Git with
"I got your request, I am processing it, ask me for the result later!".
I see the following way to realize this:
In unpack-trees.c:check_updates() [1] we loop through the cache
entries and "ask me later" could be an acceptable return value of the
checkout_entry() call. The loop could run until all entries returned
success or error.
The filter machinery is triggered in various other places in Git and
all places that want to support "ask me later" would need to be patched
accordingly.
--
Do you think this could be a viable approach?
Do you see a better way?
Thanks,
Lars
[1] https://github.com/git/git/blob/3ab228137f980ff72dbdf5064a877d07bec76df9/unpack-trees.c#L267
^ permalink raw reply
* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Michael J Gruber @ 2016-11-14 20:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Jacob Keller, Dennis Kaarsemaker,
Git mailing list
In-Reply-To: <xmqqoa1ix7nq.fsf@gitster.mtv.corp.google.com>
Junio C Hamano venit, vidit, dixit 14.11.2016 19:01:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> *My* idea of --no-index was for it to behave as similar to the
>> --index-version as possible, regarding formatting etc., and to be a good
>> substitute for ordinary diff. The proposed patch achieves exactly that -
>
> Does it? It looks to me that it does a lot more.
Yes, I didn't mean to say that it achieves only that - it achieves that
one goal exactly, and more.
>> why should a *file* argument (which is not a pathspec in --no-index
>> mode) not be treated in the same way in which every other command treats
>> a file argument? The patch un-breaks the most natural expectation.
>
> I think a filename given as a command line argument, e.g. <(cmd), is
> now treated more sensibly with [2/2]. Something that is not a
> directory to be descended into and is not a regular file needs to be
> made into a form that we can use as a blob, and reading it into an
> in-core buffer is a workable way to do so.
Yes.
> However, when taken together with [1/2], doesn't the proposed patch
> "achieves" a lot more than "exactly that", namely, by not treating
> symbolic links discovered during traversals of directories given
> from the command line as such and dereferencing?
It's not clear to me what you are saying here - 1/2 makes git diff
follow symbolic links, yes, just like ordinary diff. If I 'diff' two
dirs that contain symbolic links with the same name pointing to
different files I get a diff between the contents, not between the
filenames.
I like the proposed change a lot, maybe that didn't come across clearly.
I think it makes things more "predictable" in the sense that it meets
typical expectations.
Michael
^ permalink raw reply
* Re: [PATCH] attr: mark a file-local symbol as static
From: Stefan Beller @ 2016-11-14 20:00 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <83508d1f-e809-f6be-5afc-4c23195dbd08@ramsayjones.plus.com>
On Sun, Nov 13, 2016 at 8:42 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Stefan,
>
> If you need to re-roll your 'sb/attr' branch, could you please
> squash this into the relevant patch.
will do. I have it applied locally
>
> Alternatively, since there is only a single call site for git_attr()
> (on line #1005), you could perhaps remove git_attr() and inline that
> call. (However, that does make that line exceed 80 columns).
I'll look into that.
Thanks,
Stefan
>
> Thanks!
>
> ATB,
> Ramsay Jones
>
> attr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/attr.c b/attr.c
> index 667ba85..84c4b08 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -169,7 +169,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)
> return a;
> }
>
> -struct git_attr *git_attr(const char *name)
> +static struct git_attr *git_attr(const char *name)
> {
> return git_attr_internal(name, strlen(name));
> }
> --
> 2.10.0
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Junio C Hamano @ 2016-11-14 19:51 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZSFuq2+6xsrJ=CcXuOVbTnbDirbRtu7Fonfk+9EdRpbxg@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>> - More importantly, what do these do? I do not think of a good
>> description that generalizes "base of refs/foo/bar/boz is foo" to
>> explain your :base.
>
> $ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
> refs/heads/master refs/heads
> refs/heads/ref-filter refs/heads
> refs/remotes/junio/va/i18n refs/remotes/junio/va
>
> $ ./git for-each-ref refs/heads --format
> "%(align:left,30)%(refname)%(end) %(refname:base)"
> refs/heads/master heads
> refs/heads/ref-filter heads
> refs/remotes/junio/va/i18n remotes
>
> I guess this should clear it up.
Hmph.
I would guess from these examples that :dir is an equivalent to
dirname(). But it is unclear how :base is defined. Is it the path
component that comes immediately after "refs/" that appears at the
beginning?
^ permalink raw reply
* Re: [PATCH] fetch/push: document that private data can be leaked
From: Junio C Hamano @ 2016-11-14 19:47 UTC (permalink / raw)
To: Jeff King; +Cc: Matt McCutchen, git
In-Reply-To: <20161114190725.fxjymvztc2eiomv6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So I think the in-between answer is "it is OK to push to an
> untrustworthy place, but do not do it from a repo that may contain
> secret contents".
Yes, that sounds like a sensible piece of advice to give to the
readers.
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Ramsay Jones @ 2016-11-14 19:45 UTC (permalink / raw)
To: Jeff King, Torsten Bögershausen
Cc: Lars Schneider, Junio C Hamano, Johannes Schindelin,
Johannes Sixt, git, pranit.bauva
In-Reply-To: <20161114170105.btnohk2777ddaiul@sigill.intra.peff.net>
On 14/11/16 17:01, Jeff King wrote:
> On Mon, Nov 14, 2016 at 05:35:56PM +0100, Torsten Bögershausen wrote:
>
>>> Git 'pu' does not compile on macOS right now:
>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used uninitialized
>>> whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>
> The next step is to make sure that the topic author is aware (in this
> case, one assumes it's pb/bisect).
[+cc Pranit]
Yep, I had a quick squint, and it looks like the compiler is correct.
It should be complaining about the 'bad_syn' variable for exactly the
same reason: namely, whenever the if condition is true, the only exit
from that block is via 'goto finish' which bypasses the initialisation
of 'good_syn' and 'bad_syn'.
> Better still is to make a patch that can either be applied on top, or
> squashed as appropriate.
No patch this time, but it simply requires those variables to be
initialised to NULL in their declarations. :-D
> I know that Ramsay Jones does this, for
> example, with some of his sparse-related checks, and I'm pretty sure
> from the turnaround-time that he runs it against "pu".
Yep, the idea being to catch these simple problems before the topic
reaches 'next'.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] remote-curl: don't hang when a server dies before any output
From: Jeff King @ 2016-11-14 19:40 UTC (permalink / raw)
To: David Turner; +Cc: git, spearce
In-Reply-To: <20161114182431.e7jjnq422c4xobdb@sigill.intra.peff.net>
On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:
> 2. Have remote-curl understand enough of the protocol that it can
> abort rather than hang.
>
> I think that's effectively the approach of your patch, but for one
> specific case. But could we, for example, make sure that everything
> we proxy is a complete set of pktlines and ends with a flush? And
> if not, then we hang up on fetch-pack.
>
> I _think_ that would work, because even the pack is always encased
> in pktlines for smart-http.
So something like this. It turned out to be a lot uglier than I had
hoped because we get fed the data from curl in odd-sized chunks, so we
need a state machine.
But it does seem to work. At least it doesn't seem to break anything in
the test suite, and it fixes the new tests you added. I'd worry that
there's some obscure case where the response isn't packetized in the
same way.
---
diff --git a/remote-curl.c b/remote-curl.c
index f14c41f4c..605357d77 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -403,6 +403,18 @@ struct rpc_state {
struct strbuf result;
unsigned gzip_request : 1;
unsigned initial_buffer : 1;
+
+ enum {
+ RPC_PKTLINE_ERROR, /* bogus hex chars in length */
+ RPC_PKTLINE_INITIAL, /* no packets received yet */
+ RPC_PKTLINE_1, /* got one hex char */
+ RPC_PKTLINE_2, /* got two hex chars */
+ RPC_PKTLINE_3, /* got three hex chars */
+ RPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */
+ RPC_PKTLINE_END_OF_PACKET, /* last packet completed */
+ RPC_PKTLINE_FLUSH, /* last packet was flush */
+ } pktline_state;
+ size_t pktline_len;
};
static size_t rpc_out(void *ptr, size_t eltsize,
@@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
}
#endif
+static void update_pktline_state(struct rpc_state *rpc,
+ const char *buf, size_t len)
+{
+#define READ_ONE_HEX(shift) do { \
+ int val = hexval(buf[0]); \
+ if (val < 0) { \
+ warning("error on %d", *buf); \
+ rpc->pktline_state = RPC_PKTLINE_ERROR; \
+ return; \
+ } \
+ rpc->pktline_len |= val << shift; \
+ buf++; \
+ len--; \
+} while(0)
+
+ while (len > 0) {
+ switch (rpc->pktline_state) {
+ case RPC_PKTLINE_ERROR:
+ /* previous error; there is no recovery */
+ return;
+
+ /* We can start a new pktline at any of these states */
+ case RPC_PKTLINE_INITIAL:
+ case RPC_PKTLINE_FLUSH:
+ case RPC_PKTLINE_END_OF_PACKET:
+ rpc->pktline_len = 0;
+ READ_ONE_HEX(12);
+ rpc->pktline_state = RPC_PKTLINE_1;
+ break;
+
+ case RPC_PKTLINE_1:
+ READ_ONE_HEX(8);
+ rpc->pktline_state = RPC_PKTLINE_2;
+ break;
+
+ case RPC_PKTLINE_2:
+ READ_ONE_HEX(4);
+ rpc->pktline_state = RPC_PKTLINE_3;
+ break;
+
+ case RPC_PKTLINE_3:
+ READ_ONE_HEX(0);
+ if (rpc->pktline_len) {
+ rpc->pktline_state = RPC_PKTLINE_DATA;
+ rpc->pktline_len -= 4;
+ } else
+ rpc->pktline_state = RPC_PKTLINE_FLUSH;
+ break;
+
+ case RPC_PKTLINE_DATA:
+ if (len < rpc->pktline_len) {
+ rpc->pktline_len -= len;
+ len = 0;
+ } else {
+ buf += rpc->pktline_len;
+ len -= rpc->pktline_len;
+ rpc->pktline_len = 0;
+ rpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;
+ }
+ break;
+ }
+ }
+#undef READ_ONE_HEX
+}
+
static size_t rpc_in(char *ptr, size_t eltsize,
size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
struct rpc_state *rpc = buffer_;
+ update_pktline_state(rpc, ptr, size);
write_or_die(rpc->in, ptr, size);
return size;
}
@@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+ rpc->pktline_state = RPC_PKTLINE_INITIAL;
+
err = run_slot(slot, NULL);
if (err == HTTP_REAUTH && !large_request) {
credential_fill(&http_auth);
@@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)
if (err != HTTP_OK)
err = -1;
+ if (rpc->pktline_state != RPC_PKTLINE_FLUSH) {
+ error("invalid or truncated response from http server");
+ err = -1;
+ }
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
^ permalink raw reply related
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-14 19:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqq60nqzuye.fsf@gitster.mtv.corp.google.com>
On Mon, Nov 14, 2016 at 7:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>>>> index 600b703..f4ad297 100644
>>>> --- a/Documentation/git-for-each-ref.txt
>>>> +++ b/Documentation/git-for-each-ref.txt
>>>> @@ -96,7 +96,9 @@ refname::
>>>> slash-separated path components from the front of the refname
>>>> (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
>>>> `<N>` must be a positive integer. If a displayed ref has fewer
>>>> - components than `<N>`, the command aborts with an error.
>>>> + components than `<N>`, the command aborts with an error. For the base
>>>> + directory of the ref (i.e. foo in refs/foo/bar/boz) append
>>>> + `:base`. For the entire directory path append `:dir`.
>
> Sorry that I missed this so far and I do not know how many recent
> rerolls had them like this, but I am not sure about these :base and
> :dir suffixes. From their names I think readers would expect that
> they do rough equivalents to basename() and dirname() applied to the
> refname, but the example contradicts with that intuition. The
> result of applying basename() to 'refs/boo/bar/boz' would be 'boz'
> and not 'foo' as the example says.
>
True that the options ':dir' and ':base' seem to be conflicting with
the use case
of basename() and dirname(), These were names which I thought best fit
the options
with no relation to basename() and dirname(). But now that you mention
it, it would
make sense to change these names to something more suitable, any suggestions
would be welcome.
> So assuming that :base and :dir are unrelated to basename() and
> dirname():
>
> - I think calling these :base and :dir may be misleading
>
> - More importantly, what do these do? I do not think of a good
> description that generalizes "base of refs/foo/bar/boz is foo" to
> explain your :base.
$ ./git for-each-ref --format "%(refname)%(end) %(refname:dir)"
refs/heads/master refs/heads
refs/heads/ref-filter refs/heads
refs/remotes/junio/va/i18n refs/remotes/junio/va
$ ./git for-each-ref refs/heads --format
"%(align:left,30)%(refname)%(end) %(refname:base)"
refs/heads/master heads
refs/heads/ref-filter heads
refs/remotes/junio/va/i18n remotes
I guess this should clear it up.
>
> - A :dir that corresponds to the :base that picks 'foo' from
> 'refs/foo/bar/boz' needs an example, too.
>
I could add in an example.
> Or is the above example simply a typo? Is refs/foo/bar/boz:base
> 'boz', not 'foo'?
>
>
Not a typo, but open to changes in name.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-11-14 19:24 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xqRjMThZF7u_W6G0sHjFP3j5PMr=TszC6UxL2XCYO+CVA@mail.gmail.com>
Hello,
On Wed, Nov 9, 2016 at 5:45 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> This is part of unification of the commands 'git tag -l, git branch -l
>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>> printing options.
>>
>> Initially posted here: $(gmane/279226). It was decided that this series
>> would follow up after refactoring ref-filter parsing mechanism, which
>> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>>
>> v1 can be found here: $(gmane/288342)
>> v2 can be found here: $(gmane/288863)
>> v3 can be found here: $(gmane/290299)
>> v4 can be found here: $(gmane/291106)
>> v5b can be found here: $(gmane/292467)
>> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
>>
>
> I reviewed the full series. I found a few minor things I would have
> done differently, but overall I think it looks good. Thanks for the
> hard work and the time invested here. I remember seeing this on the
> list quite some time ago, so it's nice to see it finally come
> together.
>
Thanks for the review. I've gone through all the mails and will follow
up with replies.
Will post the next version as soon as possible. Thanks.
--
Regards,
Karthik Nayak
^ 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