* Re: [BUG] git help -a should no use page when in pipe
From: Junio C Hamano @ 2018-12-11 8:52 UTC (permalink / raw)
To: 许铖; +Cc: git
In-Reply-To: <CALmyJ=OaZAgr1i1DXMRQqCVWnKvO=QZH6AeGj8YfKMxAnRz6GA@mail.gmail.com>
许铖 <thexuc@gmail.com> writes:
> Since git 2.20.0, invoking `git help -a` will cause git using page (e.g. less)
> to display help message.
> ...
> I would suggest `git help -a` only invokes page when stdout is a tty.
It already does so for me. "GIT_PAGER=uc git help -a | cat" where
"uc" is a custom "pager" that is "tr '[a-z]' '[A-Z]'" does not show
things in ucase, but without piping to cat, the output is in ucase
as expected. Perhaps there is something different between what we
are doing.
FWIW, I am not on Windows or on a Mac.
^ permalink raw reply
* Re: Retrieving a file in git that was deleted and committed
From: Jeff King @ 2018-12-11 9:46 UTC (permalink / raw)
To: Elijah Newren; +Cc: biswaranjan.nitrkl, Bryan Turner, Git Mailing List
In-Reply-To: <CABPp-BHjR7Wq-D9tFMyPHZE1ogL5udOt8ri1rN3E1CasfD-2PQ@mail.gmail.com>
On Mon, Dec 10, 2018 at 01:33:18PM -0800, Elijah Newren wrote:
> Hmm...sure, if the file is deleted on the only relevant branch through
> history...but what if there were another branch where it weren't
> deleted? What does git blame do then?
>
> In other words, do NOT restore the file as biswaranjan suggested, but
> instead restore it this way[1]:
>
> git checkout -b keep-foo $REVISION_BEFORE_FOO_DELETED
> git commit --allow-empty -m "We want to keep foo"
> git checkout A
> git merge --no-commit keep-foo
> git checkout keep-foo -- foo.txt
> git commit
>
>
> Now, when you run
> git blame foo.txt
>
> blame should notice that foo.txt didn't exist in the first parent
> history on A, so it won't bother walking it to find that at some point
> foo.txt did exist there. Instead, it'll walk down the second parent
> and follow its history, where it should keep walking back and show all
> the old changes...right? Or did I mess up my testcase and
> misunderstand something somehow?
Yeah, I think that should work, and is a clever way of representing in
the actual history graph what you're we're trying to express. And it
shouldn't have any real downsides.
-Peff
^ permalink raw reply
* Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
From: Jeff King @ 2018-12-11 9:50 UTC (permalink / raw)
To: Junio C Hamano, git, stolee, avarab
In-Reply-To: <20181210215649.GC37614@google.com>
On Mon, Dec 10, 2018 at 01:56:49PM -0800, Josh Steadmon wrote:
> > "stat(1)" is not so portable, so you'll get complaints from minority
> > platform users later. So is "truncate(1)".
>
> Ack, thanks for the catch. I have a workaround for stat in the form of
> "wc -c", and for truncate with a combination of dd and /dev/zero.
> However, I'm finding conflicting information about whether or not
> /dev/zero exists on macOS. At the least, it sounds like it might not
> work on very old versions. Would this be acceptable, or should I add a
> new test function to do this?
If you're just interested in truncation (and not a bunch of zero bytes),
you can dd from /dev/null.
Another way to truncate is to move the file elsewhere, and then copy
only some of the bytes back (see t1450's "fsck detects truncated loose
object").
-Peff
^ permalink raw reply
* Re: silent_exec_failure when calling gpg
From: Jeff King @ 2018-12-11 9:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Passaro, git
In-Reply-To: <xmqqh8fkln5q.fsf@gitster-ct.c.googlers.com>
On Tue, Dec 11, 2018 at 01:09:37PM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > John Passaro <john.a.passaro@gmail.com> writes:
> >
> >> I've noticed that in v2.19.1, when using git to pretty print
> >> information about the signature, if git cannot find gpg (e.g. "git
> >> config gpg.program nogpg"), it prints an error to stderr:
> >>
> >> $ git show -s --pretty=%G?
> >> fatal: cannot run nogpg: No such file or directory
> >> N
> >
> > I think the uninteded behaviour change was in 17809a98 ("Merge
> > branch 'jk/run-command-notdot'", 2018-10-30).
>
> Perhaps something like this. There needs an additional test added
> for this codepath, which I haven't done yet, though.
Thanks, both, for the report and the patch.
> diff --git a/run-command.c b/run-command.c
> index d679cc267c..e2bc18a083 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -728,6 +728,8 @@ int start_command(struct child_process *cmd)
> if (prepare_cmd(&argv, cmd) < 0) {
> failed_errno = errno;
> cmd->pid = -1;
> + if (!cmd->silent_exec_failure)
> + error_errno("cannot run %s", cmd->argv[0]);
> goto end_of_spawn;
> }
Yes, I think this is the right fix. For a test, I think we could just
do:
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index cf932c8514..866268dfd1 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -33,7 +33,8 @@ test_expect_success 'run_command is restricted to PATH' '
write_script should-not-run <<-\EOF &&
echo yikes
EOF
- test_must_fail test-tool run-command run-command should-not-run
+ test_must_fail test-tool run-command run-command should-not-run 2>err &&
+ grep should-not-run err
'
test_expect_success !MINGW 'run_command can run a script without a #! line' '
I assume you'll wrap that up into a real commit?
-Peff
^ permalink raw reply related
* Re: pw/add-p-select, was Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Johannes Schindelin @ 2018-12-11 9:56 UTC (permalink / raw)
To: phillip.wood; +Cc: Slavica Djukic, Junio C Hamano, git
In-Reply-To: <d69259ce-bc45-9201-693b-3902a6379265@talktalk.net>
Hi Phillip,
[Cc:ing Slavica, the Outreachy intern working on converting `add -i` to a
built-in]
On Mon, 10 Dec 2018, Phillip Wood wrote:
> On 09/12/2018 20:31, Johannes Schindelin wrote:
> >
> > On Sun, 9 Dec 2018, Junio C Hamano wrote:
> >
> > > * pw/add-p-select (2018-07-26) 4 commits
> > > - add -p: optimize line selection for short hunks
> > > - add -p: allow line selection to be inverted
> > > - add -p: select modified lines correctly
> > > - add -p: select individual hunk lines
> > >
> > > "git add -p" interactive interface learned to let users choose
> > > individual added/removed lines to be used in the operation, instead
> > > of accepting or rejecting a whole hunk.
> > >
> > > Will discard.
> > > No further feedbacks on the topic for quite some time.
> >
> > That is not quite true. I did comment that this feature
>
> Sorry I meant to reply to that comment but never got round to it.
No worries. We're all busy down here.
> > (which I take as being inspired by Git GUI's "Stage Selected Line"),
>
> not particularly, I don't use git gui I just wanted a way to easily
> stage a subset of lines without editing the hunk.
Okay. I used to use Git GUI quite a bit to stage individual lines, but
recently I tried to stay more in the terminal and used the `split` and
`edit` commands of `add -p` quite extensively. Wishing for an quicker way
to stage individual lines between all of my debug print statements.
> > and thought that it would be useful.
> >
> > I could imagine, however, that it would make sense for `git add -p` to
> > imitate that feature more closely: by allowing to stage a single line
> > and then presenting the current hunk (re-computed) again.
>
> that sounds like it would be quite tedious to stage more than a couple
> of lines,
It would be. But then, if you want to do anything slightly more
complicated than staging a hunk or a line, I personally prefer the `edit`
command *a lot*, as it lets me even split unrelated changes in the same
line into two commits.
> and it could be awkward to get it to stage modified lines correctly
> (While I was writing the feature I tried git gui, I think it is supposed
> to be able to stage modified lines correctly but I never persuaded it to
> do it for me. I also tried gitg, tig and hg commit -i but I couldn't get
> them to do modified lines either)
Git GUI works very reliably for me, but then, I have Git for Windows'
patched Git GUI at my finger tips (oh how I wish we had a Git GUI
maintainer again).
It should not be awkward to stage a single modified line at all.
Essentially, you take the hunk, strip out all `-` and `+` lines except the
one you want to stage, then stage that with `git apply --cached
--recount`, and then you simply re-generate that hunk.
> I'll try and re-roll in the New Year, when does the outreachy project
> for converting add -i start? - it would be good to try and get this
> pinned down before then.
Too late. Slavica started on December 4th, and you can even read about it
on their blog: https://slavicadj.github.io/blog/
Ciao,
Dscho
^ permalink raw reply
* Re: Git clone fails with fatal: the remote end hung up unexpectedly
From: Jeff King @ 2018-12-11 10:08 UTC (permalink / raw)
To: Owen Ofiesh; +Cc: Bryan Turner, Git Users
In-Reply-To: <CY4PR19MB152668FA1C3E592CA123E3E5ADA60@CY4PR19MB1526.namprd19.prod.outlook.com>
On Tue, Dec 11, 2018 at 02:25:41AM +0000, Owen Ofiesh wrote:
> > On Mon, Dec 10, 2018 at 4:55 PM Owen Ofiesh <mailto:oofiesh@maxlinear.com> wrote:
> > >
> > > We are seeing an issue where git clone in Linux Ubuntu 14.04.5 LTS fails
> > > with the following error using the HTTP protocol.
> > >
> > > The error on the client is:
> > > fatal: the remote end hung up unexpectedly
> > > fatal: early EOF
> > > fatal: index-pack failed
> > >
> > > The client is writing to an NFS volume.
> >
> > A further detail on this (Owen correct me if I'm wrong), but the same
> > clone performed to a local disk, rather than NFS, succeeds.
>
> Correct. The clone to local disk succeeds.
That's weird. The messages imply that the server has started sending the
packfile, at which point we should be pumping data from git-remote-https
to fetch-pack, and from there into an index-pack process. And the
messages imply that the client saw a hangup of the network connection,
at which point index-pack complained of the truncated packfile (and then
we complained that index-pack complained).
But what's weird is that none of that should really be relevant to the
choice of local filesystem. Is it possible that using NFS stresses the
network to the point that the HTTP connection may be killed?
It's also possible there's something racy in Git's handling of the data,
and NFS is slower to write. What version of Git is this?
> One more data point. I tried this with SSH just now, and the behavior
> is somewhat improved in that I am only seeing the failure on the NFS
> volume so far with 1 in 6 clone attempts (5 successful). Whereas with
> HTTP, we see the failure every time.
If you use ssh's verbose mode, what does a failing case look like? If
you have Git v2.10 or later, you can just do:
GIT_SSH_COMMAND='ssh -v' git clone ...
If it's older, the simplest thing is probably to put:
Host yourserver...
LogLevel Verbose
into your ~/.ssh/config.
I know the problem is more common with HTTP, but I suspect ssh's logging
may be better than ours. ;) You can also try:
GIT_CURL_VERBOSE=1 git clone ...
or if you have recent enough (Git v2.10 again, I think), you can use:
GIT_TRACE_CURL=1 git clone ...
which has a few more details.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] rebase: introduce --reschedule-failed-exec
From: Johannes Schindelin @ 2018-12-11 10:14 UTC (permalink / raw)
To: Elijah Newren
Cc: Johannes Schindelin via GitGitGadget, Git Mailing List,
Junio C Hamano
In-Reply-To: <CABPp-BGoJxKFFu+JE9n52t8Fygzf0+mpPSOo8rftR2v0_i+eZw@mail.gmail.com>
Hi Elijah,
On Mon, 10 Dec 2018, Elijah Newren wrote:
> On Mon, Dec 10, 2018 at 1:18 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > @@ -1195,6 +1201,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> > break;
> > }
> >
> > + if (options.reschedule_failed_exec && !is_interactive(&options))
> > + die(_("--reschedule-failed-exec requires an interactive rebase"));
> > +
>
> I was surprised at first that you checked is_interactive() rather than
> checking for --exec being specified. But I guess this is because users
> can manually specify 'exec' lines.
Indeed, that is exactly the reason.
> What if the user specifies an implicitly interactive rebase (i.e. no
> editing of the todo list, such as with --rebase-merges or
> --keep-empty, or soon --strategy or --strategy-option) and also
> doesn't specify --exec?
Then the todo list won't have any `exec` lines, and the flag is irrelevant
(but does not do any harm).
... except in the case that the rebase fails at some stage, the user edits
the todo list with `git rebase --edit-todo` and inserts an `exec` line.
So I would contend that it still makes sense to allow that flag in those
cases, i.e. whenever the user asked for the interactive backend.
> > @@ -534,6 +545,9 @@ then
> > # git-rebase.txt caveats with "unless you know what you are doing"
> > test -n "$rebase_merges" &&
> > die "$(gettext "error: cannot combine '--preserve-merges' with '--rebase-merges'")"
> > +
> > + test -n "$reschedule_failed_exec" &&
> > + die "$(gettext "error: cannot combine '--preserve-merges' with '--reschedule-failed-exec'")"
> > fi
> >
> > if test -n "$rebase_merges"
>
> In the builtin rebase, you checked that --reschedule-failed-exec had
> to be used with an interactive rebase. Here in the legacy rebase you
> have no such check at all.
>
> Not sure if that's an oversight, or if we're at the point where we
> just start intentionally allowing legacy rebase to lag and soon throw
> it out. (When do we get to that point?)
Good point. My thinking was that the legacy rebase does not matter all
that much anymore, I would expect that we get rid of it in v2.21.0.
But you're right, I should not intentionally diverge the functionality out
of sheer laziness.
Will fix.
> The rest of the patch looks good to me.
Thanks!
Dscho
^ permalink raw reply
* Re: Difficulty with parsing colorized diff output
From: Jeff King @ 2018-12-11 10:17 UTC (permalink / raw)
To: Stefan Beller; +Cc: george.w.king, git
In-Reply-To: <CAGZ79kbd=2_eHdbVYwmNoAYupwnP3YDn6nT0m=v1CL0AkWXk=Q@mail.gmail.com>
On Mon, Dec 10, 2018 at 07:26:46PM -0800, Stefan Beller wrote:
> > Context lines do have both. It's just that the default color for context
> > lines is empty. ;)
>
> The content itself can contain color codes.
>
> Instead of unconditionally resetting each line, we could parse each
> content line to determine if we actually have to reset the colors.
Good point. I don't recall that being the motivation back when this
behavior started, but it's a nice side effect (and the more recent line
you mentioned in emit_line_0 certainly is doing it intentionally).
That doesn't cover _other_ terminal codes, which could also make for
confusing output, but I do think color codes are somewhat special. We
generally send patches through "less -R", which will pass through the
colors but show escaped versions of other codes.
> Another idea would be to allow Git to output its output
> as if it was run through test_decode_color, slightly related:
> https://public-inbox.org/git/20180804015317.182683-8-sbeller@google.com/
> i.e. we'd markup the output instead of coloring it.
Yeah, I think in the most general form, the problem is that colorizing
(including whitespace highlighting) loses information within a single
line. It would be nice to have a machine-readable format that represents
all the various annotations (like whitespace and coloring moved bits)
that Git computes.
-Peff
^ permalink raw reply
* email lags, was Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically
From: Johannes Schindelin @ 2018-12-11 10:19 UTC (permalink / raw)
To: Elijah Newren
Cc: Johannes Schindelin via GitGitGadget, Git Mailing List,
Junio C Hamano
In-Reply-To: <CABPp-BGeDA=Cm6MYkrCK=pN94y9AKRHgknjyXN1oMfnSsTCnzw@mail.gmail.com>
Hi Elijah,
On Mon, 10 Dec 2018, Elijah Newren wrote:
> On Mon, Dec 10, 2018 at 3:13 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > The idea was brought up by Paul Morelle.
> >
> > To be honest, this idea of rescheduling a failed exec makes so much sense
> > that I wish we had done this from the get-go.
> >
> > So let's do the next best thing: implement a command-line option and a
> > config setting to make it so.
> >
> > The obvious intent behind that config setting is to not only give users a
> > way to opt into that behavior already, but also to make it possible to flip
> > the default at a later date, after the feature has been battle-tested and
> > after deprecating the non-rescheduling behavior for a couple of Git
> > versions.
> >
> > If the team agrees with my reasoning, then the 3rd patch (introducing -y
> > <cmd> as a shortcut for --reschedule-failed-exec -x <cmd>) might not make
> > much sense, as it would introduce a short option that would become obsolete
> > soon.
> >
>
> Complete side-track: This email showed up for me just five minutes
> ago, whereas the rest of the series showed up four hours ago, making
> me think this email had disappeared and trying to figure out how to
> respond when I didn't have the original. Any ideas why there might be
> that level of lag?
I have such email woes for roughly half a year now. No idea where this
comes from, whether this is some graylisting at work, or whether the
`<author> via GitGitGadget` marks gitgitgadget@gmail.com as suspect with
some mail providers and/or central lists of dubious email addresses.
At first, I thought it was only GMX, but yes, I also see it with GMail
now.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] run-command: report exec failure
From: Jeff King @ 2018-12-11 10:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Passaro
In-Reply-To: <xmqqd0q8liow.fsf@gitster-ct.c.googlers.com>
On Tue, Dec 11, 2018 at 02:46:07PM +0900, Junio C Hamano wrote:
> In 321fd823 ("run-command: mark path lookup errors with ENOENT",
> 2018-10-24), we rewrote the logic to execute a command by looking
> in the directories on $PATH; as a side effect, a request to run a
> command that is not found on $PATH is noticed even before a child
> process is forked to execute it.
>
> We however stopped to report an exec failure in such a case by
> mistake. Add a logic to report the error unless silent-exec-failure
> is requested, to match the original code.
>
> Reported-by: John Passaro <john.a.passaro@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ah, thanks, I didn't see this before writing my other message. The
commit message and fix look good to me.
> * Strictly speaking, the failure that is diagnosed by the spawned
> child is reported with die() and prefixed with "failure:"; I am
> adding error_errno(), so this will be reported with "error:"
> prefix, which is a slight change in behaviour, but I am guessing
> that this should be OK.
Yes, IMHO that's fine. Arguably the in-child version should say
"error:", too, as the fact that there is a second process is purely an
implementation detail (and not even true on Windows, or if we were to
start using posix_spawn).
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index cf932c8514..9c83d44d9c 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,11 +13,13 @@ cat >hello-script <<-EOF
> EOF
>
> test_expect_success 'start_command reports ENOENT (slash)' '
> - test-tool run-command start-command-ENOENT ./does-not-exist
> + test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
> + test_i18ngrep "cannot run" err
> '
This one is already correct before the patch, but I agree it's a good
idea to test it. Here (and in the others), grepping for "does-not-exist"
would be slightly more robust against us later changing the error
message, but it's probably not a big deal in practice.
Thanks again for a quick fix for my bug.
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] rebase: offer to reschedule failed exec commands automatically
From: Johannes Schindelin @ 2018-12-11 10:31 UTC (permalink / raw)
To: Stefan Beller; +Cc: Johannes Sixt, gitgitgadget, git, Junio C Hamano
In-Reply-To: <CAGZ79kY18SCaCBvkWyeVd+oeJ4EhoJK4=QxGhJ9a77iX2sR8ew@mail.gmail.com>
Hi Stefan,
On Mon, 10 Dec 2018, Stefan Beller wrote:
> On Mon, Dec 10, 2018 at 2:08 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >
> > Am 10.12.18 um 20:04 schrieb Johannes Schindelin via GitGitGadget:
> > > The idea was brought up by Paul Morelle.
> > >
> > > To be honest, this idea of rescheduling a failed exec makes so much sense
> > > that I wish we had done this from the get-go.
> >
> > The status quo was actually not that bad a decision, because it made 'x
> > false' as a substitute for 'break' very convenient.
> >
> > But now that we have a real 'break', I'm very much in favor of flipping
> > the behavior over to rescheduling. (I'm actually not a user of the
> > feature, but the proposed behavior is so compellingly logical.)
>
> In rare cases I had commands that may be dangerous if rerun,
> but I'd just not run them with -y but with -x.
Please note that 3/3 (i.e. the `-y` option) is *really* only intended as a
"I could do this if anybody *really* wanted to" patch. I actually would
strongly prefer not to have this patch in git.git at all.
> That brings me to some confusion I had in the last patch:
> To catch a flaky test I surely would be tempted to
> git rebase -x make -y "make test"
> but that might reschedule a compile failure as well,
> as a single -y has implications on all other -x's.
>
> I wonder if it might be better to push this mechanism
> one layer down: Instead of having a flag that changes
> the behavior of the "exec" instructions and having a
> handy '-y' short cut for the new mode, we'd rather have
> a new type of command that executes&retries a command
> ("exnrt", 'n'), which can still get the '-y' command line flag,
> but more importantly by having 2 separate sets of
> commands we'd have one set that is a one-shot, and the
> other that is retried. Then we can teach the user which
> is safe and which isn't for rescheduling.
>
> By having two classes, I would anticipate fewer compatibility
> issues ('"Exec" behaves differently, and I forgot I had turned
> on the rescheduling').
As Junio points out, this brings us back to the proposal to have two
different `exec` commands.
I am really unenthusiastic about this idea, as I find it "hard to sell":
what little benefit it would bring to have two commands that pretty much
do the same thing almost all the time would be outweighed *by far* by the
confusion we would sow by that.
It is amazing to me how much my perspective changed when I actually had to
teach Git to new users. Things that I live with easily all of a sudden
become these unnecessarily confusing road blocks that make it *so hard* to
actually use Git.
> Talking about rescheduling: In the above example the flaky
> test can flake more than once, so I'd be stuck with keeping
> 'git rebase --continue'ing after I see the test flaked once again.
No, you would not be stuck with that.
You would read the error message carefully (we do that all the time, yes?)
and then run `git rebase --edit-todo` to remove that line before
continuing.
:-)
If you feel very strongly about this, we could introduce a new option for
`git rebase --skip`, say, `git rebase --skip --skip-next-commands-too=1`.
(I specifically used a too-long option name, as you and I are both
Germans, known to use too-long names for everything. I do not really
intend to use such an awkward option name, if we decide that such an
option would be a good idea. Probably more like `git rebase
--skip-next[=<N>]`.)
> My workflow with interactive rebase and fixing up things as I go
> always involves a manual final "make test" to check "for real",
> which I could lose now, which is nice.
My workflow with `git rebase -r --exec "make test"` is pretty similar to
yours. With the difference that I would want those commands to be
rescheduled *even if* the command is flakey. Actually, *in particular*
when it is flakey.
I really see myself calling
git config --global rebase.rescheduleFailedExec true
in all my setups.
Ciao,
Dscho
^ permalink raw reply
* [PATCH 0/3] protocol v2 and hidden refs
From: Jeff King @ 2018-12-11 10:42 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, Jonathan Tan
When using the v2 protocol, hidden-ref config is not respected at all:
$ git config transfer.hiderefs refs/tags/
$ git -c protocol.version=0 ls-remote . | grep -c refs/tags
0
$ git -c protocol.version=2 ls-remote . | grep -c refs/tags
1424
The fix in patch 3 is pretty straightforward, but note:
- I'm a little worried this may happen again with future features. The
root cause is that "ls-refs" follows a different code path than the
ref advertisement for "upload-pack". So if we add any new config,
it needs to go both places (non ref-advertisement config is OK, as
the v2 "fetch" command is a lot closer to a v0 upload-pack).
I think this is just an issue for any future features. I looked for
other existing features which might be missing in v2, but couldn't
find any.
I don't know if there's a good solution. I tried running the whole
test suite with v2 as the default. It does find this bug, but it has
a bunch of other problems (notably fetch-pack won't run as v2, but
some other tests I think also depend on v0's reachability rules,
which v2 is documented not to enforce).
- The "serve" command is funky, because it has no concept of whether
the "ls-refs" is for fetching or pushing. Is git-serve even a thing
that we want to support going forward? I know part of the original
v2 conception was that one would be able to just connect to
"git-serve" and do a number of operations. But in practice the v2
probing requires saying "I'd like to git-upload-pack, and v2 if you
please". So no client ever calls git-serve.
Is this something we plan to eventually move to? Or can it be
considered a funny vestige of the development? In the latter case, I
think we should consider removing it.
I've worked around it here with patch 2, but note that "git serve"
would not ever respect uploadpack.hiderefs nor receive.hiderefs
(since it has no idea which operation it's doing).
The patches are:
[1/3]: serve: pass "config context" through to individual commands
[2/3]: parse_hide_refs_config: handle NULL section
[3/3]: upload-pack: support hidden refs with protocol v2
builtin/upload-pack.c | 1 +
ls-refs.c | 16 +++++++++++++++-
ls-refs.h | 3 ++-
refs.c | 3 ++-
serve.c | 9 +++++----
serve.h | 7 +++++++
t/t5512-ls-remote.sh | 6 ++++++
upload-pack.c | 4 ++--
upload-pack.h | 4 ++--
9 files changed, 42 insertions(+), 11 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/3] serve: pass "config context" through to individual commands
From: Jeff King @ 2018-12-11 10:43 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, Jonathan Tan
In-Reply-To: <20181211104236.GA6899@sigill.intra.peff.net>
In protocol v2, instead of just running "upload-pack", we have a generic
"serve" loop which runs command requests from the client. What used to
be "upload-pack" is now generally split into two operations: "ls-refs"
and "fetch". The latter knows it must respect uploadpack.* config, but
the former is not actually specific to a fetch operation (we do not yet
do v2 receive-pack, but eventually we may, and ls-refs would support
both operations).
However, ls-refs does need to know which operation we're performing, in
order to read the correct config (e.g., uploadpack.hiderefs versus
receive.hiderefs; we don't read _either_ right now, but this is the
first step to fixing that).
In the generic "git-serve" program, we don't have that information. But
in the protocol as it is actually used by clients, the client still asks
to run "git-upload-pack", and then issues an "ls-refs" from there. So we
_do_ know at that point that "uploadpack" is the right config context.
This patch teaches the protocol v2 "serve" code to pass that context
through to the individual commands (a future patch will teach ls-refs to
actually use it).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/upload-pack.c | 1 +
ls-refs.c | 4 +++-
ls-refs.h | 3 ++-
serve.c | 9 +++++----
serve.h | 7 +++++++
upload-pack.c | 4 ++--
upload-pack.h | 4 ++--
7 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 42dc4da5a1..67192cfa9e 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -52,6 +52,7 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix)
case protocol_v2:
serve_opts.advertise_capabilities = opts.advertise_refs;
serve_opts.stateless_rpc = opts.stateless_rpc;
+ serve_opts.config_context = "uploadpack";
serve(&serve_opts);
break;
case protocol_v1:
diff --git a/ls-refs.c b/ls-refs.c
index a06f12eca8..e8e31475f0 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -69,7 +69,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
-int ls_refs(struct repository *r, struct argv_array *keys,
+int ls_refs(struct repository *r,
+ const char *config_section,
+ struct argv_array *keys,
struct packet_reader *request)
{
struct ls_refs_data data;
diff --git a/ls-refs.h b/ls-refs.h
index b62877e8da..da26fc9824 100644
--- a/ls-refs.h
+++ b/ls-refs.h
@@ -4,7 +4,8 @@
struct repository;
struct argv_array;
struct packet_reader;
-extern int ls_refs(struct repository *r, struct argv_array *keys,
+extern int ls_refs(struct repository *r, const char *config_context,
+ struct argv_array *keys,
struct packet_reader *request);
#endif /* LS_REFS_H */
diff --git a/serve.c b/serve.c
index bda085f09c..70f89cf0d9 100644
--- a/serve.c
+++ b/serve.c
@@ -48,6 +48,7 @@ struct protocol_capability {
* This field should be NULL for capabilities which are not commands.
*/
int (*command)(struct repository *r,
+ const char *config_context,
struct argv_array *keys,
struct packet_reader *request);
};
@@ -158,7 +159,7 @@ enum request_state {
PROCESS_REQUEST_DONE,
};
-static int process_request(void)
+static int process_request(struct serve_options *opts)
{
enum request_state state = PROCESS_REQUEST_KEYS;
struct packet_reader reader;
@@ -222,7 +223,7 @@ static int process_request(void)
if (!command)
die("no command requested");
- command->command(the_repository, &keys, &reader);
+ command->command(the_repository, opts->config_context, &keys, &reader);
argv_array_clear(&keys);
return 0;
@@ -249,10 +250,10 @@ void serve(struct serve_options *options)
* a single request/response exchange
*/
if (options->stateless_rpc) {
- process_request();
+ process_request(options);
} else {
for (;;)
- if (process_request())
+ if (process_request(options))
break;
}
}
diff --git a/serve.h b/serve.h
index fe65ba9f46..d527224cbb 100644
--- a/serve.h
+++ b/serve.h
@@ -8,6 +8,13 @@ extern int has_capability(const struct argv_array *keys, const char *capability,
struct serve_options {
unsigned advertise_capabilities;
unsigned stateless_rpc;
+
+ /*
+ * Some operations may need to know the context when looking up config;
+ * e.g., set this to "uploadpack" to respect "uploadpack.hiderefs" (as
+ * opposed to "receive.hiderefs").
+ */
+ const char *config_context;
};
#define SERVE_OPTIONS_INIT { 0 }
extern void serve(struct serve_options *options);
diff --git a/upload-pack.c b/upload-pack.c
index 5e81f1ff24..914bbb40bc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1413,8 +1413,8 @@ enum fetch_state {
FETCH_DONE,
};
-int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request)
+int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request)
{
enum fetch_state state = FETCH_PROCESS_ARGS;
struct upload_pack_data data;
diff --git a/upload-pack.h b/upload-pack.h
index cab2178796..2a9f51197c 100644
--- a/upload-pack.h
+++ b/upload-pack.h
@@ -13,8 +13,8 @@ void upload_pack(struct upload_pack_options *options);
struct repository;
struct argv_array;
struct packet_reader;
-extern int upload_pack_v2(struct repository *r, struct argv_array *keys,
- struct packet_reader *request);
+extern int upload_pack_v2(struct repository *r, const char *config_context,
+ struct argv_array *keys, struct packet_reader *request);
struct strbuf;
extern int upload_pack_advertise(struct repository *r,
--
2.20.0.734.gb4f2c0d2be
^ permalink raw reply related
* [PATCH 2/3] parse_hide_refs_config: handle NULL section
From: Jeff King @ 2018-12-11 10:43 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, Jonathan Tan
In-Reply-To: <20181211104236.GA6899@sigill.intra.peff.net>
This helper function looks for config in two places: transfer.hiderefs,
or $section.hiderefs, where $section is passed in by the caller (and is
"uploadpack" or "receive", depending on the context).
In preparation for callers which do not even have that context (namely
the "git-serve" command), let's handle a NULL by looking only at
transfer.hiderefs (as opposed to segfaulting).
Signed-off-by: Jeff King <peff@peff.net>
---
refs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index f9936355cd..099e91d9cc 100644
--- a/refs.c
+++ b/refs.c
@@ -1267,7 +1267,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
{
const char *key;
if (!strcmp("transfer.hiderefs", var) ||
- (!parse_config_key(var, section, NULL, NULL, &key) &&
+ (section &&
+ !parse_config_key(var, section, NULL, NULL, &key) &&
!strcmp(key, "hiderefs"))) {
char *ref;
int len;
--
2.20.0.734.gb4f2c0d2be
^ permalink raw reply related
* [PATCH 3/3] upload-pack: support hidden refs with protocol v2
From: Jeff King @ 2018-12-11 10:44 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, Jonathan Tan
In-Reply-To: <20181211104236.GA6899@sigill.intra.peff.net>
In the v2 protocol, upload-pack's advertisement has been moved to the
"ls-refs" command. That command does not respect hidden-ref config (like
transfer.hiderefs) at all, and advertises everything.
While there are some features that are not supported in v2 (e.g., v2
always allows fetching any sha1 without respect to advertisements), the
lack of this feature is not documented and is likely just a bug. Let's
make it work, as otherwise upgrading a server to a v2-capable git will
start exposing these refs that the repository admin has asked to remain
hidden.
Note that we depend on the config_context from the caller here to
realize that we should respect uploadpack.hiderefs. When run from the
"git-serve" tool, we won't have that context and will only respect
transfer.hiderefs. This should be OK, as git-serve is not actually used
for normal protocol operations.
Signed-off-by: Jeff King <peff@peff.net>
---
ls-refs.c | 12 ++++++++++++
t/t5512-ls-remote.sh | 6 ++++++
2 files changed, 18 insertions(+)
diff --git a/ls-refs.c b/ls-refs.c
index e8e31475f0..8a8143338b 100644
--- a/ls-refs.c
+++ b/ls-refs.c
@@ -5,6 +5,7 @@
#include "argv-array.h"
#include "ls-refs.h"
#include "pkt-line.h"
+#include "config.h"
/*
* Check if one of the prefixes is a prefix of the ref.
@@ -40,6 +41,9 @@ static int send_ref(const char *refname, const struct object_id *oid,
const char *refname_nons = strip_namespace(refname);
struct strbuf refline = STRBUF_INIT;
+ if (ref_is_hidden(refname_nons, refname))
+ return 0;
+
if (!ref_match(&data->prefixes, refname))
return 0;
@@ -69,6 +73,12 @@ static int send_ref(const char *refname, const struct object_id *oid,
return 0;
}
+static int ls_refs_config(const char *var, const char *value,
+ void *config_context)
+{
+ return parse_hide_refs_config(var, value, config_context);
+}
+
int ls_refs(struct repository *r,
const char *config_section,
struct argv_array *keys,
@@ -78,6 +88,8 @@ int ls_refs(struct repository *r,
memset(&data, 0, sizeof(data));
+ git_config(ls_refs_config, (void *)config_section);
+
while (packet_reader_read(request) != PACKET_READ_FLUSH) {
const char *arg = request->line;
const char *out;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..ca69636fd5 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -204,6 +204,12 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
grep refs/tags/magic actual
'
+test_expect_success 'protocol v2 supports hiderefs' '
+ test_config uploadpack.hiderefs refs/tags &&
+ git -c protocol.version=2 ls-remote . >actual &&
+ ! grep refs/tags actual
+'
+
test_expect_success 'ls-remote --symref' '
git fetch origin &&
cat >expect <<-EOF &&
--
2.20.0.734.gb4f2c0d2be
^ permalink raw reply related
* Re: Announcing Pro Git Second Edition Reedited
From: Jeff King @ 2018-12-11 10:50 UTC (permalink / raw)
To: Jon Forrest; +Cc: git
In-Reply-To: <pujni2$ulb$1@blaine.gmane.org>
On Sun, Dec 09, 2018 at 10:42:12AM -0800, Jon Forrest wrote:
> Several years ago I released what I called Pro Git Reedited. This was an
> attempt to tighten up the text of the excellent Pro Git book written by
> Scott Chacon. Since then, Scott and Ben Straub released the second
> edition of Pro Git so once again I'm releasing a reedited version of
> what they wrote. I hope you enjoy it.
>
> The PDF of my version is at
>
> https://drive.google.com/file/d/18wGebSU0dyYU1L_bfyoDQtZRF1Vo1H3p/view?usp=sharing
>
> If there's enough interest, I'll try to put up an HTML version of
> the book.
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.
-Peff
^ permalink raw reply
* Re: [PATCH on master v2] revision: use commit graph in get_reference()
From: Jeff King @ 2018-12-11 10:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, git, stolee
In-Reply-To: <xmqqwooj5xpr.fsf@gitster-ct.c.googlers.com>
On Sun, Dec 09, 2018 at 09:51:28AM +0900, Junio C Hamano wrote:
> > -static int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)
> > +static struct commit *parse_commit_in_graph_one(struct repository *r,
> > + struct commit_graph *g,
> > + struct commit *shell,
> > + const struct object_id *oid)
>
> Now the complexity of the behaviour of this function deserves to be
> documented in a comment in front. Let me see if I can get it
> correctly without such a comment by explaining the function aloud.
>
> The caller may or may not have already obtained an in-core commit
> object for a given object name, so shell could be NULL but otherwise
> it could be used for optimization. When shell==NULL, the function
> looks up the commit object using the oid parameter instead. The
> returned in-core commit has the parents etc. filled as if we ran
> parse_commit() on it. If the commit is not yet in the graph, the
> caller may get a NULL even if the commit exists.
Yeah, this was the part that took me a bit to figure out, as well. The
optimization here is really just avoiding a call to lookup_commit(),
which will do a single hash-table lookup. I wonder if that's actually
worth this more complex interface (as opposed to just always taking an
oid and then always returning a "struct commit", which could be old or
new).
-Peff
^ permalink raw reply
* Re: [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit
From: Jeff King @ 2018-12-11 10:57 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
In-Reply-To: <20181209225628.22216-2-szeder.dev@gmail.com>
On Sun, Dec 09, 2018 at 11:56:22PM +0100, SZEDER Gábor wrote:
> Right now if a test script receives SIGTERM or SIGHUP (e.g., because a
> test was hanging and the user 'kill'-ed it or simply closed the
> terminal window the test was running in), the shell exits immediately.
> This can be annoying if the test script did any global setup, like
> starting apache or git-daemon, as it will not have an opportunity to
> clean up after itself. A subsequent run of the test won't be able to
> start its own daemon, and will either fail or skip the tests.
>
> Instead, let's trap SIGTERM and SIGHUP as well to make sure we do a
> clean shutdown, and just chain it to a normal exit (which will trigger
> any cleanup).
>
> This patch follows suit of da706545f7 (t: translate SIGINT to an exit,
> 2015-03-13), and even stole its commit message as well.
No wonder it was so nicely explained. ;)
I think this is quite a reasonable thing to do. Since we're trying to
clean up, in theory we would like to hook any signal death, but these
three are the common ones in practice. We handle QUIT and PIPE as well
in our C code; the latter isn't an issue here. SIGQUIT is a possibility,
I guess, but seems rather unlikely.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/7] test-lib: parse some --options earlier
From: Jeff King @ 2018-12-11 11:09 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
In-Reply-To: <20181209225628.22216-3-szeder.dev@gmail.com>
On Sun, Dec 09, 2018 at 11:56:23PM +0100, SZEDER Gábor wrote:
> 'test-lib.sh' looks for the presence of certain options like '--tee'
> and '--verbose-log', so it can execute the test script again to save
> its standard output and error. This happens way before the actual
> option parsing loop, and the condition looking for these options looks
> a bit odd, too. This patch series will add two more options to look
> out for, and, in addition, will have to extract these options' stuck
> arguments (i.e. '--opt=arg') as well.
>
> Add a proper option parsing loop to check these select options early
> in 'test-lib.sh', making this early option checking more readable and
> keeping those later changes in this series simpler. Use a 'for opt in
> "$@"' loop to iterate over the options to preserve "$@" intact, so
> options like '--verbose-log' can execute the test script again with
> all the original options.
>
> As an alternative, we could parse all options early, but there are
> options that do require an _unstuck_ argument, which is tricky to
> handle properly in such a for loop, and the resulting complexity is,
> in my opinion, worse than having this extra, partial option parsing
> loop.
In general, I'm not wild about having multiple option-parsing loops that
skip the normal left-to-right parsing, since it introduces funny corner
cases (like "-foo --bar" which should be the same as "--foo=--bar"
instead thinking that "--bar" was passed as an option).
But looking at what this is replacing:
> -case "$GIT_TEST_TEE_STARTED, $* " in
> -done,*)
> - # do not redirect again
> - ;;
> -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)
your version is easily an order of magnitude less horrible. ;)
> t/test-lib.sh | 53 +++++++++++++++++++++++++++++++--------------------
> 1 file changed, 32 insertions(+), 21 deletions(-)
This looks good to me overall, though...
> +# Parse some options early, taking care to leave $@ intact.
> +for opt
> +do
> + case "$opt" in
> + --tee)
> + tee=t ;;
> + -V|--verbose-log)
> + verbose_log=t ;;
> + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
> + valgrind=memcheck ;;
> + --valgrind=*)
> + valgrind=${opt#--*=} ;;
> + --valgrind-only=*)
> + valgrind_only=${opt#--*=} ;;
> + *)
> + # Other options will be handled later.
> + esac
> +done
> [...]
> +elif test -n "$tee" || test -n "$verbose_log" ||
> + test -n "$valgrind" || test -n "$valgrind_only"
Now that we've nicely moved the parsing up, would it make sense to put
the annotation for "this option implies --tee" with those options?
I.e., set tee=t when we see --verbose-log, which keeps all of the
verbose-log logic together?
> @@ -336,9 +344,12 @@ do
> echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
> fi
> shift ;;
> - -V|--verbose-log)
> - verbose_log=t
> - shift ;;
> + --tee|\
> + -V|--verbose-log|\
> + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
> + --valgrind=*|\
> + --valgrind-only=*)
> + shift ;; # These options were handled already.
> *)
It's too bad there's not an easy way to selectively remove from the $@
array (which would avoid duplicating this list here).
The best I could come up with is:
-- >8 --
first=t
for i in "$@"; do
test -n "$first" && set --
first=
case "$i" in
--foo)
echo "saw foo" ;;
*)
set -- "$@" "$i" ;;
esac
done
for i in "$@"; do
echo "remainder: $i"
done
-- 8< --
but I won't be surprised if there are portability problems with
assigning $@ in the middle of a loop that iterates over it.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
From: Jeff King @ 2018-12-11 11:16 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Junio C Hamano
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
On Sun, Dec 09, 2018 at 11:56:21PM +0100, SZEDER Gábor wrote:
> This patch series tries to make reproducing rare failures in flaky
> tests easier: it adds the '--stress' option to our test library to run
> the test script repeatedly in multiple parallel jobs, in the hope that
> the increased load creates enough variance in the timing of the test's
> commands that such a failure is eventually triggered.
>
> Notable changes since v1:
>
> - Made it more Peff-friendly :), namely it will now show the log of
> the failed test and will rename its trash directory.
>
> Furthermore, '--stress' will now imply '--verbose -x --immediate'.
:) Thanks. I do sympathize with the notion of keeping orthogonal things
orthogonal, but I hope that this will make the tool a little more
pleasant to use out of the box. We can always tweak the behavior later,
too. It's not like this is something user-facing that we've promised as
a scripting interface.
> - Improved abort handling based on the discussion of the previous
> version. (As a result, the patch is so heavily modified, that
> 'range-diff' with default parameters consideres it a different
> patch; increasing the creation factor then results in one big ugly
> diff of a diff, so I left it as-is.)
Yeah, this all looked good to me.
> - Added a few new patches, mostly preparatory refactorings, though
> the first one might be considered a bugfix.
I left a few minor comments, but these all looked good to me.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
From: Johannes Schindelin @ 2018-12-11 11:25 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
In-Reply-To: <11b17e5d-e843-463b-77da-263e8e3b7598@kdbg.org>
Hi Hannes,
On Mon, 10 Dec 2018, Johannes Sixt wrote:
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index 34b3880b29..4d009901d8 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)
> > char *mingw_mktemp(char *template)
> > {
> > wchar_t wtemplate[MAX_PATH];
> > + int offset = 0;
> > +
> > if (xutftowcs_path(wtemplate, template) < 0)
> > return NULL;
> > +
> > + if (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&
> > + iswalpha(wtemplate[0]) && wtemplate[1] == L':') {
> > + /* We have an absolute path missing the drive prefix */
>
> This comment is true for the source part, template, but I can't find
> where the destination, wtemplate, suddenly gets the drive prefix. As far
> as I can see, xutftowcs_path() just does a plain textual conversion
> without any interpretation of the text as path. Can you explain it?
It is legal on Windows for such a path to lack the drive prefix, also in
the wide-character version. So the explanation is: even `wtemplate` won't
get the drive prefix. It does not need to.
> BTW, iswalpha() is not restricted to ASCII letters, I would rewrite it
> as (wtemplate[0] <= 127 && isalpha(wtemplate[0]).
Very good point! Will fix.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH 0/3] protocol v2 and hidden refs
From: Ævar Arnfjörð Bjarmason @ 2018-12-11 11:45 UTC (permalink / raw)
To: Jeff King; +Cc: git, Brandon Williams, Jonathan Tan
In-Reply-To: <20181211104236.GA6899@sigill.intra.peff.net>
On Tue, Dec 11 2018, Jeff King wrote:
> When using the v2 protocol, hidden-ref config is not respected at all:
>
> $ git config transfer.hiderefs refs/tags/
> $ git -c protocol.version=0 ls-remote . | grep -c refs/tags
> 0
> $ git -c protocol.version=2 ls-remote . | grep -c refs/tags
> 1424
>
> The fix in patch 3 is pretty straightforward, but note:
>
> - I'm a little worried this may happen again with future features. The
> root cause is that "ls-refs" follows a different code path than the
> ref advertisement for "upload-pack". So if we add any new config,
> it needs to go both places (non ref-advertisement config is OK, as
> the v2 "fetch" command is a lot closer to a v0 upload-pack).
>
> I think this is just an issue for any future features. I looked for
> other existing features which might be missing in v2, but couldn't
> find any.
>
> I don't know if there's a good solution. I tried running the whole
> test suite with v2 as the default. It does find this bug, but it has
> a bunch of other problems (notably fetch-pack won't run as v2, but
> some other tests I think also depend on v0's reachability rules,
> which v2 is documented not to enforce).
I think a global test mode for it would be a very good idea.
> - The "serve" command is funky, because it has no concept of whether
> the "ls-refs" is for fetching or pushing. Is git-serve even a thing
> that we want to support going forward? I know part of the original
> v2 conception was that one would be able to just connect to
> "git-serve" and do a number of operations. But in practice the v2
> probing requires saying "I'd like to git-upload-pack, and v2 if you
> please". So no client ever calls git-serve.
>
> Is this something we plan to eventually move to? Or can it be
> considered a funny vestige of the development? In the latter case, I
> think we should consider removing it.
>
> I've worked around it here with patch 2, but note that "git serve"
> would not ever respect uploadpack.hiderefs nor receive.hiderefs
> (since it has no idea which operation it's doing).
>
> The patches are:
>
> [1/3]: serve: pass "config context" through to individual commands
> [2/3]: parse_hide_refs_config: handle NULL section
> [3/3]: upload-pack: support hidden refs with protocol v2
Does this issue rise to the level of needing a security point-release
(which I'm discussing here as the details are already public). The
transfer.hideRefs docs have said:
Even if you hide refs, a client may still be able to steal the
target objects via the techniques described in the "SECURITY"
section of the gitnamespaces(7) man page; it’s best to keep private
data in a separate repository.
So we never promised to hide the objects, but definitely promised to
hide the ref names. I don't know if anyone uses this in practice for
secret ref names, but if they do they have a data leak if they enable
protocol v2.
More importantly, the docs for receive.hideRefs say. "An attempt to
update or delete a hidden ref by git push is rejected.". It seems this
bit was enforced, i.e. this passes before and after your 3/3, but I have
not dug enough to be 100% satisfied with that.
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca69636fd5..20059c3308 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -210,6 +210,13 @@ test_expect_success 'protocol v2 supports hiderefs' '
! grep refs/tags actual
'
+test_expect_success 'protocol v2 respects hiderefs when pushing' '
+ git init --bare server.git &&
+ git -C server.git config transfer.hideRefs refs/tags &&
+ test_must_fail git -c protocol.version=0 push "file://$PWD/server.git" mark &&
+ test_must_fail git -c protocol.version=2 push "file://$PWD/server.git" mark
+'
+
test_expect_success 'ls-remote --symref' '
git fetch origin &&
cat >expect <<-EOF &&
If there's some bug where you can bypass this push protection that would
be much worse. E.g. GitLab uses "keep-around" refs to track its own
internal state, and it would be bad if users could manipulate it.
> builtin/upload-pack.c | 1 +
> ls-refs.c | 16 +++++++++++++++-
> ls-refs.h | 3 ++-
> refs.c | 3 ++-
> serve.c | 9 +++++----
> serve.h | 7 +++++++
> t/t5512-ls-remote.sh | 6 ++++++
> upload-pack.c | 4 ++--
> upload-pack.h | 4 ++--
> 9 files changed, 42 insertions(+), 11 deletions(-)
>
> -Peff
^ permalink raw reply
* Re: [PATCH] run-command: report exec failure
From: Johannes Schindelin @ 2018-12-11 12:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, John Passaro
In-Reply-To: <xmqqd0q8liow.fsf@gitster-ct.c.googlers.com>
Hi Junio,
On Tue, 11 Dec 2018, Junio C Hamano wrote:
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index cf932c8514..9c83d44d9c 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -13,11 +13,13 @@ cat >hello-script <<-EOF
> EOF
>
> test_expect_success 'start_command reports ENOENT (slash)' '
> - test-tool run-command start-command-ENOENT ./does-not-exist
> + test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
> + test_i18ngrep "cannot run" err
This breaks on Windows (on Windows, the error message says "cannot spawn", see
https://dev.azure.com/git-for-windows/git/_build/results?buildId=26917):
-- snipsnap --
[...]
2018-12-11T11:13:59.9924183Z ++ grep 'cannot run' err
2018-12-11T11:14:00.0092569Z ++ echo 'error: '\''grep cannot run' 'err'\'' didn'\''t find a match in:'
2018-12-11T11:14:00.0099500Z error: 'grep cannot run err' didn't find a match in:
2018-12-11T11:14:00.0100663Z ++ test -s err
2018-12-11T11:14:00.0101058Z ++ cat err
2018-12-11T11:14:00.0239691Z error: cannot spawn ./does-not-exist: No such file or directory
2018-12-11T11:14:00.0254289Z ++ return 1
2018-12-11T11:14:00.0254489Z not ok 1 - start_command reports ENOENT (slash)
2018-12-11T11:14:00.0258844Z error: last command exited with $?=1
2018-12-11T11:14:00.0472195Z #
2018-12-11T11:14:00.0473619Z # test-tool run-command start-command-ENOENT ./does-not-exist 2>err &&
2018-12-11T11:14:00.0473874Z # test_i18ngrep "cannot run" err
2018-12-11T11:14:00.0474098Z #
^ permalink raw reply
* Re: [PATCH 1/5] multi-pack-index: prepare for 'expire' verb
From: Derrick Stolee @ 2018-12-11 12:32 UTC (permalink / raw)
To: SZEDER Gábor, Stefan Beller
Cc: gitgitgadget, git, Jeff King, Jonathan Nieder,
Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee
In-Reply-To: <20181211015957.GR30222@szeder.dev>
On 12/10/2018 8:59 PM, SZEDER Gábor wrote:
> On Mon, Dec 10, 2018 at 05:35:28PM -0800, Stefan Beller wrote:
>> On Mon, Dec 10, 2018 at 10:06 AM Derrick Stolee via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>> +expire::
>>> + When given as the verb,
>> Can it be given in another way? Or rather "if the verb is expire",
>> then ...
>> (I just checked the current man page, and both write and verify use
>> this pattern as well. I find it strange as this first part of the sentence
>> conveys little information, but is repeated 3 times now (once for
>> each verb)).
>>
>> Maybe we can restructure the man page to have it more like
>>
>> The following verbs are available:
>> +write::
>> + create a new MIDX file, writing to <dir>/packs/multi-pack-index.
>> +
>> +verify::
>> + verify the contents ...
> I think a s/verb/subcommand/ would help a lot, too, because that's
> what we call it everywhere else.
Thanks, both. V2 will include a new patch that reformats the doc to use
these suggestions, then extend it for the new subcommand.
-Stolee
^ permalink raw reply
* Re: [PATCH] http: add http.version option to select http protocol version
From: Johannes Schindelin @ 2018-12-11 12:40 UTC (permalink / raw)
To: Eric Sunshine; +Cc: silvio.fricke, Git List
In-Reply-To: <CAPig+cQW5_9fH-P8X50Mx5kGJRwEOskw2L49Ajk+3D4xWpcOHg@mail.gmail.com>
Hi Eric,
On Mon, 10 Dec 2018, Eric Sunshine wrote:
> On Mon, Dec 10, 2018 at 5:50 PM Silvio Fricke <silvio.fricke@gmail.com> wrote:
> > HTTP has several protocol versions. By default, libcurl is using HTTP/2
> > today and check if the remote can use this protocol variant and fall
> > back to a previous version if not.
> >
> > Under rare conditions it is needed to switch the used protocol version
> > to fight again wrongly implemented authorization mechanism like ntlm
> > with gssapi on remote side.
Please note that this has been addressed for NTLM in
https://github.com/curl/curl/pull/3345 and the gssapi problem is probably
worked around by https://github.com/curl/curl/pull/3349.
Both patches were backported to the cURL version included in Git for
Windows v2.20.0.
> > Signed-off-by: Silvio Fricke <silvio.fricke@gmail.com>
>
> This looks very similar to [1] which is already in Junio's "next"
> branch (although not yet in a released version of Git).
Small correction: it is in Git *for Windows* v2.20.0, so in a manner of
speaking it *is* in a released version of Git.
The reason: even if we included the NTLM/Kerberos patches in Git for
Windows, there might be other scenarios where neither of those patches
catch.
Ciao,
Johannes
> [1]: https://public-inbox.org/git/71f8b71b346f132d0dc9a23c9a7f2ca2cb91966f.1541735051.git.gitgitgadget@gmail.com/
>
^ 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