* Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context
From: Jeff King @ 2018-12-12 11:02 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, gitster, masayasuzuki
In-Reply-To: <df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com>
On Tue, Dec 11, 2018 at 04:25:15PM -0800, Josh Steadmon wrote:
> From: Masaya Suzuki <masayasuzuki@google.com>
>
> In the Git pack protocol definition, an error packet may appear only in
> a certain context. However, servers can face a runtime error (e.g. I/O
> error) at an arbitrary timing. This patch changes the protocol to allow
> an error packet to be sent instead of any packet.
>
> Following this protocol spec change, the error packet handling code is
> moved to pkt-line.c.
This is a change in the spec with an accompanying change in the code,
which raises the question: what do other implementations do with this
change (both older Git, and implementations like JGit, libgit2, etc)?
I think the answer for older Git is "hang up unceremoniously", which is
probably OK given the semantics of the change. And I'd suspect most
other implementations would do the same. I just wonder if anybody tested
it with other implementations.
> +An error packet is a special pkt-line that contains an error string.
> +
> +----
> + error-line = PKT-LINE("ERR" SP explanation-text)
> +----
> +
> +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY
> +be sent. Once this packet is sent by a client or a server, the data transfer
> +process defined in this protocol is terminated.
The packfile data is typically packetized, too, and contains arbitrary
data (that could have "ERR" in it). It looks like we don't specifically
say PKT-LINE() in that part of the protocol spec, though, so I think
this is OK.
Likewise, in the implementation:
> diff --git a/pkt-line.c b/pkt-line.c
> index 04d10bbd03..ce9e42d10e 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> return PACKET_READ_EOF;
> }
>
> + if (starts_with(buffer, "ERR ")) {
> + die(_("remote error: %s"), buffer + 4);
> + }
> +
> if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> len && buffer[len-1] == '\n')
> len--;
This ERR handling has been moved to a very low level. What happens if
we're passing arbitrary data via the packet_read() code? Could we
erroneously trigger an error if a packfile happens to have the bytes
"ERR " at a packet boundary?
For packfiles via upload-pack, I _think_ we're OK, because we only
packetize it when a sideband is in use. In which case this would never
match, because we'd have "\1" in the first byte slot.
But are there are other cases we need to worry about? Just
brainstorming, I can think of:
1. We also pass packetized packfiles between git-remote-https and
the stateless-rpc mode of fetch-pack/send-pack. And I don't think
we use sidebands there.
2. The packet code is used for long-lived clean/smudge filters these
days, which also pass arbitrary data.
So I think it's probably not a good idea to unconditionally have callers
of packet_read_with_status() handle this. We'd need a flag like
PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.
-Peff
^ permalink raw reply
* Re: Minor(?) usability issue with branch.<name>.pushRemote
From: Junio C Hamano @ 2018-12-12 9:48 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <871s6n43ng.fsf@javad.com>
Sergey Organov <sorganov@gmail.com> writes:
> So, finally, it's 'branch.linux.pushremote' that is the "offender".
>
> Looks like both 'git status' and 'git branch -vv' should somehow learn
> about 'branch.<name>.pushremote' feature so that their
> output/suggestions make more sense?
Doesn't "ahead of X by N" mean "you forked from X and built N commits
on top", not "you have N commits that is not in X which is where you
would push to"?
^ permalink raw reply
* Re: [PATCH 2/2] help -a: handle aliases with long names gracefully
From: Junio C Hamano @ 2018-12-12 9:43 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <850cd5d15abe5b8a4356efe86ba81bc032278dce.1544540287.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We take pains to determine the longest command beforehand, so that we
> can align the category column after printing the command names.
>
> However, then we re-use that value when printing the aliases. If any
> alias name is longer than the longest command name, we consequently try
> to add a negative number of spaces (but `mput_char()` does not expect
> any negative values and simply decrements until the value is 0, i.e.
> it tries to add close to 2**31 spaces).
Thanks. Let's take this and a few other topics that fix 2.20 brown
paper bag breakages and prepare for 2.20.1 soonish.
^ permalink raw reply
* Re: [PATCH v3 0/4] Unify pkt-line error handling and refactor smart-http
From: Junio C Hamano @ 2018-12-12 8:43 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, peff, masayasuzuki
In-Reply-To: <cover.1544572142.git.steadmon@google.com>
Josh Steadmon <steadmon@google.com> writes:
> This is a reroll of js/smart-http-detect-remote-error that also includes
> a fixed version of ms/proto-err-packet-anywhere [1].
Yay. Thanks for reducing a topic I have to worry about by 1 ;-).
> The first patch clarifies the use of ERR messages in the pkt-line
> protocol and unifies error handling in pkt-line.c
>
> The second patch refactors smart-http discovery in remote-curl.c. Among
> other improvements, it makes more use of the pkt-line functions, which
> allows us to catch remote errors that were previously ignored.
>
> The third patch makes the version check in remote-curl more strict.
>
> The final patch adds a test to verify that the fix in patch #2 does
> actually catch remote HTTP errors.
Thanks. All look sensible. Will queue.
>
> [1]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/
>
> Jeff King (2):
> remote-curl: refactor smart-http discovery
> remote-curl: tighten "version 2" check for smart-http
>
> Josh Steadmon (1):
> lib-httpd, t5551: check server-side HTTP errors
>
> Masaya Suzuki (1):
> pack-protocol.txt: accept error packets in any context
>
> Documentation/technical/pack-protocol.txt | 20 ++---
> builtin/archive.c | 2 -
> connect.c | 3 -
> fetch-pack.c | 2 -
> pkt-line.c | 4 +
> remote-curl.c | 93 ++++++++++++++---------
> t/lib-httpd.sh | 1 +
> t/lib-httpd/apache.conf | 4 +
> t/lib-httpd/error-smart-http.sh | 3 +
> t/t5551-http-fetch-smart.sh | 5 ++
> t/t5703-upload-pack-ref-in-want.sh | 4 +-
> 11 files changed, 89 insertions(+), 52 deletions(-)
> create mode 100644 t/lib-httpd/error-smart-http.sh
^ permalink raw reply
* Re: [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf
From: Junio C Hamano @ 2018-12-12 8:38 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget; +Cc: git
In-Reply-To: <pull.98.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> The verbose output included these values. Note how '\r' appears in the
> variable values.
> ...
> ++ read tag rest
Interesting. "read" does not realize that it is reading from a text
file and do whatever appropriate for the platform (i.e. treat CRLF
as the end-of-line)?
> Derrick Stolee (1):
> .gitattributes: ensure t/oid-info/* has eol=lf
>
> .gitattributes | 1 +
> 1 file changed, 1 insertion(+)
> base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Sounds good. The base is 2.20; the problematic topic was designed
to be mergeable to 2.19.x track, but I do not forsee us merging the
bc/hash-independent-tests topic to the maintenance track, so for
this particular fix, it should be OK to base the fix there.
^ permalink raw reply
* Issues with gitattributes pattern matching
From: Sebastian Schuberth @ 2018-12-12 8:23 UTC (permalink / raw)
To: git
Hi,
after reading though [1] and [2] again, I believe a pattern in
.gitattributes like
*/src/*/assets/**/*-expected-* text eol=lf
should match a committed file at
reporter/src/funTest/assets/NPM-is-windows-1.0.2-expected-NOTICE
In other words, "**" should be able to match "nothing", but it doesn't
seem to do in my case.
To cross-check, assuming that ls-files supports the same patterns, running
git ls-files "*/src/*/assets/**/*-expected-*"
indeed does not list the committed file at
reporter/src/funTest/assets/NPM-is-windows-1.0.2-expected-NOTICE
for me. Tested with Git 2.20 on Windows and Git 2.19.2 on Linux. Is it a
documentation error or a bug in Git?
[1] https://git-scm.com/docs/gitattributes
[2] https://git-scm.com/docs/gitignore#_pattern_format
--
Sebastian Schuberth
^ permalink raw reply
* Re: Announcing Pro Git Second Edition Reedited
From: Robert P. J. Day @ 2018-12-12 7:58 UTC (permalink / raw)
To: Jon Forrest; +Cc: Jeff King, git
In-Reply-To: <a1941151-9453-5830-7175-7c8e27425274@gmail.com>
On Tue, 11 Dec 2018, Jon Forrest wrote:
> On 12/11/2018 2:50 AM, Jeff King wrote:
>
> > The content at https://git-scm.com/book is pulled regularly from
> > https://github.com/progit/progit2, which has collected a number of
> > fixes (as well as translations) since the 2nd edition was
> > released.
> >
> > Have you considered sending some of your edits there? It sounds
> > like they may be too large to just dump as a big PR, but it might
> > be possible to grow together over time.
>
> Fair question. I had tried doing this for the first edition of Pro
> Git, but the person who was in charge of accepting changes wasn't a
> native speaker of English. As a result I had a hard time convincing
> him that my changes were necessary. Many of my changes were very
> subjective, and not technical, so this was hard to overcome. Things
> might have been different if I were correcting technical errors or
> adding significant sections to the book. But, since I'm not a Git
> expert, that's not what I was attempting to do.
>
> Things have changed for the better for the second edition of Pro
> Git. Its management seems much more willing to accept the kind of
> changes I make, as shown by their reaction to the excellent work by
> Robert Day.
thank ya, thank ya very much. :-) most of my submissions to that
book have been cosmetic -- punctuation, font changes, clarifications
-- and others are to keep up with changes to git. that said, i
definitely have ideas for more wide-ranging changes if i ever get the
time; i think some section re-ordering could be helpful but that's all
in due time.
i do what i can.
rday
--
========================================================================
Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca/dokuwiki
Twitter: http://twitter.com/rpjday
LinkedIn: http://ca.linkedin.com/in/rpjday
========================================================================
^ permalink raw reply
* Re: [PATCH] parse-options: fix SunCC compiler warning
From: Junio C Hamano @ 2018-12-12 7:50 UTC (permalink / raw)
To: Duy Nguyen
Cc: Ævar Arnfjörð Bjarmason, Git Mailing List,
Eric Sunshine, SZEDER Gábor
In-Reply-To: <CACsJy8BFn7xbJO8G_mTeeHBbZ3J52ZQkSAyrmyxe=BfVaapPEA@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> I have a better plan: stop exposing parse-options loop to outside.
> What all these commands need is the ability to deal with unknown
> options (or non-options in update-index case). They could register
> callbacks to deal with those and keep calling parse_options() instead.
> I'm converting rev-list to use parse_options() and as an intermediate
> step, this callback idea should work well. The end game though is a
> single parse_options() call that covers everything in, say, git-log.
> But we will see.
Sounds like a good plan.
Thanks.
^ permalink raw reply
* Re: [PATCH 5/5] midx: implement midx_repack()
From: Junio C Hamano @ 2018-12-12 7:40 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget
Cc: git, sbeller, peff, jrnieder, avarab, Derrick Stolee
In-Reply-To: <41ef671ec8361a9635dc78c078d2d84e9d985236.1544465177.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> + SECOND_SMALLEST_SIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 2 | tail -n 1) &&
awk is capable of remembering $5 from each line of input, sorting
them and picking the second smallest element from it, isn't it?
> + BATCH_SIZE=$(($SECOND_SMALLEST_SIZE + 1)) &&
... or incrementing the number by one, before reporting, for that
matter.
^ permalink raw reply
* Re: [PATCH 3/8] entry: support CE_WT_REMOVE flag in checkout_entry
From: Junio C Hamano @ 2018-12-12 7:36 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Thomas Gummerer, Git Mailing List, Elijah Newren
In-Reply-To: <CACsJy8C6VuH=hr9JE+AXeqU5V9RwouzZSb5+jV++GHU3myoDTA@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Although we could make it clear by saying "5 paths updated, 2 deleted"
> (but that may make us say "3 paths added" as well, hmm). Or maybe just
> "%d paths updated" where updates include file creation and deletion.
Yeah, the last one is the simplest and good.
^ permalink raw reply
* Re: [PATCH v2 1/3] git clone <url> C:\cygwin\home\USER\repo' is working (again)
From: Johannes Sixt @ 2018-12-12 7:29 UTC (permalink / raw)
To: Steven Penny; +Cc: Johannes Schindelin, git
In-Reply-To: <CAAXzdLU7dJGOW689tDkYuRYko1zYHXMcj_2PaVa0qStYA7ELNw@mail.gmail.com>
Am 12.12.18 um 01:42 schrieb Steven Penny:
> On Tue, Dec 11, 2018 at 7:39 AM Johannes Schindelin wrote:
>>> - pc-windows
>>> - pc-win
>>> - win
>>
>> I find all of those horrible.
>
> one windows triplet in use is "x86_64-pc-windows", used by Rust:
>
> https://forge.rust-lang.org/other-installation-methods.html
>
> which is how i came about my suggestions - again they arent great but they arent
> misleading as "Win32" is.
As long as C:\Windows\System32 on my Windows computer contains only
64-Bit binaries, I consider the characters "3" and "2" next to each
other in this order just noise and without any form of information. The
important part of the name is "win".
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/8] introduce no-overlay and cached mode in git checkout
From: Junio C Hamano @ 2018-12-12 7:28 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Elijah Newren, Git Mailing List,
Nguyễn Thái Ngọc
In-Reply-To: <20181211225241.GW4883@hank.intra.tgummerer.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> I think I got the right solution for that in patch 5, with deleting
> the file if it's deleted in "their" version and we pass --theirs to
> 'git checkout', and analogous for --ours. I was just wondering if
> there were any further edge cases that I can't think of right no.
That sounds quite sensible.
^ permalink raw reply
* Re: [PATCHSET] git-reverse-trailer-xrefs: Reverse map cherry-picks and other cross-references
From: Junio C Hamano @ 2018-12-12 7:26 UTC (permalink / raw)
To: Tejun Heo; +Cc: git, Junio C Hamano, Jeff King, kernel-team, Stefan Xenos
In-Reply-To: <20181211234909.2855638-1-tj@kernel.org>
Tejun Heo <tj@kernel.org> writes:
> Some trailers refer to other commits. Let's call them xrefs
> (cross-references). For example, a cherry pick trailer points to the
> source commit. It is sometimes useful to build a reverse map of these
> xrefs - ie. source -> cherry-pick instead of cherry-pick -> source.
>
> This, e.g, can answer which releases a commit ended up in on
> repositories which cherry-picks fixes back from the development
> branch. Let's say the repository looks like the following.
>
> D'---E'' release-B
> /
> C' E' release-D
> / /
> A---B---C---D---E master
>
> where the followings cherry-picks took place.
>
> C -> C'
> D -> D'
> E -> E' -> E''
>
> Using the reverse-mapping in this patchset, we can get the following
> annotated log which succinctly shows which commit ended up where.
> ...
Thanks. A few comments, after skimming the patches enough to get
the general feel of the design but before deeply reading them to see
how complete the implementation is.
The topic introduces two new hooks: one to run immediately after
cherry-picking so that the reverse mapping for a single src->dst
pair can be added, and another to run immediately after fetching so
that all new commits that have appeared over the wire can be scanned
to see if any of them is a cherry-pick of other commits.
They are good starting points, but for a complete solution I'd
imagine that you'd need to hook to many other places in the
workflow. For example, you'd need to deal with the case where a
commit created by cherry-picking an original is further rewritten
with "commit --amend" or "rebase". They may already trigger the
post rewrite hook, so you may not have to add a new hook, but in
[3/5], you'd need to describe in the documentaiton for the new
reverse-trailer command full set of hooks you expect the command to
be used to maintain the reverse mapping, as the hooks you give them
as examples in [5/5] are merely a part of a complete solution.
It also is not immediately obvious to me what your general strategy
to maintain this reverse mapping is, when new ways and codepaths to
cause new commits with "cherry-picked-from" trailer appear. Do we
keep piling on new hooks? Or is the reverse mapping information
allowed to be a bit stale and be completed immediately before it
gets used? A totally different approach could be to record list of
commits, all commits behind which have been scanned for reverse
mapping, in the tip of the notes history, perhaps in the commit log
message (which is machine generated anyway). Then, before you need
the up-to-date-to-the-last-second reverse mapping, you could run
git rev-list --all --not $these_tips_recorded
and scan the commits, just like you scan what you fetched. And when
you update the reverse mapping notes tree, the commit to record that
notes update can record the tip of the above traverseal.
I also wonder how this topic and the topic Stefan Xenos has been
working on (cf. <20181201005240.113210-1-sxenos@google.com>) can
benefit from each other by cross pollination. Stefan's topic also
needs to answer, given a commit, what other commits were created out
of it by cherry-picking and then further amending the results, so
that when the original commit itself gets amended, the cherry-picked
ones that were created from the original can be easily identified
and get updated in the same way as necessary.
The motivating workflow your topic's cover letter gives us is to
maintain the release branch that goes only forward, so in that
context, how a commit on the release branch that was created by
cherry-picking an original gets updated when the original commit
gets amended would be different (I am assuming that you cannot
affored to rebase the release branch to update a deeply buried
commit that was an earlier cherry-pick), but I would imagine that
both topics share the need for a good data structure to keep track
of (1) the relationship between the original and copy of the
cherry-pick and (2) the fact that the original of such a cherry-pick
is now stale and the copy might want to get updated.
Thanks.
^ permalink raw reply
* Minor(?) usability issue with branch.<name>.pushRemote
From: Sergey Organov @ 2018-12-12 7:15 UTC (permalink / raw)
To: git
Hello,
I've got confusing behavior and the cause was somewhat hard to discover:
-- 8< --
$ git status
On branch linux
Your branch is ahead of 'vendor/jps2rin_arm' by 2 commits.
(use "git push" to publish your local commits)
nothing to commit, working tree clean
$ git push
Everything up-to-date
$ git status
On branch linux
Your branch is ahead of 'vendor/jps2rin_arm' by 2 commits.
(use "git push" to publish your local commits)
nothing to commit, working tree clean
$ git branch -vv
* linux e8906f9 [vendor/jps2rin_arm: ahead 2] Linux: get rid of unused files
master 4d1f931 [origin/master] Linux: add README and config
-- 8< --
What's going on here? Why 'git status' and 'git branch' both insist
there are 2 unpushed commits yet 'git push' does nothing? Let's try to
figure:
-- 8< --
$ git push -v
Pushing to /var/local/group/firmware/git/jps2rin
To /var/local/group/firmware/git/jps2rin
= [up to date] linux -> linux
updating local tracking ref 'refs/remotes/origin/linux'
Everything up-to-date
-- 8< --
So it pushes branch 'linux' to 'linux' at
/var/local/group/firmware/git/jps2rin
where everything is already published... And this push destination
doesn't match 'vendor/jps2rin_arm' to which both 'git status' and 'git
branch -vv' refer, so that's where the difference is!
Here is actual branch configuration:
-- 8< --
$ git config --get-regexp branch[.]linux
branch.linux.remote vendor
branch.linux.merge jps2rin_arm
branch.linux.pushremote origin
branch.linux.rebase preserve
$ git remote -v
origin /var/local/group/firmware/git/jps2rin (fetch)
origin /var/local/group/firmware/git/jps2rin (push)
vendor ssh://git@git/gis/Justin2 (fetch)
vendor ssh://git@git/gis/Justin2 (push)
-- 8< --
So, finally, it's 'branch.linux.pushremote' that is the "offender".
Looks like both 'git status' and 'git branch -vv' should somehow learn
about 'branch.<name>.pushremote' feature so that their
output/suggestions make more sense?
By the way, is there a simpler/better way to print entire configuration
of a [current] branch? More human-readable? Including
"branch.<name>.description"?
$ git --version
git version 2.20.0.1.g8ad5d13
--
Sergey
^ permalink raw reply
* Re: [PATCH 3/8] entry: support CE_WT_REMOVE flag in checkout_entry
From: Duy Nguyen @ 2018-12-12 6:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Gummerer, Git Mailing List, Elijah Newren
In-Reply-To: <xmqqbm5sn6f6.fsf@gitster-ct.c.googlers.com>
On Tue, Dec 11, 2018 at 3:28 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> >> + if (ce->ce_flags & CE_WT_REMOVE) {
> >> + if (topath)
> >> + BUG("Can't remove entry to a path");
> >> + unlink_entry(ce);
> >> + return 0;
> >> + }
> >
> > This makes the path counting in nd/checkout-noisy less accurate. But
> > it's not your fault of course.
>
> When we check out absense of one path, how do we want to count it?
> Do we say "one path checked out?" when we remove one path?
It is still "checked out" according to this non-overlay concept.
Although we could make it clear by saying "5 paths updated, 2 deleted"
(but that may make us say "3 paths added" as well, hmm). Or maybe just
"%d paths updated" where updates include file creation and deletion.
--
Duy
^ permalink raw reply
* Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified
From: Sergey Organov @ 2018-12-12 5:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> When cherry-picking multiple commits, it's impossible to have both
>> merge- and non-merge commits on the same command-line. Not specifying
>> '-m 1' results in cherry-pick refusing to handle merge commits, while
>> specifying '-m 1' fails on non-merge commits.
>
[...]
> Now, it appears, at least to me, that the world pretty much accepted
> that the first-parent worldview is often very convenient and worth
> supporting by the tool, so the next logical step might be to set
> opts->mainline to 1 by default (and allow an explicit "-m $n" from
> the command line to override it). But that should happen after this
> patch lands---it is logically a separate step, I would think.
>
[...]
>> + } else if (1 < opts->mainline)
>> + /* Non-first parent explicitly specified as mainline for
>> + * non-merge commit */
>
> Style.
>
> /*
> * Our multi-line comments are to be
> * formatted like this.
> */
Comment style fixed:
-- 8< --
When cherry-picking multiple commits, it's impossible to have both
merge- and non-merge commits on the same command-line. Not specifying
'-m 1' results in cherry-pick refusing to handle merge commits, while
specifying '-m 1' fails on non-merge commits.
This patch allows '-m 1' for non-merge commits. Besides, as mainline is
always the only parent for a non-merge commit, it made little sense to
disable it in the first place.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
sequencer.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index e1a4dd1..d0fd61b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1766,9 +1766,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
return error(_("commit %s does not have parent %d"),
oid_to_hex(&commit->object.oid), opts->mainline);
parent = p->item;
- } else if (0 < opts->mainline)
- return error(_("mainline was specified but commit %s is not a merge."),
- oid_to_hex(&commit->object.oid));
+ } else if (1 < opts->mainline)
+ /*
+ * Non-first parent explicitly specified as mainline for
+ * non-merge commit
+ */
+ return error(_("commit %s does not have parent %d"),
+ oid_to_hex(&commit->object.oid), opts->mainline);
else
parent = commit->parents->item;
--
2.10.0.1.g57b01a3
^ permalink raw reply related
* Re: [PATCH v2 1/3] git clone <url> C:\cygwin\home\USER\repo' is working (again)
From: Elijah Newren @ 2018-12-12 3:47 UTC (permalink / raw)
To: svnpenn; +Cc: Torsten Bögershausen, Git Mailing List
In-Reply-To: <CAAXzdLVTjCVDmBik-j9B_Z_2wgSj=_6baqmjmGEGBFOsjkyOsw@mail.gmail.com>
On Fri, Dec 7, 2018 at 4:51 PM Steven Penny <svnpenn@gmail.com> wrote:
>
> On Fri, Dec 7, 2018 at 11:04 AM wrote:
> > The solution is to implement has_dos_drive_prefix(), skip_dos_drive_prefix()
> > is_dir_sep(), offset_1st_component() and convert_slashes() for cygwin
> > in the same way as it is done in 'Git for Windows' in compat/mingw.[ch]
> >
> > Instead of duplicating the code, it is extracted into compat/mingw-cygwin.[ch]
> > Some need for refactoring and cleanup came up in the review, they are adressed
> > in a seperate commit.
>
> i have applied the 3 patches against current master, and my original test
> passes, so looks good to me.
>
> however like Johannes i take issue with the naming. as he said "mingw-cygwin"
> really isnt appropriate. ideally it would be "windows.h", but as that is
> conspicuously in use, something like these:
>
> - pc-windows
> - pc-win
> - win
>
> i disagree with him on using "win32" - that doesnt really make sense, as
> obviously you can compile 64-bit Git for Windows. if you wanted to go that route
> you would want to use something like:
>
> - windows-api
> - win-api
> - winapi
>
> further - i disagree with the "DOS" moniker being used at all. DOS is a defunkt
> operating system that i dont think Git has *ever* supported, so it doesnt make
> sense to be referring to it this way. again, a more approriate name would be
> something like "win_drive_prefix".
You seem to want internal function and filenames to be based on the
product or marketing names of currently targetted systems. I don't
see why this is desirable; could you explain why it is?
I admit that I seem to see things more from Dscho's angle. However, I
know much less about Windows than either of you. Perhaps my best
understanding of the situation might help, limited as it is:
- Using currently targetted system names means future code churn --
we may have to rename functions and files for absolutely no useful
gain, muddying the history, making it harder for developers to
remember how to find things, etc., simply because an external party
renamed their libraries or introduced a new product.
- For people less familiar with windows,
"windows-api/win-api/winapi" and "win_drive_prefix" may sound like
something only available in newer systems, making them wonder if the
file name or function name is referring to some new windows concept
they are unfamiliar with
- Mentioning an anchor point where the relevant concept originally
targetted or where it came from (win32/path.c, dos_drive_prefix)
avoids or greatly reduces both problems.
I'm worried based on other emails in this thread that there is a
fundamental difference in frame of reference leading to a
misunderstanding about rationale for naming, and worse that folks
might not even realize where the misunderstanding is coming from,
attributing it to different motives rather than different frames of
reference. If that's true, I hope this email can begin the process of
clearing up the differences of understanding. If I'm wrong, then I
apologize for the noise; just ignore me.
Best wishes,
Elijah
^ permalink raw reply
* High locking contention during repack?
From: Iucha, Florin @ 2018-12-12 3:01 UTC (permalink / raw)
To: git@vger.kernel.org
Greetings,
I am running “git-repack -A -d -f -F --window=250 --depth=250” on a Git repository converted using git-svn.
The repository contains more than 10 years of development, and a mixture of source code and media assets. The size of the objects directory is around 50GB, and there are around 700k objects. There are several svn branches.
I have ran the repack with Git 2.17.1 (original package from Ubuntu 18.04), 2.19.1 (package from Ubuntu Git PPA) and with git next (bc1bbc6f855c3b5ef7fcbd0f688f647c4e5b208b).
The system is a 16 core / 32 thread Threadripper with 128GB of RAM and NVMe storage. The repack starts strong, with 32 threads but it fairly quickly gets to 99% done and the number of threads drops to 4 then 3 then 2. However, running “dstat 5” I see lots of “sys” time without any IO time (the network traffic you see is caused by SSH).
--total-cpu-usage-- -dsk/total- -net/total- ---paging-- ---system--
usr sys idl wai stl| read writ| recv send| in out | int csw
24 24 48 4 0| 0 0 | 362B 8392B| 0 0 | 64k 5042
27 25 44 4 0| 0 0 | 289B 4830B| 0 0 | 36k 5017
24 24 48 4 0| 0 0 | 362B 8392B| 0 0 | 64k 5042
27 25 44 4 0| 0 0 | 289B 4830B| 0 0 | 36k 5017
20 22 54 4 0| 0 0 | 330B 4826B| 0 0 | 31k 4657
22 23 51 3 0| 0 0 | 286B 4834B| 0 0 | 27k 4536
17 23 56 4 0| 0 0 | 299B 4820B| 0 0 | 29k 4099
24 23 49 4 0| 0 0 | 347B 4830B| 0 0 | 32k 4436
27 24 45 4 0| 0 0 | 292B 4820B| 0 0 | 22k 4513
26 27 44 3 0| 0 0 | 292B 4820B| 0 0 | 24k 4878
25 28 44 4 0| 0 0 | 329B 4837B| 0 0 | 45k 5023
23 30 44 3 0| 0 9830B| 370B 4836B| 0 0 | 40k 5064
23 26 47 4 0| 0 0 | 373B 4848B| 0 0 | 52k 4746
25 26 45 4 0| 0 0 | 323B 4833B| 0 0 | 31k 4874
24 28 45 3 0| 0 0 | 269B 4842B| 0 0 | 30k 4820
25 28 43 4 0| 0 0 | 280B 4826B| 0 0 | 45k 5237
25 29 43 3 0| 0 0 | 263B 4826B| 0 0 | 26k 5037
22 30 45 4 0| 0 0 | 287B 4823B| 0 0 | 45k 4154
0 53 42 5 0| 0 0 | 275B 1210B| 0 0 |5017 1331
0 53 42 5 0| 0 0 | 97B 0 | 0 0 |5006 1272
0 53 42 5 0| 0 0 | 70B 0 | 0 0 |5008 1272
0 53 42 5 0| 0 0 | 24B 0 | 0 0 |5007 1283
11 43 43 4 0| 0 0 | 644B 19k| 0 0 | 20k 2995
25 28 43 4 0| 0 123k| 304B 4802B| 0 0 | 29k 5118
25 27 44 4 0| 0 0 | 297B 4800B| 0 0 | 41k 5121
26 27 43 4 0| 0 0 | 320B 4786B| 0 0 | 29k 4973
22 30 44 3 0| 0 0 | 332B 4786B| 0 0 | 32k 4610
24 27 46 3 0| 0 0 | 296B 4786B| 0 0 | 56k 4632
35 17 44 3 0| 0 0 | 292B 4776B| 0 0 | 100k 4889
34 20 43 4 0| 0 9830B| 610B 4820B| 0 0 | 59k 4811
This when it gets to two threads phase:
1 40 59 0 0| 0 38k| 373B 2458B| 0 0 | 50k 2874
0 40 59 0 0| 0 0 | 268B 2892B| 0 0 | 10k 2858
0 40 59 0 0| 0 0 | 299B 3102B| 0 0 |8392 2919
1 40 59 0 0| 0 0 | 297B 3176B| 0 0 | 21k 2831
0 40 59 0 0| 0 0 | 281B 3414B| 0 0 |9192 2856
1 40 59 0 0| 0 0 | 264B 3493B| 0 0 | 17k 2905
1 40 59 0 0| 0 35k| 340B 3571B| 0 0 | 13k 2885
The repack still progresses, as I see the count of repacked objects being incremented every several minutes. But I suspect there is some terrible lock contention, because otherwise I can’t explain the high system usage with no IO.
Running a strace on the running git-repack process shows only these:
--- SIGALRM {si_signo=SIGALRM, si_code=SI_KERNEL} ---
--- SIGALRM {si_signo=SIGALRM, si_code=SI_KERNEL} ---
--- SIGALRM {si_signo=SIGALRM, si_code=SI_KERNEL} ---
--- SIGALRM {si_signo=SIGALRM, si_code=SI_KERNEL} ---
--- SIGALRM {si_signo=SIGALRM, si_code=SI_KERNEL} ---
--- SIGALRM {si_signo=SIGALRM, si_code=SI_KERNEL} ---
Any idea on how to debug this? I have ran git-repack under gdb, but it seems to spin on builtin/repack.c line 409.
Thank you for Git,
florin
^ permalink raw reply
* Re: [PATCH v2 1/3] git clone <url> C:\cygwin\home\USER\repo' is working (again)
From: Steven Penny @ 2018-12-12 0:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <nycvar.QRO.7.76.6.1812111429320.43@tvgsbejvaqbjf.bet>
On Tue, Dec 11, 2018 at 7:39 AM Johannes Schindelin wrote:
> I have no intention of flaming anybody. That is simply a
> misrepresentation.
you may see yourself "through a glass darkly", but i dont. this language is not
constructive:
> > - pc-windows
> > - pc-win
> > - win
>
> I find all of those horrible.
one windows triplet in use is "x86_64-pc-windows", used by Rust:
https://forge.rust-lang.org/other-installation-methods.html
which is how i came about my suggestions - again they arent great but they arent
misleading as "Win32" is.
> It is a long established convention to talk about the Win32 API as the set
> of functions developed for Windows NT and backported to Windows 95.
>
> There is no benefit in abandoning that convention just to please you.
Quoting from Wikipedia (emphasis mine):
> The **Windows API**, informally **WinAPI**, is Microsoft's core set of
> application programming interfaces (APIs) available in the Microsoft Windows
> operating systems. The name **Windows API** collectively refers to several
> different platform implementations that are often referred to by their own
> names (for example, **Win32** API)
and:
> Microsoft eventually changed the name of the then current **Win32** API family
> into **Windows API**, and made it into a catch-all term for both past and
> future API versions.
http://wikipedia.org/wiki/Windows_API
and quoting directly from Microsoft:
> The **Windows API** can be used in all Windows-based desktop applications, and
> the same functions are generally supported on 32-bit and 64-bit Windows.
http://docs.microsoft.com/windows/desktop/apiindex/api-index-portal
So again, "Win32" refers specifically to the old 32-bit only version of the API,
while:
- windows-api
- win-api
- winapi
refer to the current version.
> If you want to change something that has been in use for a long time, you
> have to have good reasons. None of your arguments convinces me so far that
> you have any good reason to change these.
i am certainly not interested in convincing you. i figured you wouldve gleaned
this from the fact that i removed you from the CC. Nevertheless, see above
links.
> If anyone truly cares about an issue to be fixed, I would expect more
> assisting, and less distracting, to do wonders.
hmm:
- http://public-inbox.org/git/CAAXzdLXSJU5bC_D1Q_gCWqKG7mcdcAvRkiYzano-VsrRRxazDQ@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLXmJ1YKiTF17b=ZfkM3HtJCNkvVMQNU=riW8R42VLid_Q@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLWByGC+B_XdDiJwounoPgMAsMq=EuOSx9bdV-f5vQUhnA@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLXCEeZdkCXT+-0n=Fn7_=Nz5cm+6xr0w-cd6B1om028uA@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLU3dsCabgYKnD9c7iWZcXx1cfO3tisJ7r0dNjiiTHk1mA@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLWBSD5coxqbyRN_d9B1e4AA-Q6VQ7iRo8BPuhBKDicMRQ@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLX4jU7+i1W1A_Q1LpPFa1D4FYVPW5rcMnqr_tDHEJn+tw@mail.gmail.com
- http://public-inbox.org/git/CAAXzdLWtDw09umyr23qZkv2jQ6_mTeFXbktgb-f6S2w6Zf1Egg@mail.gmail.com
^ permalink raw reply
* [PATCH 2/3] builtin/fetch-pack: support protocol version 2
From: Jonathan Tan @ 2018-12-12 0:27 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, avarab, peff
In-Reply-To: <cover.1544573604.git.jonathantanmy@google.com>
Currently, if support for running Git's entire test suite with protocol
v2 were added, some tests would fail because the fetch-pack CLI command
dies if it encounters protocol v2. To avoid this, teach fetch-pack
support for protocol v2.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
builtin/fetch-pack.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 63e69a5801..f6a513495e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -55,6 +55,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
struct oid_array shallow = OID_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
struct packet_reader reader;
+ enum protocol_version version;
fetch_if_missing = 0;
@@ -219,9 +220,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_GENTLE_ON_EOF);
- switch (discover_version(&reader)) {
+ version = discover_version(&reader);
+ switch (version) {
case protocol_v2:
- die("support for protocol v2 not implemented yet");
+ get_remote_refs(fd[1], &reader, &ref, 0, NULL, NULL);
+ break;
case protocol_v1:
case protocol_v0:
get_remote_heads(&reader, &ref, 0, NULL, &shallow);
@@ -231,7 +234,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
}
ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
- &shallow, pack_lockfile_ptr, protocol_v0);
+ &shallow, pack_lockfile_ptr, version);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related
* [PATCH 3/3] also squash this into your patch
From: Jonathan Tan @ 2018-12-12 0:27 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, avarab, peff
In-Reply-To: <cover.1544573604.git.jonathantanmy@google.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5500-fetch-pack.sh | 13 +++++++------
t/t5616-partial-clone.sh | 3 +--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 9c18875c9c..a5a8f348a2 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -41,8 +41,7 @@ pull_to_client () {
test_expect_success "$number pull" '
(
cd client &&
- GIT_TEST_PROTOCOL_VERSION=0 \
- git fetch-pack -k -v .. $heads &&
+ git fetch-pack -k -v .. $heads &&
case "$heads" in
*A*)
@@ -441,7 +440,6 @@ test_expect_success 'setup tests for the --stdin parameter' '
'
test_expect_success 'fetch refs from cmdline' '
- sane_unset GIT_TEST_PROTOCOL_VERSION &&
(
cd client &&
git fetch-pack --no-progress .. $(cat ../input)
@@ -630,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
test_commit -C server 6 &&
git init client &&
- test_must_fail git -C client fetch-pack ../server \
+
+ # Other protocol versions (e.g. 2) allow fetching an unadvertised
+ # object, so run this test with the default protocol version (0).
+ test_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION git -C client fetch-pack ../server \
$(git -C server rev-parse refs/heads/master^) 2>err &&
test_i18ngrep "Server does not allow request for unadvertised object" err
'
@@ -790,7 +791,7 @@ test_expect_success 'shallow clone exclude tag two' '
'
test_expect_success 'fetch exclude tag one' '
- git -C shallow12 fetch --shallow-exclude one origin &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION git -C shallow12 fetch --shallow-exclude one origin &&
git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
test_write_lines three two >expected &&
test_cmp expected actual
@@ -808,7 +809,7 @@ test_expect_success 'fetching deepen' '
git -C deepen log --pretty=tformat:%s master >actual &&
echo three >expected &&
test_cmp expected actual &&
- git -C deepen fetch --deepen=1 &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION git -C deepen fetch --deepen=1 &&
git -C deepen log --pretty=tformat:%s origin/master >actual &&
cat >expected <<-\EOF &&
four
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index feedf84ce1..336f02a41a 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -144,8 +144,7 @@ test_expect_success 'manual prefetch of missing objects' '
sort >observed.oids &&
test_line_count = 6 observed.oids &&
- GIT_TEST_PROTOCOL_VERSION=0 \
- git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
+ git -C pc1 fetch-pack --stdin "file://$(pwd)/srv.bare" <observed.oids &&
git -C pc1 rev-list --quiet --objects --missing=print \
master..origin/master >revs &&
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related
* [PATCH 1/3] squash this into your patch
From: Jonathan Tan @ 2018-12-12 0:27 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, avarab, peff
In-Reply-To: <cover.1544573604.git.jonathantanmy@google.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5539-fetch-http-shallow.sh | 12 ++++++++----
t/t5541-http-push-smart.sh | 10 ++++++++--
t/t5551-http-fetch-smart.sh | 23 +++++++++++++++--------
t/t5570-git-daemon.sh | 2 +-
4 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 5fbf67c446..996ce060cd 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -67,7 +67,8 @@ test_expect_success 'no shallow lines after receiving ACK ready' '
cd clone &&
git checkout --orphan newnew &&
test_commit new-too &&
- GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch --depth=2 &&
+ GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git fetch --depth=2 &&
grep "fetch-pack< ACK .* ready" ../trace &&
! grep "fetch-pack> done" ../trace
)
@@ -114,7 +115,8 @@ test_expect_success 'shallow clone exclude tag two' '
'
test_expect_success 'fetch exclude tag one' '
- git -C shallow12 fetch --shallow-exclude one origin &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git -C shallow12 fetch --shallow-exclude one origin &&
git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
test_write_lines three two >expected &&
test_cmp expected actual
@@ -128,14 +130,16 @@ test_expect_success 'fetching deepen' '
test_commit two &&
test_commit three &&
mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
- git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git clone --depth 1 $HTTPD_URL/smart/shallow-deepen.git deepen &&
mv "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" .git &&
test_commit four &&
git -C deepen log --pretty=tformat:%s master >actual &&
echo three >expected &&
test_cmp expected actual &&
mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-deepen.git" &&
- git -C deepen fetch --deepen=1 &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git -C deepen fetch --deepen=1 &&
git -C deepen log --pretty=tformat:%s origin/master >actual &&
cat >expected <<-\EOF &&
four
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 5475afc052..20f1308578 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -45,14 +45,20 @@ test_expect_success 'no empty path components' '
# In the URL, add a trailing slash, and see if git appends yet another
# slash.
cd "$ROOT_PATH" &&
- git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
+ # Other protocol versions may make different requests, so perform this
+ # clone with protocol v0.
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
check_access_log exp
'
test_expect_success 'clone remote repository' '
rm -rf test_repo_clone &&
- git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
+ # Other protocol versions may make different requests, so perform this
+ # clone with protocol v0.
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
(
cd test_repo_clone && git config push.default matching
)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc39..56fd9351a6 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -43,7 +43,8 @@ test_expect_success 'clone http repository' '
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Type: application/x-git-upload-pack-result
EOF
- GIT_TRACE_CURL=true git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
+ GIT_TRACE_CURL=true env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
test_cmp file clone/file &&
tr '\''\015'\'' Q <err |
sed -e "
@@ -92,7 +93,7 @@ test_expect_success 'fetch changes via http' '
echo content >>file &&
git commit -a -m two &&
git push public &&
- (cd clone && git pull) &&
+ (cd clone && env --unset=GIT_TEST_PROTOCOL_VERSION git pull) &&
test_cmp file clone/file
'
@@ -143,7 +144,8 @@ test_expect_success 'clone from auth-only-for-push repository' '
test_expect_success 'clone from auth-only-for-objects repository' '
echo two >expect &&
set_askpass user@host pass@host &&
- git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git clone --bare "$HTTPD_URL/auth-fetch/smart/repo.git" half-auth &&
expect_askpass both user@host &&
git --git-dir=half-auth log -1 --format=%s >actual &&
test_cmp expect actual
@@ -151,7 +153,8 @@ test_expect_success 'clone from auth-only-for-objects repository' '
test_expect_success 'no-op half-auth fetch does not require a password' '
set_askpass wrong &&
- git --git-dir=half-auth fetch &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git --git-dir=half-auth fetch &&
expect_askpass none
'
@@ -187,7 +190,8 @@ test_expect_success 'create namespaced refs' '
'
test_expect_success 'smart clone respects namespace' '
- git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git clone "$HTTPD_URL/smart_namespace/repo.git" ns-smart &&
echo namespaced >expect &&
git --git-dir=ns-smart/.git log -1 --format=%s >actual &&
test_cmp expect actual
@@ -214,7 +218,8 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
EOF
git config http.cookiefile cookies.txt &&
git config http.savecookies true &&
- git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
+ env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
tail -3 cookies.txt | sort >cookies_tail.txt &&
test_cmp expect_cookies.txt cookies_tail.txt
'
@@ -306,7 +311,8 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
git init --bare test_reachable.git &&
git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
- test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+ test_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
'
test_expect_success 'test allowanysha1inwant with unreachable' '
@@ -325,7 +331,8 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
git init --bare test_reachable.git &&
git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
- test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
+ test_must_fail env --unset=GIT_TEST_PROTOCOL_VERSION \
+ git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
git -C "$server" config uploadpack.allowanysha1inwant 1 &&
git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 7466aad111..4a2a937bb0 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -190,7 +190,7 @@ test_expect_success 'daemon log records all attributes' '
EOF
>daemon.log &&
GIT_OVERRIDE_VIRTUAL_HOST=localhost \
- git -c protocol.version=1 \
+ env --unset=GIT_TEST_PROTOCOL_VERSION git -c protocol.version=1 \
ls-remote "$GIT_DAEMON_URL/interp.git" &&
grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
test_cmp expect actual
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply related
* [PATCH 0/3] Some fixes and improvements
From: Jonathan Tan @ 2018-12-12 0:27 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, avarab, peff
In-Reply-To: <20181211212135.21126-2-avarab@gmail.com>
Thanks, Aevar for looking into this. I haven't looked into detail, but:
- s/where/var/ in your patch 1
- I think that variables should be unset with env --unset instead of
sane_unset, because (as far as I can tell) the effects of sane_unset
"leak" from test block to test block.
- On my computer, some tests still fail. I have included a patch (patch
1) that makes these tests succeed.
- I noticed that some of the tests fail because fetch-pack doesn't
support protocol v2. It turns out that adding support for protocol v2
in fetch-pack wasn't too difficult, so I have done it. This is patch
2.
- With that support added, some of the other tests no longer need the
unsetting of the environment variable. This is patch 3.
If you agree with the general direction I'm going in, when you send out
another version, I would add patch 2 somewhere near the beginning of the
set, and then squash both patch 1 and patch 3 in the G_T_P_V=2 patch.
Jonathan Tan (3):
squash this into your patch
builtin/fetch-pack: support protocol version 2
also squash this into your patch
builtin/fetch-pack.c | 9 ++++++---
t/t5500-fetch-pack.sh | 13 +++++++------
t/t5539-fetch-http-shallow.sh | 12 ++++++++----
t/t5541-http-push-smart.sh | 10 ++++++++--
t/t5551-http-fetch-smart.sh | 23 +++++++++++++++--------
t/t5570-git-daemon.sh | 2 +-
t/t5616-partial-clone.sh | 3 +--
7 files changed, 46 insertions(+), 26 deletions(-)
--
2.19.0.271.gfe8321ec05.dirty
^ permalink raw reply
* [PATCH v3 4/4] lib-httpd, t5551: check server-side HTTP errors
From: Josh Steadmon @ 2018-12-12 0:25 UTC (permalink / raw)
To: git; +Cc: peff, gitster, masayasuzuki
In-Reply-To: <cover.1544572142.git.steadmon@google.com>
Add an HTTP path to the test server config that returns an ERR pkt-line
unconditionally. Verify in t5551 that remote-curl properly detects this
ERR message and aborts.
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf | 4 ++++
t/lib-httpd/error-smart-http.sh | 3 +++
t/t5551-http-fetch-smart.sh | 5 +++++
4 files changed, 13 insertions(+)
create mode 100644 t/lib-httpd/error-smart-http.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..4e946623bb 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -131,6 +131,7 @@ prepare_httpd() {
mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
+ install_script error-smart-http.sh
install_script error.sh
install_script apply-one-time-sed.sh
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..6de2bc775c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/
</LocationMatch>
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
+ScriptAlias /error_smart/ error-smart-http.sh/
ScriptAlias /error/ error.sh/
ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
<Directory ${GIT_EXEC_PATH}>
@@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1
<Files broken-smart-http.sh>
Options ExecCGI
</Files>
+<Files error-smart-http.sh>
+ Options ExecCGI
+</Files>
<Files error.sh>
Options ExecCGI
</Files>
diff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh
new file mode 100644
index 0000000000..e65d447fc4
--- /dev/null
+++ b/t/lib-httpd/error-smart-http.sh
@@ -0,0 +1,3 @@
+echo "Content-Type: application/x-git-upload-pack-advertisement"
+echo
+printf "%s" "0019ERR server-side error"
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 8630b0cc39..ba83e567e5 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '
! grep "=> Send data" err
'
+test_expect_success 'server-side error detected' '
+ test_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&
+ grep "server-side error" actual
+'
+
stop_httpd
test_done
--
2.20.0.rc2.403.gdbc3b29805-goog
^ permalink raw reply related
* [PATCH v3 3/4] remote-curl: tighten "version 2" check for smart-http
From: Josh Steadmon @ 2018-12-12 0:25 UTC (permalink / raw)
To: git; +Cc: peff, gitster, masayasuzuki
In-Reply-To: <cover.1544572142.git.steadmon@google.com>
From: Jeff King <peff@peff.net>
In a v2 smart-http conversation, the server should reply to our initial
request with a pkt-line saying "version 2" (this is the start of the
"capabilities advertisement"). We check for the string using
starts_with(), but that's overly permissive (it would match "version
20", for example).
Let's tighten this check to use strcmp(). Note that we don't need to
worry about a trailing newline here, because the ptk-line code will have
chomped it for us already.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
remote-curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index 38f51dffb8..b77b173f33 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const char *service,
d->len = src_len;
d->proto_git = 1;
- } else if (starts_with(line, "version 2")) {
+ } else if (!strcmp(line, "version 2")) {
/*
* v2 smart http; do not consume version packet, which will
* be handled elsewhere.
--
2.20.0.rc2.403.gdbc3b29805-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox