* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-05 20:21 UTC (permalink / raw)
To: Josh Triplett
Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy, Mike Hommey
In-Reply-To: <20161105151836.wztypzrdywyltvrc@x>
On Sat, Nov 5, 2016 at 4:18 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Sat, Nov 05, 2016 at 01:45:27PM +0100, Christian Couder wrote:
>> And with what Peff says above it looks like we will need ways
>> configure and tweak commit reachability with gitlink/gitref anyway. So
>> the point of gitref compared to gitlink would be that they just have a
>> different reachability by default. But couldn't that be replaced by a
>> default rule saying that when a gitlink is reached "this way or that
>> way" then the commit reachability should be enforced, and otherwise it
>> should not be?
>
> Any version of git unaware of that rule, though, would consider objects
> only reachable by gitlink as unreachable and delete them, causing data
> loss. Likewise for a server not aware of that rule. And a server
> unaware of that rule would not supply those objects to a client pulling
> such a branch.
Yeah, so you would really need an up-to-date server and client to
store the git-series data.
But anyway if we create a gitref object, you would also need
up-to-date servers and clients to make it work.
> So I don't think "gitlink defined as reachable" quite works, unless we
> make some other format-incompatible change that forces clients and
> servers touching that gitlink to know about that reachability rule. (In
> the absence of a hack such as making the same commit a parent.)
There are other tools that would like to tweak reachability rules for objects.
For example Mike Hommey's git-cinnabar:
https://public-inbox.org/git/20150331100756.GA13377@glandium.org/
and my current work on external object databases:
https://public-inbox.org/git/20160628181933.24620-1-chriscool@tuxfamily.org/
would be interested in a way to make some blobs not reachable.
So if we had default rules and a generic way to specify that some
objects are, or are not, reachable, that could be used by many tools,
and the design of these tools would be simplified.
Maybe this could be specified in the attributes files, or in a special
file like for shallow clone.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-05 15:18 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy
In-Reply-To: <CAP8UFD1EZ8HBzLAeyFBFgU7n2uJpswqgEgA4XM1YJuRAG_ZAAQ@mail.gmail.com>
On Sat, Nov 05, 2016 at 01:45:27PM +0100, Christian Couder wrote:
> And with what Peff says above it looks like we will need ways
> configure and tweak commit reachability with gitlink/gitref anyway. So
> the point of gitref compared to gitlink would be that they just have a
> different reachability by default. But couldn't that be replaced by a
> default rule saying that when a gitlink is reached "this way or that
> way" then the commit reachability should be enforced, and otherwise it
> should not be?
Any version of git unaware of that rule, though, would consider objects
only reachable by gitlink as unreachable and delete them, causing data
loss. Likewise for a server not aware of that rule. And a server
unaware of that rule would not supply those objects to a client pulling
such a branch.
So I don't think "gitlink defined as reachable" quite works, unless we
make some other format-incompatible change that forces clients and
servers touching that gitlink to know about that reachability rule. (In
the absence of a hack such as making the same commit a parent.)
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-05 12:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: Josh Triplett, git, Shawn O. Pierce, Jeff King,
Nguyen Thai Ngoc Duy
In-Reply-To: <CAP8UFD0bqNQZ3nuGUDX0qrSo44hf1NL9LeZB_FQcXg3j0mD38A@mail.gmail.com>
On Sat, Nov 5, 2016 at 1:17 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sat, Nov 5, 2016 at 5:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> Couldn't a RefTree be used to store refs that point to the base
>>> commit,
>>
>> I think it is the other way around. With the new "gitref" thing
>> that is a pointer to an in-repository commit, RefTree can be
>> naturally implemented.
>
> Yeah, I should have read Shawn's RefTree email thread again before
> posting and especially before replying to Josh.
By the way, reading the following email by Peff where gitlink
reachability was already discussed:
https://public-inbox.org/git/20151217221045.GA8150@sigill.intra.peff.net/
and where Peff wrote:
> Of course, the lack of reachability has advantages, too. You can
> drop commits pointed to by old reflogs without rewriting the ref
> history. Unfortunately you cannot expunge the reflogs at all. That's
> good if you like audit trails. Bad if you are worried that your reflogs
> will grow large. :)
I think that we may not need "gitref" at all. We perhaps could just
have more ways to configure and tweak how a repo manages commit
reachability related to gitlinks.
With shallow clones we already need ways to configure and tweak commit
reachability anyway.
And with what Peff says above it looks like we will need ways
configure and tweak commit reachability with gitlink/gitref anyway. So
the point of gitref compared to gitlink would be that they just have a
different reachability by default. But couldn't that be replaced by a
default rule saying that when a gitlink is reached "this way or that
way" then the commit reachability should be enforced, and otherwise it
should not be?
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-05 12:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Triplett, git, Shawn O. Pierce, Jeff King
In-Reply-To: <xmqq1syqedv4.fsf@gitster.mtv.corp.google.com>
On Sat, Nov 5, 2016 at 5:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> Couldn't a RefTree be used to store refs that point to the base
>> commit,
>
> I think it is the other way around. With the new "gitref" thing
> that is a pointer to an in-repository commit, RefTree can be
> naturally implemented.
Yeah, I should have read Shawn's RefTree email thread again before
posting and especially before replying to Josh.
^ permalink raw reply
* Re: gitk: avoid obscene memory consumption
From: Paul Mackerras @ 2016-11-05 11:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: Markus Hitter, git@vger.kernel.org
In-Reply-To: <CAGZ79kbavzGJ2sAcz5heg+BO+tZ=TgtrhxMH1-kqeJUpNNavyw@mail.gmail.com>
On Fri, Nov 04, 2016 at 03:45:09PM -0700, Stefan Beller wrote:
> On Fri, Nov 4, 2016 at 12:49 PM, Markus Hitter <mah@jump-ing.de> wrote:
> >
> > Hello all,
>
> +cc Paul Mackeras, who maintains gitk.
Thanks.
> >
> > after Gitk brought my shabby development machine (Core2Duo, 4 GB RAM, Ubuntu 16.10, no swap to save the SSD) to its knees once more than I'm comfortable with, I decided to investigate this issue.
> >
> > Result of this investigation is, my Git repo has a commit with a diff of some 365'000 lines and Gitk tries to display all of them, consuming more than 1.5 GB of memory.
> >
> > The solution is to cut off diffs at 50'000 lines for the display. This consumes about 350 MB RAM, still a lot. These first 50'000 lines are shown, followed by a copyable message on how to view the full diff on the command line. Diffs shorter than this limit are displayed as before.
That sounds reasonable.
>
> Bikeshedding: I'd argue to even lower the number to 5-10k lines.
I could go with 10k.
>
> >
> > To test the waters whether such a change is welcome, here's the patch as I currently use it. If this patch makes sense I'll happily apply change requests and bring it more in line with Git's patch submission expectations.
>
> I have never contributed to gitk myself,
> which is hosted at git://ozlabs.org/~paulus/gitk
> though I'd expect these guide lines would roughly apply:
> https://github.com/git/git/blob/master/Documentation/SubmittingPatches
Paul.
^ permalink raw reply
* [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: Johannes Schindelin @ 2016-11-05 9:50 UTC (permalink / raw)
To: git-for-windows, git
Dear Git users,
I finally got around to rebase the Windows-specific patches (which seem to
not make it upstream as fast as we get new ones) on top of upstream Git
v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
It would be really nice if those of you who have access to Windows [*2*]
could try it out and report bugs before v2.11.0 final.
Thanks,
Johannes
Footnote *1*: https://github.com/git-for-windows/git/wiki/MinGit
Footnote *2*: You can get free (time-limited) VMs running Windows here:
http://dev.modern.ie/tools/vms/windows/
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Torsten Bögershausen @ 2016-11-05 7:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, santiago, gitster, peff, sunshine, walters
In-Reply-To: <xmqqwpgoqjct.fsf@gitster.mtv.corp.google.com>
> * st/verify-tag (2016-10-10) 7 commits
> - t/t7004-tag: Add --format specifier tests
> - t/t7030-verify-tag: Add --format specifier tests
> - builtin/tag: add --format argument for tag -v
> - builtin/verify-tag: add --format to verify-tag
> - tag: add format specifier to gpg_verify_tag
> - ref-filter: add function to print single ref_array_item
> - gpg-interface, tag: add GPG_VERIFY_QUIET flag
>
> "git tag" and "git verify-tag" learned to put GPG verification
> status in their "--format=<placeholders>" output format.
>
> Waiting for a reroll.
> cf. <20161007210721.20437-1-santiago@nyu.edu>
I don't know if this has been reported before:
Some tests needs to be protected by GPG:
+test_expect_success GPG 'verifying a proper tag with --format pass and format accordingly'
test_expect_success GPG 'verifying a forged tag with --format fail and format accordingly'
test_expect_success GPG 'verifying a forged tag with --format fail and format accordingly'
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #09; Mon, 31)
From: Torsten Bögershausen @ 2016-11-05 7:27 UTC (permalink / raw)
To: Martin-Louis Bright; +Cc: Lars Schneider, Junio C Hamano, git
In-Reply-To: <CAG2PGspq34wn2bAGyhR6B-XmmayadmL-v3_65y5LJWTWNHXkOQ@mail.gmail.com>
On Thu, Nov 03, 2016 at 09:30:44AM -0400, Martin-Louis Bright wrote:
> I will see if I can find a OSX 10.6 system to test with, and I'll try with
> perl 5.10.
>
> --Martin
No need to worry too much:
I have tested Peffs patch applied on next OK-
And the integration into pu that came the 2nd Novevember is tested OK as well.
(And please everybody: avoid top-posting here)
^ permalink raw reply
* Re: [PATCH 4/5] grep: optionally recurse into submodules
From: Jonathan Tan @ 2016-11-05 5:09 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20161027223834.35312-5-bmwill@google.com>
On Thu, Oct 27, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8887b6add..f34f16df9 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -18,12 +18,20 @@
> #include "quote.h"
> #include "dir.h"
> #include "pathspec.h"
> +#include "submodule.h"
>
> static char const * const grep_usage[] = {
> N_("git grep [<options>] [-e] <pattern> [<rev>...] [[--] <path>...]"),
> NULL
> };
>
> +static const char *super_prefix;
I think that the super_prefix changes could be in its own patch.
> +static int recurse_submodules;
> +static struct argv_array submodule_options = ARGV_ARRAY_INIT;
I guess this has to be static because it is shared by multiple threads.
> +
> +static int grep_submodule_launch(struct grep_opt *opt,
> + const struct grep_source *gs);
> +
> #define GREP_NUM_THREADS_DEFAULT 8
> static int num_threads;
>
> @@ -174,7 +182,10 @@ static void *run(void *arg)
> break;
>
> opt->output_priv = w;
> - hit |= grep_source(opt, &w->source);
> + if (w->source.type == GREP_SOURCE_SUBMODULE)
> + hit |= grep_submodule_launch(opt, &w->source);
> + else
> + hit |= grep_source(opt, &w->source);
It seems to me that GREP_SOURCE_SUBMODULE is of a different nature
than the other GREP_SOURCE_.* - in struct work_item, could we instead
have another variable that distinguishes between submodules and
"native" sources? This might also assuage Junio's concerns in
<xmqq37jbqf83.fsf@gitster.mtv.corp.google.com> about the nature of the
sources.
That variable could also be the discriminant for a tagged union, such
that we have "struct grep_source" for the "native" sources and a new
struct (holding only submodule-relevant information) for the
submodule.
> +/*
> + * Prep grep structures for a submodule grep
> + * sha1: the sha1 of the submodule or NULL if using the working tree
> + * filename: name of the submodule including tree name of parent
> + * path: location of the submodule
> + */
> +static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
> + const char *filename, const char *path)
> +{
> + if (!(is_submodule_initialized(path) &&
> + is_submodule_checked_out(path))) {
> + warning("skiping submodule '%s%s' since it is not initialized and checked out",
> + super_prefix ? super_prefix: "",
> + path);
> + return 0;
> + }
> +
> +#ifndef NO_PTHREADS
> + if (num_threads) {
> + add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, sha1);
> + return 0;
> + } else
> +#endif
> + {
> + struct work_item w;
> + int hit;
> +
> + grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
> + filename, path, sha1);
> + strbuf_init(&w.out, 0);
> + opt->output_priv = &w;
> + hit = grep_submodule_launch(opt, &w.source);
> +
> + write_or_die(1, w.out.buf, w.out.len);
> +
> + grep_source_clear(&w.source);
> + strbuf_release(&w.out);
> + return hit;
> + }
This is at least the third invocation of this "if pthreads, add work,
otherwise do it now" pattern - could this be extracted into its own
function (in another patch)? Ideally, there would also be exactly one
function in which the grep_source.* functions are invoked, and both
"run" and the non-pthread code path can use it.
> +}
> +
> +static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec,
> + int cached)
This line isn't modified other than the line break, as far as I can
tell, so I wouldn't break it.
> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
> new file mode 100755
> index 000000000..b670c70cb
> --- /dev/null
> +++ b/t/t7814-grep-recurse-submodules.sh
> @@ -0,0 +1,99 @@
> +#!/bin/sh
> +
> +test_description='Test grep recurse-submodules feature
> +
> +This test verifies the recurse-submodules feature correctly greps across
> +submodules.
> +'
> +
> +. ./test-lib.sh
> +
Would it be possible to also test it while num_threads is zero? (Or,
if num_threads is already zero, to test it while it is not zero?)
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Junio C Hamano @ 2016-11-05 5:00 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Josh Triplett, Git mailing list
In-Reply-To: <xmqq60o2edy5.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> I think the main complication is that the reachability rules are used
>> during object transfer.
One should not type after spending 20+ waking hours on plane and
airport. I missed it when I wrote my first response, but yes, the
reachability that originates from inside a tree object indeed is a
problem, as we do not dig into trees while doing the want/have/ack
exhange.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Jeff King @ 2016-11-05 4:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Josh Triplett, Git mailing list
In-Reply-To: <xmqq60o2edy5.fsf@gitster.mtv.corp.google.com>
On Fri, Nov 04, 2016 at 09:41:06PM -0700, Junio C Hamano wrote:
> > I think the main complication is that the reachability rules are used
> > during object transfer. So you'd probably want to introduce some
> > protocol extension to say "I understand gitrefs", so that when one side
> > says "I have sha1 X and its reachable objects", we know whether they are
> > including gitrefs there. And likewise receivers with
> > transfer.fsckObjects may complain about the new gitref tree mode
> > (fortunately a new object type shouldn't be needed).
>
> Quite honestly I do not think backward compatibility here matters.
> When gitlinks were introduced, a repository that was created with
> gitlink capable version of Git would have failed "git fsck" that is
> not gitlink aware, and I think this new "link with reachability" is
> the same deal. No existing implemention understands a tree entry
> whose mode bits are 140000 or whatever new bit pattern we would
> assign to this thing. You have to wait until both ends understand
> the new thing, and that is perfectly OK.
I'm OK with saying "if you use the gitref feature, you cannot push or
pull those objects with remotes that do not understand it". But unlike
gitlink, if we fail to notice the situation, we run into a case where we
might silently lose objects, which is bad. So I think we need to be a
bit more careful.
I don't think the problems are insurmountable. I just think that's where
the real complexity is, not in the changes to teach a single git about
gitrefs.
I'm happy to stand back and let you or Josh figure out all the corner
cases. :)
-Peff
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Junio C Hamano @ 2016-11-05 4:42 UTC (permalink / raw)
To: Christian Couder; +Cc: Josh Triplett, git, Shawn O. Pierce, Jeff King
In-Reply-To: <CAP8UFD2+A0MUKazAfSwCvv61TJRPuoOzH5EkqcrBOUi4TcuoDw@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> Couldn't a RefTree be used to store refs that point to the base
> commit,
I think it is the other way around. With the new "gitref" thing
that is a pointer to an in-repository commit, RefTree can be
naturally implemented.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Jeff King @ 2016-11-05 4:41 UTC (permalink / raw)
To: Josh Triplett; +Cc: Jacob Keller, Junio C Hamano, Git mailing list
In-Reply-To: <A269CCA2-5F8C-4AF0-820E-2EA26FEF03D5@joshtriplett.org>
On Fri, Nov 04, 2016 at 09:55:23PM -0600, Josh Triplett wrote:
> >> Is it possible currently for a protocol extension to result in "oh
> >the
> >> server doesn't support this so I'm going to stop pushing"?
> >
> >Yes, it would be easy for the client to abort if the server fails to
> >advertise a particular extension.
>
> And the reverse (old client, new server) should work as well?
Yes. The server says "I know about gitrefs" and if the client does not
respond with "I also know about gitrefs", then the server can act
appropriately (e.g., for a fetch, bail if the fetched content includes
gitrefs).
-Peff
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Junio C Hamano @ 2016-11-05 4:41 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Josh Triplett, Git mailing list
In-Reply-To: <20161104194907.3yxu2rkayfyic4dr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Nov 04, 2016 at 12:19:55PM -0700, Jacob Keller wrote:
>
>> I agree with your assessment here. The main difficulty in implementing
>> gitrefs is to ensure that they actually do get picked up by
>> reachability checks to prevent dropping commits. I'm not sure how easy
>> this is, but I would much rather we go this route rather than
>> continuing along with the hack. This seems like the ideal solution,
>> since it solves the entire problem and doesn't need more hacks bolted
>> on.
>
> I think the main complication is that the reachability rules are used
> during object transfer. So you'd probably want to introduce some
> protocol extension to say "I understand gitrefs", so that when one side
> says "I have sha1 X and its reachable objects", we know whether they are
> including gitrefs there. And likewise receivers with
> transfer.fsckObjects may complain about the new gitref tree mode
> (fortunately a new object type shouldn't be needed).
Quite honestly I do not think backward compatibility here matters.
When gitlinks were introduced, a repository that was created with
gitlink capable version of Git would have failed "git fsck" that is
not gitlink aware, and I think this new "link with reachability" is
the same deal. No existing implemention understands a tree entry
whose mode bits are 140000 or whatever new bit pattern we would
assign to this thing. You have to wait until both ends understand
the new thing, and that is perfectly OK.
Besides, I think the point of having this discussion is that Josh
did a good prototyping work of "git series" to discover what he can
do in that area of "keeping track of history of history" and what
operations are useful, without wasting time on mucking with the
object model and traversal machinery that are available to him.
Now his prototyping is at the point where he knows at least one
enhancement to the core that would help him to redo the prototype in
the right way. And I do not mind helping him from the core side.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-05 3:55 UTC (permalink / raw)
To: Jeff King, Jacob Keller; +Cc: Junio C Hamano, Git mailing list
In-Reply-To: <20161105014817.vm4ush2wfbblzsc7@sigill.intra.peff.net>
On November 4, 2016 7:48:17 PM MDT, Jeff King <peff@peff.net> wrote:
>On Fri, Nov 04, 2016 at 04:34:34PM -0700, Jacob Keller wrote:
>
>> > You might also want fallback rules for storing gitrefs on "old"
>servers
>> > (e.g., backfilling gitrefs you need if the server didn't them in
>the
>> > initial fetch). But I guess storing any gitrefs on such a server is
>> > inherently dangerous, because the server might prune them at any
>time.
>>
>> Is it possible currently for a protocol extension to result in "oh
>the
>> server doesn't support this so I'm going to stop pushing"?
>
>Yes, it would be easy for the client to abort if the server fails to
>advertise a particular extension.
And the reverse (old client, new server) should work as well?
- Josh Triplett
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Jonathan Tan @ 2016-11-05 2:34 UTC (permalink / raw)
To: Brandon Williams, git, sbeller
In-Reply-To: <1477953496-103596-2-git-send-email-bmwill@google.com>
On 10/31/2016 03:38 PM, Brandon Williams wrote:
> + struct strbuf buf = STRBUF_INIT;
> + char *submodule_url = NULL;
> +
> + strbuf_addf(&buf, "submodule.%s.url", module->name);
> + ret = !git_config_get_string(buf.buf, &submodule_url);
> +
> + free(submodule_url);
> + strbuf_release(&buf);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Determine if a submodule has been checked out at a given 'path'
> + */
> +int is_submodule_checked_out(const char *path)
> +{
> + int ret = 0;
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addf(&buf, "%s/.git", path);
> + ret = file_exists(buf.buf);
> +
> + strbuf_release(&buf);
In this and the previous function, you can use xstrfmt.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Jeff King @ 2016-11-05 1:48 UTC (permalink / raw)
To: Jacob Keller; +Cc: Junio C Hamano, Josh Triplett, Git mailing list
In-Reply-To: <CA+P7+xpwUZscpgzLJYf5vkKKsT6SFkC3TrsyBJXJjGo9cF94nQ@mail.gmail.com>
On Fri, Nov 04, 2016 at 04:34:34PM -0700, Jacob Keller wrote:
> > You might also want fallback rules for storing gitrefs on "old" servers
> > (e.g., backfilling gitrefs you need if the server didn't them in the
> > initial fetch). But I guess storing any gitrefs on such a server is
> > inherently dangerous, because the server might prune them at any time.
>
> Is it possible currently for a protocol extension to result in "oh the
> server doesn't support this so I'm going to stop pushing"?
Yes, it would be easy for the client to abort if the server fails to
advertise a particular extension.
What I would worry about more is that "somehow" an older client gets
hold of history with a gitref, and then pushes it. It would be nice if
even an old server said "nope, I don't understand this and I won't take
it" rather than propagating the data to a server that will throw it
away.
> Right. I'm assuming tree objects don't get checked for invalid mode
> already? If they do, we could just change the mode to something
> unsupported currently. But... that seems like it might not be the case
> because it requires checking every tree object coming in?
>
> I'm not familiar with what sort of checking already exists... Thoughts?
If the server sets receive.fsckObjects, then fsck_tree() runs and will
reject any non-standard mode. That option is not the default, though
some big hosters set it (GitHub does, but I am actually not that worried
about GitHub; if gitrefs support materialized I would probably ship it
there fairly promptly).
-Peff
^ permalink raw reply
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Christian Couder @ 2016-11-05 0:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <xmqq8tt3ovnp.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 8:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> +splitIndex.maxPercentChange::
>> + When the split index feature is used, this specifies the
>> + percent of entries the split index can contain compared to the
>> + whole number of entries in both the split index and the shared
>> + index before a new shared index is written.
>> + The value should be between 0 and 100. If the value is 0 then
>> + a new shared index is always written, if it is 100 a new
>> + shared index is never written.
>
> Hmph. The early part of the description implies this will kick in
> only when some other conditions (i.e. the bit in the index or the
> other configuration) are met, but if this disables the split index
> when it is set to 0, would we even need the other configuration
> variable? IOW, perhaps we can do without core.splitIndex?
I think it is easier for user to be able to just set core.splitIndex
to true to enable split-index.
If we ask them to instead set splitIndex.maxPercentChange to a
"reasonable value like 20" to enable it, they will start to wonder why
20 and not 50 or 100 and might be unnecessarily confused.
Also it might be good to make it possible to have other values than
"true" and "false" for core.splitIndex in the future.
For example if we decide to first enable split-index only on
$GIT_DIR/index, we might later want an "everywhere" or "always" value
for core.splitIndex if we find a way to enable it on other indexes.
We might also want an "auto" mode in the future when split-index will
work really well, so that it can be turned on automatically on all the
repos where it makes sense (like when the index contains more than
10000 entries for example).
>> + By default the value is 20, so a new shared index is written
>> + if the number of entries in the split index would be greater
>> + than 20 percent of the total number of entries.
>> + See linkgit:git-update-index[1].
^ permalink raw reply
* Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()
From: Christian Couder @ 2016-11-05 0:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <xmqqh97rovx9.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> + return -1;
>
> Perhaps do the usual
>
> return error(_("..."));
>
> here?
Ok, it will be in the next version.
^ permalink raw reply
* Re: [PATCH v3] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-05 0:18 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, git@vger.kernel.org, Blake Burkhart,
Jonathan Nieder
In-Reply-To: <20161104230902.ulkyejwamm6q575a@sigill.intra.peff.net>
On 11/04, Jeff King wrote:
> On Fri, Nov 04, 2016 at 02:35:57PM -0700, Stefan Beller wrote:
>
> > On Fri, Nov 4, 2016 at 1:58 PM, Brandon Williams <bmwill@google.com> wrote:
> > > On 11/04, Brandon Williams wrote:
> > >> Signed-off-by: Brandon Williams <bmwill@google.com>
> > >
> > > Is there an acceptable way to give credit to Jeff for helping with this patch?
> >
> > What about:
> > Helped-by: Jeff King <peff@peff.net>
>
> That, or often I would write:
>
> Based on a patch by Jeff King <peff@peff.net>
>
> in the commit message. Basically anything is OK _except_ forging
> signed-off-by, because it has a very specific meaning. So let me also
> say that I am happy to give my:
>
> Signed-off-by: Jeff King <peff@peff.net>
>
> to the original (which you should add in, to make clear that the
> copyright issues are OK).
>
> In some cases it makes sense to just roll somebody's patch into your
> series, and then build on top. I'm fine with it all going into a single
> patch here.
>
> -Peff
Oh if it would be more clear I can easily break it up into two patches.
--
Brandon Williams
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-04 23:46 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Junio C Hamano, Git mailing list
In-Reply-To: <CA+P7+xoKORo6hC2n-E-gHG2OYg3h-m3ZnUQbdopS7S3-5AWoPQ@mail.gmail.com>
On Fri, Nov 04, 2016 at 04:37:34PM -0700, Jacob Keller wrote:
> On Fri, Nov 4, 2016 at 2:55 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> > That said, I'd *love* to have gitrefs available, for a wide variety of
> > applications, and I can see an argument for introducing them and waiting
> > a few years for them to become universally available, similar to the
> > process gitlinks went through.
> >
> > But I'd also love to have a backward-compatible solution.
> >
> > - Josh Triplett
>
> I think that you won't really find a backwards compatible solution
> other than something like automatically generating refs for each point
> of history. I know that gerrit does something like this by storing
> each version in "refs/changes/id/version" or something along those
> lines. I think this might actually be cleaner than your parent links
> hack, and could be used as a fallback for when gitrefs don't work,
> though you'd have to code exactly how to tell what to push to a
> repository when pushing a series?
I'm not sure what the advantage of that would be, and it would mean that
if you ever have one branch without pushing the other(s), you'd get
severe time-delated breakage due to pruning. (And if you pushed the
series without the other ref(s), its history would look right but then
you couldn't access the underlying versions of the patch series.)
One of my design goals was to *not* need a special "git series push" or
"git series pull"; you should just be able to use git push and git pull,
and you can set up normal refspecs.
That said, I could fairly easily generate the existing format with
artificial parent refs for backward compatibility, and provide a way to
use the new gitref-based storage format if you know that all your
servers and clients can handle it. I'm also open to other suggestions
for how to make such a transition while still working with every git
server and git client that exists today.
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Jacob Keller @ 2016-11-04 23:37 UTC (permalink / raw)
To: Josh Triplett; +Cc: Jeff King, Junio C Hamano, Git mailing list
In-Reply-To: <20161104215538.xmpth6qfuou6nde6@x>
On Fri, Nov 4, 2016 at 2:55 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> That said, I'd *love* to have gitrefs available, for a wide variety of
> applications, and I can see an argument for introducing them and waiting
> a few years for them to become universally available, similar to the
> process gitlinks went through.
>
> But I'd also love to have a backward-compatible solution.
>
> - Josh Triplett
I think that you won't really find a backwards compatible solution
other than something like automatically generating refs for each point
of history. I know that gerrit does something like this by storing
each version in "refs/changes/id/version" or something along those
lines. I think this might actually be cleaner than your parent links
hack, and could be used as a fallback for when gitrefs don't work,
though you'd have to code exactly how to tell what to push to a
repository when pushing a series?
Thanks,
Jake
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Jacob Keller @ 2016-11-04 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Josh Triplett, Git mailing list
In-Reply-To: <20161104194907.3yxu2rkayfyic4dr@sigill.intra.peff.net>
On Fri, Nov 4, 2016 at 12:49 PM, Jeff King <peff@peff.net> wrote:
> I think the main complication is that the reachability rules are used
> during object transfer. So you'd probably want to introduce some
> protocol extension to say "I understand gitrefs", so that when one side
> says "I have sha1 X and its reachable objects", we know whether they are
> including gitrefs there. And likewise receivers with
> transfer.fsckObjects may complain about the new gitref tree mode
> (fortunately a new object type shouldn't be needed).
>
> You might also want fallback rules for storing gitrefs on "old" servers
> (e.g., backfilling gitrefs you need if the server didn't them in the
> initial fetch). But I guess storing any gitrefs on such a server is
> inherently dangerous, because the server might prune them at any time.
>
Is it possible currently for a protocol extension to result in "oh the
server doesn't support this so I'm going to stop pushing"? This would
be a rather hard transition, but it would at least ensure that pushing
to a server which doesn't support gitrefs would fail rather than
silently accept objects and then discard them later? I think this is
the only real transition unless we can make a change that old servers
object to already.
> So perhaps a related question is: how can gitrefs be designed such that
> existing servers reject them (rather than accepting the push and then
> later throwing away half the data). It would be easy to notice in the
> client during a push that we are sending gitrefs to a server which does
> not claim that capability. But it seems more robust if it is the server
> who decides "I will not accept these bogus objects".
>
> I haven't thought all that hard about this. That's just my initial
> thoughts on what sound hard. Tweaking the reachability code doesn't seem
> all that bad; we already know all of the spots that care about
> S_ISGITLINK(). It may even be that some of those spots work out of the
> box (because gitlinks are usually about telling the graph-walking code
> that we _don't_ care about reachability; we do by default for trees and
> blobs).
Right. I'm assuming tree objects don't get checked for invalid mode
already? If they do, we could just change the mode to something
unsupported currently. But... that seems like it might not be the case
because it requires checking every tree object coming in?
I'm not familiar with what sort of checking already exists... Thoughts?
>
> I'd be surprised if all such sites work out of the box, though. Even if
> they see "ah, sha1 X is referenced by tree Y and isn't a gitlink, and
> therefore should be reachable", they need to also note that "X" is a
> commit and recursively walk its objects.
>
They won't all work out of the box, but it shouldn't be much work to
do this part.
> -Peff
^ permalink raw reply
* Re: [PATCH v3] transport: add protocol policy config option
From: Jeff King @ 2016-11-04 23:09 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, git@vger.kernel.org, Blake Burkhart,
Jonathan Nieder
In-Reply-To: <CAGZ79kZB9wMgPDktMUpBfcvgJy16N8P74SvLP5740UNftZkbVg@mail.gmail.com>
On Fri, Nov 04, 2016 at 02:35:57PM -0700, Stefan Beller wrote:
> On Fri, Nov 4, 2016 at 1:58 PM, Brandon Williams <bmwill@google.com> wrote:
> > On 11/04, Brandon Williams wrote:
> >> Signed-off-by: Brandon Williams <bmwill@google.com>
> >
> > Is there an acceptable way to give credit to Jeff for helping with this patch?
>
> What about:
> Helped-by: Jeff King <peff@peff.net>
That, or often I would write:
Based on a patch by Jeff King <peff@peff.net>
in the commit message. Basically anything is OK _except_ forging
signed-off-by, because it has a very specific meaning. So let me also
say that I am happy to give my:
Signed-off-by: Jeff King <peff@peff.net>
to the original (which you should add in, to make clear that the
copyright issues are OK).
In some cases it makes sense to just roll somebody's patch into your
series, and then build on top. I'm fine with it all going into a single
patch here.
-Peff
^ permalink raw reply
* Re: [PATCH v3] transport: add protocol policy config option
From: Jeff King @ 2016-11-04 23:06 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, jrnieder
In-Reply-To: <1478292933-7873-1-git-send-email-bmwill@google.com>
On Fri, Nov 04, 2016 at 01:55:33PM -0700, Brandon Williams wrote:
> Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
> specify a whitelist of protocols to be used in clone/fetch/pull
> commands. This patch introduces new configuration options for more
> fine-grained control for allowing/disallowing protocols. This also has
> the added benefit of allowing easier construction of a protocol
> whitelist on systems where setting an environment variable is
> non-trivial.
Good rationale.
> Now users can specify a policy to be used for each type of protocol via
> the 'protocol.<name>.allow' config option. A default policy for all
> unknown protocols can be set with the 'protocol.allow' config option.
I think "unconfigured" is a better word here than "unknown", as it would
apply to known protocols like "https", etc.
That made me wonder if "unknown" would be a better behavior, but I'm
pretty sure it is not. It is harder to explain, and I think would be
less convenient in practice. I.e., you really do want:
git config protocol.allow never
git config protocol.https.allow always
to allow nothing but https.
> If no user configured default is made git, by default, will allow
> known-safe protocols (http, https, git, ssh, file), disallow
> known-dangerous protocols (ext), and have a default poliy of `user` for
> all other protocols.
I think this is a good way of thinking about it. The order of
enforcement becomes:
- GIT_ALLOW_PROTOCOL; environment variables always take precedence
over config, so this makes sense. And it also is nice to put the
blunt hammer at the front for backwards-compatibility.
- protocol-specific config
- protocol-generic config
- built-in defaults (known-safe, known-scary, unknown)
which seems right.
Also, s/poliy/policy/.
> The supported policies are `always`, `never`, and `user`. The `user`
> policy can be used to configure a protocol to be usable when explicitly
> used by a user, while disallowing it for commands which run
> clone/fetch/pull commands without direct user intervention (e.g.
> recursive initialization of submodules). Commands which can potentially
> clone/fetch/pull from untrusted repositories without user intervention
> can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
> protocols configured to the `user` policy from being used.
Makes sense. I know "user" came from me. I don't know if there is a
better word to describe it. I originally called it "cmdline", but that
seemed too obscure (especially when a tool external to git sets it).
Something like "trusted" might make sense (we allow it only in a
more-trusted setting), but it's kind of vague. And it also doesn't leave
room for there to be more types of trust in the future. So "user" is
probably reasonable (or perhaps "user-only" or similar).
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 27069ac..5d845c4 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2308,6 +2308,31 @@ pretty.<name>::
> Note that an alias with the same name as a built-in format
> will be silently ignored.
>
> +protocol.allow::
> + If set, provide a user defined default policy for all protocols which
> + don't explicitly have a policy (protocol.<name>.allow). By default,
> + if unset, known-safe protocols (http, https, git, ssh, file) have a
> + default policy of `always`, known-dangerous protocols (ext) have a
> + default policy of `never`, and all other protocols have a default policy
> + of `user`. Supported policies:
> ++
> +--
> +
> +* `always` - protocol is always able to be used.
> +
> +* `never` - protocol is never able to be used.
> +
> +* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
> + either unset or has a value of 1. This policy should be used when you want a
> + protocol to be usable by the user but don't want it used by commands which
> + execute clone/fetch/pull commands without user input, e.g. recursive
> + submodule initialization.
Makes sense. I wonder if it would be good to emphasize _directly_ usable
here. I.e., "...when you want a protocol to be directly usable by the
user but don't want...".
Should clone/fetch/pull also include push?
> +protocol.<name>.allow::
> + Set a policy to be used by protocol <name> with clone/fetch/pull commands.
> +
Nice that this matches protocol.allow, so we don't need to re-explain
that.
Should the list of protocols be here? I know they're covered under
GIT_ALLOW_PROTOCOL already, but if this is the preferred system, we
should probably explain them here, and then just have GIT_ALLOW_PROTOCOL
refer the user.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index ab7215e..ab25580 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1150,13 +1150,13 @@ of clones and fetches.
> cloning a repository to make a backup).
>
> `GIT_ALLOW_PROTOCOL`::
> - If set, provide a colon-separated list of protocols which are
> - allowed to be used with fetch/push/clone. This is useful to
> - restrict recursive submodule initialization from an untrusted
> - repository. Any protocol not mentioned will be disallowed (i.e.,
> - this is a whitelist, not a blacklist). If the variable is not
> - set at all, all protocols are enabled. The protocol names
> - currently used by git are:
> + The new way to configure allowed protocols is done through the config
> + interface, though this setting takes precedences. See
> + linkgit:git-config[1] for more details. If set, provide a
> + colon-separated list of protocols which are allowed to be used with
> + fetch/push/clone. Any protocol not mentioned will be disallowed (i.e.,
> + this is a whitelist, not a blacklist). The protocol names currently
> + used by git are:
I wonder if we can explain this in terms of the config system. Something
like:
If set to a colon-separated list of zero or more protocols, behave as
if `protocol.allow` is set to `never`, and each of the listed
protocols has `protocol.$protocol.allow` set to `always`.
> +`GIT_PROTOCOL_FROM_USER`::
> + Set to 0 to prevent protocols used by fetch/push/clone which are
> + configured to the `user` state. This is useful to restrict recursive
> + submodule initialization from an untrusted repository. See
> + linkgit:git-config[1] for more details.
Under "this is useful", it may make sense to make it clear that external
programs can use this, too. Something like:
It may also be useful for programs which feed potentially-untrusted
URLs to git commands.
> diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
> index b0917d9..5950fbf 100644
> --- a/t/lib-proto-disable.sh
> +++ b/t/lib-proto-disable.sh
> @@ -1,15 +1,12 @@
> # Test routines for checking protocol disabling.
>
> -# test cloning a particular protocol
> -# $1 - description of the protocol
> -# $2 - machine-readable name of the protocol
> -# $3 - the URL to try cloning
> -test_proto () {
> +# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
> +test_whitelist () {
> desc=$1
> proto=$2
> url=$3
>
> - test_expect_success "clone $1 (enabled)" '
> + test_expect_success "clone $desc (enabled)" '
Yeah, this should have been $desc all along. It makes the diff really
noisy, though. Should it be split out into a preparatory change?
> +# test cloning a particular protocol
> +# $1 - description of the protocol
> +# $2 - machine-readable name of the protocol
> +# $3 - the URL to try cloning
> +test_proto () {
> + test_whitelist "$@"
> +
> + test_config "$@"
> +}
This makes sense. It's probably more testing than we actually need. We
could just check the config version per-protocol, and then confirm that
GIT_ALLOW_PROTOCOL behaves as I described above for at least one
protocol. The per-protocol code paths are really just making sure that
the protocol is correctly named for each code path.
That being said, simple and stupid test setup is nice as long as it does
not take too long to run.
> diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
> index bc44ac3..75c570a 100755
> --- a/t/t5509-fetch-push-namespaces.sh
> +++ b/t/t5509-fetch-push-namespaces.sh
> @@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
> . ./test-lib.sh
>
> test_expect_success setup '
> + git config --global protocol.ext.allow user &&
> test_tick &&
> git init original &&
These remote-ext fixups might be worth a note in the commit message, or
a comment here explaining what is going on.
> +static enum protocol_allow_config get_protocol_config(const char *type)
> +{
> + char *key = xstrfmt("protocol.%s.allow", type);
> + char *value;
> +
> + if (!git_config_get_string(key, &value)) {
> + enum protocol_allow_config ret =
> + parse_protocol_config(key, value);
> + free(key);
> + free(value);
> + return ret;
> + }
> + free(key);
> +
> + /* if defined, use user default for unknown protocols */
> + if (!git_config_get_string("protocol.allow", &value)) {
> + enum protocol_allow_config ret =
> + parse_protocol_config("protocol.allow", value);
> + free(value);
> + return ret;
> + }
> +
> + /* known safe */
> [...]
It's probably worth a comment at this point in the function to follow-up
on your "if defined" comment above. So the end result reads something
like:
/* first check the per-protocol config */
...
/* now fallback to the generic config */
...
/* and then fallback to our built-in defaults */
-Peff
^ 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