* 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
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-04 23:04 UTC (permalink / raw)
To: Josh Triplett
Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King,
Stefano Zacchiroli
In-Reply-To: <20161104211959.3532uiud27nhumt7@x>
On Fri, Nov 4, 2016 at 10:19 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Fri, Nov 04, 2016 at 09:47:41PM +0100, Christian Couder wrote:
>> On Fri, Nov 4, 2016 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> >
>> > Imagine we invent a new tree entry type, "gitref", that is similar
>> > to "gitlink" in that it can record a commit object name in a tree,
>> > but unlike "gitlink" it does imply reachability. And you do not add
>> > phony parents to your commit object. A tree that has "gitref"s in
>> > it is about annotating the commits in the same repository (e.g. the
>> > tree references two commits, "base" and "tip", to point into a slice
>> > of the main history). And it is perfectly sensible for such a
>> > pointer to imply reachability---after all it serves different
>> > purposes from "gitlink".
>>
>> The more I think about this (and also about how to limit ref
>> advertisements as recently discussed in
>> https://public-inbox.org/git/20161024132932.i42rqn2vlpocqmkq@sigill.intra.peff.net/),
>> the more I think about Shawn's RefTree:
>>
>> https://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/
>>
>> Couldn't a RefTree be used to store refs that point to the base
>> commit, the tip commit and the blob that contains the cover letter,
>> and maybe also a ref pointing to the RefTree of the previous version
>> of the series?
>
> That's really interesting! The Software Heritage project is working on
> something similar, because they want to store all the refs as part of
> their data model as well. I'll point them to the reftree work.
Yeah, I know them :-) and I think I have already told Stefano
Zacchiroli about this, but I am not sure anymore.
Anyway I am CC'ing him.
> If upstream git supported RefTree, I could potentially use that for
> git-series. However, I do want a commit message and history for the
> series itself, and using refs in the reftree to refer to the parents
> seems like abusing reftree to recreate commits, in a reversal of the
> hack of using commit parents as a reftree. :)
Yeah, maybe :-) But the properties of the existing Git objects we
already use wouldn't change at all.
> What if, rather than storing a hash reference to a reftree as a single
> reference and replacing it with no history,
In what I suggest the history is kept because the new reftree has a
ref that points to the old one it is replacing.
Yeah, this reftree history maybe seen as "redundant" with the commit
history, but in my opinion this can be seen as a "feature" that will
prevent us from "mucking" too much with the commit object.
> a reftree could be
> referenced from a commit and have history? (That would also allow
> tagging a version of the reftree.)
I think that tags are already allowed to point to any kind of Git
object, so tagging a reftree should be allowed anyway if we add a
reftree object.
^ permalink raw reply
* Re: gitk: avoid obscene memory consumption
From: Stefan Beller @ 2016-11-04 22:45 UTC (permalink / raw)
To: Markus Hitter, Paul Mackerras; +Cc: git@vger.kernel.org
In-Reply-To: <47c374cf-e6b9-8cd3-ee0d-d877e9e96a62@jump-ing.de>
On Fri, Nov 4, 2016 at 12:49 PM, Markus Hitter <mah@jump-ing.de> wrote:
>
> Hello all,
+cc Paul Mackeras, who maintains gitk.
>
> 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.
Bikeshedding: I'd argue to even lower the number to 5-10k lines.
>
> 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
^ permalink raw reply
* Re: [PATCH v3] transport: add protocol policy config option
From: Stefan Beller @ 2016-11-04 22:38 UTC (permalink / raw)
To: Brandon Williams
Cc: git@vger.kernel.org, Blake Burkhart, Jeff King, Jonathan Nieder
In-Reply-To: <1478292933-7873-1-git-send-email-bmwill@google.com>
On Fri, Nov 4, 2016 at 1:55 PM, Brandon Williams <bmwill@google.com> 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.
>
> 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.
> 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.
>
> 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.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> Documentation/config.txt | 25 ++++++++
> Documentation/git.txt | 19 +++---
> git-submodule.sh | 12 ++--
> t/lib-proto-disable.sh | 132 +++++++++++++++++++++++++++++++++++----
> t/t5509-fetch-push-namespaces.sh | 1 +
> t/t5802-connect-helper.sh | 1 +
> transport.c | 73 +++++++++++++++++++++-
> 7 files changed, 235 insertions(+), 28 deletions(-)
>
> 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,
Use hyphens (`protocol.<name>.allow`) to highlight the config option.
By default, if unset, ... have a default policy ...
sounds strange. How about just dropping the first 4 words here:
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:
What happens if protocol.allow is set to true?
> + 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.
> +
> +--
> +
> +protocol.<name>.allow::
> + Set a policy to be used by protocol <name> with clone/fetch/pull commands.
How does this interact with protocol.allow?
When protocol.allow is set, this overrides the specific protocol.
If protocol is not set, it overrides the specific protocol as well(?)
> +
> pull.ff::
> By default, Git does not create an extra merge commit when merging
> a commit that is a descendant of the current commit. Instead, the
> 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
This is not the right place to mention what is newer. ;)
However it is useful to know about the config interface, which is
* (supposedly) easier to use
* more fine grained
* taking less priority than this env var.
> + test_expect_success "clone $desc (disabled)" '
> + rm -rf tmp.git &&
> + test_must_fail git clone --bare "$url" tmp.git
> + '
I could not spot a test for GIT_ALLOW_PROTOCOL overriding
any protocol*allow policy. Is that also worth testing? (for
backwards compatibility of tools that make use of GIT_ALLOW_PROTOCOL
but the user already setup a policy.
>
> void transport_check_allowed(const char *type)
> --
Thanks!
Stefan
^ permalink raw reply
* gitk: avoid obscene memory consumption
From: Markus Hitter @ 2016-11-04 19:49 UTC (permalink / raw)
To: git
Hello all,
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.
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. The patch is made against git(k) version 2.9.3, the one coming with latest Ubuntu. Please also note that this is the first time I wrote some Tcl code, so the strategy used might not follow best Tcl practices.
$ diff -uw /usr/bin/gitk.org /usr/bin/gitk
--- /usr/bin/gitk.org 2016-08-16 22:32:47.000000000 +0200
+++ /usr/bin/gitk 2016-11-04 20:06:14.805920404 +0100
@@ -7,6 +7,15 @@
# and distributed under the terms of the GNU General Public Licence,
# either version 2, or (at your option) any later version.
+# Markus: trying to limit memory consumption. It happened that
+# complex commits led to more than 1.5 GB of memory usage.
+#
+# The problem was identified to be caused by extremely long diffs. The
+# commit leading to this research had some 365'000 lines of diff, consuming
+# these 1.5 GB when drawn into the canvas. The solution is to limit diffs to
+# 50'000 lines and skipping the rest. In case of a cutoff, a CLI command for
+# getting the full diff is shown.
+
package require Tk
proc hasworktree {} {
@@ -7956,6 +7965,7 @@
proc getblobdiffs {ids} {
global blobdifffd diffids env
+ global parseddifflines
global treediffs
global diffcontext
global ignorespace
@@ -7987,6 +7997,7 @@
}
fconfigure $bdf -blocking 0 -encoding binary -eofchar {}
set blobdifffd($ids) $bdf
+ set parseddifflines 0
initblobdiffvars
filerun $bdf [list getblobdiffline $bdf $diffids]
}
@@ -8063,20 +8074,34 @@
proc getblobdiffline {bdf ids} {
global diffids blobdifffd
+ global parseddifflines
global ctext
set nr 0
+ set maxlines 50000
$ctext conf -state normal
while {[incr nr] <= 1000 && [gets $bdf line] >= 0} {
+ incr parseddifflines
+ if {$parseddifflines >= $maxlines} {
+ break
+ }
if {$ids != $diffids || $bdf != $blobdifffd($ids)} {
catch {close $bdf}
return 0
}
parseblobdiffline $ids $line
}
+ if {$parseddifflines >= $maxlines} {
+ $ctext insert end "\n------------------" hunksep
+ $ctext insert end " Lines exceeding $maxlines skipped " hunksep
+ $ctext insert end "------------------\n\n" hunksep
+ $ctext insert end "To get a full diff, run\n\n" hunksep
+ $ctext insert end " git diff-tree -p -C --cc $ids\n\n" hunksep
+ $ctext insert end "on the command line.\n" hunksep
+ }
$ctext conf -state disabled
blobdiffmaybeseehere [eof $bdf]
- if {[eof $bdf]} {
+ if {[eof $bdf] || $parseddifflines >= $maxlines} {
catch {close $bdf}
return 0
}
@@ -9093,6 +9118,7 @@
proc diffcommits {a b} {
global diffcontext diffids blobdifffd diffinhdr currdiffsubmod
+ global parseddifflines
set tmpdir [gitknewtmpdir]
set fna [file join $tmpdir "commit-[string range $a 0 7]"]
@@ -9114,6 +9140,7 @@
set blobdifffd($diffids) $fd
set diffinhdr 0
set currdiffsubmod ""
+ set parseddifflines 0
filerun $fd [list getblobdiffline $fd $diffids]
}
Cheers,
Markus
--
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-04 21:55 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, Junio C Hamano, Git mailing list
In-Reply-To: <20161104194907.3yxu2rkayfyic4dr@sigill.intra.peff.net>
On Fri, Nov 04, 2016 at 03:49:07PM -0400, Jeff King wrote:
> 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).
>
> 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.
>
> 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".
This seems like the critical problem, here. The parent hack I used in
git-series might be a hack, but it transparently works with old servers
and clients. So, for instance, I can push a git-series ref to github,
with no changes required on github's part. If git added gitrefs, and I
started using them in git-series, then that'd eliminate parent hack and
allow many standard git tools to work naturally on git-series commits
and history, but it'd also mean that people couldn't push git-series
commits to any server until that server updates git.
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
^ permalink raw reply
* [PATCH] doc: fill in omitted word
From: Kristoffer Haugsbakk @ 2016-11-04 21:43 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
---
Documentation/gitcore-tutorial.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
index 4546fa0..9860517 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -25,7 +25,7 @@ you want to understand Git's internals.
The core Git is often called "plumbing", with the prettier user
interfaces on top of it called "porcelain". You may not want to use the
plumbing directly very often, but it can be good to know what the
-plumbing does for when the porcelain isn't flushing.
+plumbing does for you when the porcelain isn't flushing.
Back when this document was originally written, many porcelain
commands were shell scripts. For simplicity, it still uses them as
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v3] transport: add protocol policy config option
From: Stefan Beller @ 2016-11-04 21:35 UTC (permalink / raw)
To: Brandon Williams
Cc: git@vger.kernel.org, Blake Burkhart, Jeff King, Jonathan Nieder
In-Reply-To: <20161104205815.GA127933@google.com>
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>
>
> --
> Brandon Williams
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-04 21:19 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, git, Shawn O. Pierce, Jeff King
In-Reply-To: <CAP8UFD2+A0MUKazAfSwCvv61TJRPuoOzH5EkqcrBOUi4TcuoDw@mail.gmail.com>
On Fri, Nov 04, 2016 at 09:47:41PM +0100, Christian Couder wrote:
> On Fri, Nov 4, 2016 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Imagine we invent a new tree entry type, "gitref", that is similar
> > to "gitlink" in that it can record a commit object name in a tree,
> > but unlike "gitlink" it does imply reachability. And you do not add
> > phony parents to your commit object. A tree that has "gitref"s in
> > it is about annotating the commits in the same repository (e.g. the
> > tree references two commits, "base" and "tip", to point into a slice
> > of the main history). And it is perfectly sensible for such a
> > pointer to imply reachability---after all it serves different
> > purposes from "gitlink".
>
> The more I think about this (and also about how to limit ref
> advertisements as recently discussed in
> https://public-inbox.org/git/20161024132932.i42rqn2vlpocqmkq@sigill.intra.peff.net/),
> the more I think about Shawn's RefTree:
>
> https://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/
>
> Couldn't a RefTree be used to store refs that point to the base
> commit, the tip commit and the blob that contains the cover letter,
> and maybe also a ref pointing to the RefTree of the previous version
> of the series?
That's really interesting! The Software Heritage project is working on
something similar, because they want to store all the refs as part of
their data model as well. I'll point them to the reftree work.
If upstream git supported RefTree, I could potentially use that for
git-series. However, I do want a commit message and history for the
series itself, and using refs in the reftree to refer to the parents
seems like abusing reftree to recreate commits, in a reversal of the
hack of using commit parents as a reftree. :)
What if, rather than storing a hash reference to a reftree as a single
reference and replacing it with no history, a reftree could be
referenced from a commit and have history? (That would also allow
tagging a version of the reftree.)
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Josh Triplett @ 2016-11-04 21:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqa8dfdt6y.fsf@gitster.mtv.corp.google.com>
On Fri, Nov 04, 2016 at 10:57:09AM -0700, Junio C Hamano wrote:
> After your talk at LPC2016, I was thinking about your proposal to
> give an option to hide certain parents from "git log" traversal.
>
> While I do not think we would terribly mind a new feature in the
> core to support third-party additions like "git series" better, I
> think this particular one is a big mistake that we shouldn't take.
[...]
> I think this is backwards. The root cause of the issue you have
> with "gitk" is because you added something that is *NOT* a parent to
> your commit. We shouldn't have to add a mechanism to filter
> something that shouldn't have been added there in the first place.
>
> I am wondering if an alternative approach would work better.
>
> Imagine we invent a new tree entry type, "gitref", that is similar
> to "gitlink" in that it can record a commit object name in a tree,
> but unlike "gitlink" it does imply reachability. And you do not add
> phony parents to your commit object. A tree that has "gitref"s in
> it is about annotating the commits in the same repository (e.g. the
> tree references two commits, "base" and "tip", to point into a slice
> of the main history). And it is perfectly sensible for such a
> pointer to imply reachability---after all it serves different
> purposes from "gitlink".
I absolutely agree with this, and I'd love to have gitref or similar in
core git. Given the availability of that mechanism, I'd love to use it
in git-series. (And in git submodule, as well, for other projects.)
The one critical issue there, though: that would break backward
compatibility with old versions of git. No old version of git could
push, pull, gc, repack, or otherwise touch a repository that used this
feature.
The advantages of the approach (viewing and manipulating the series with
pure git) seem sufficiently high to make that worth considering, but it
is a significant downside.
> Another alternative that I am negative about (but is probably a
> better hack than how you abused the "parent" link) might be to add a
> new commit object header field that behaves similarly to "parent"
> only in that it implies reachability. But recording the extra
> parent in commit object was not something you wanted to do in the
> first place (i.e. your series processing is done solely on the
> contents of the tree, and you do not read this extra parent). If you
> need to add an in-tree reference to another commit in your future
> versions of "git series", with either this variant or your original
> implementation, you would end up needing adding more "parent" (or
> pseudo parent) only to preserve reachability. At that point, I
> think it makes more sense to have entries in the tree to directly
> ensure reachability, if you want these entries to always point at an
> in-tree object.
This would similarly break compatibility with old git, as old git
wouldn't follow those reachability-only links from commits, so it could
throw away the data.
One approach compatible with old git would be to continue adding the
relevant commits as artificial parents, but have a separate commit
metadata field that says which parents to ignore; old git would then do
the right thing, as long as it doesn't rewrite the commit entirely.
That does have the same disadvantages of having to duplicate the
information in both the tree and the parent list, though; it's the same
class of hack, just with improved usability. I'd much rather use
gitrefs.
> I am afraid that I probably am two steps ahead of myself, because I
> am reasonably sure that it is quite possible that I have overlooked
> something trivially obvious that makes the "gitref" approach
> unworkable.
gitref seems like a good idea to me, as long as we can sort out the
compatibility story.
^ permalink raw reply
* Re: [PATCH v3] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-04 20:58 UTC (permalink / raw)
To: git; +Cc: sbeller, bburky, peff, jrnieder
In-Reply-To: <1478292933-7873-1-git-send-email-bmwill@google.com>
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?
--
Brandon Williams
^ permalink raw reply
* [PATCH v3] transport: add protocol policy config option
From: Brandon Williams @ 2016-11-04 20:55 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, bburky, peff, jrnieder
In-Reply-To: <1478125247-62372-1-git-send-email-bmwill@google.com>
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.
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.
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.
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.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/config.txt | 25 ++++++++
Documentation/git.txt | 19 +++---
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 132 +++++++++++++++++++++++++++++++++++----
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 73 +++++++++++++++++++++-
7 files changed, 235 insertions(+), 28 deletions(-)
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.
+
+--
+
+protocol.<name>.allow::
+ Set a policy to be used by protocol <name> with clone/fetch/pull commands.
+
pull.ff::
By default, Git does not create an extra merge commit when merging
a commit that is a descendant of the current commit. Instead, the
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:
- `file`: any local file-based path (including `file://` URLs,
or local paths)
@@ -1174,6 +1174,11 @@ of clones and fetches.
- any external helpers are named by their protocol (e.g., use
`hg` to allow the `git-remote-hg` helper)
+`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.
Discussion[[Discussion]]
------------------------
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..0a477b4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -21,14 +21,10 @@ require_work_tree
wt_prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
-# Restrict ourselves to a vanilla subset of protocols; the URLs
-# we get are under control of a remote repository, and we do not
-# want them kicking off arbitrary git-remote-* programs.
-#
-# If the user has already specified a set of allowed protocols,
-# we assume they know what they're doing and use that instead.
-: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
-export GIT_ALLOW_PROTOCOL
+# Tell the rest of git that any URLs we get don't come
+# directly from the user, so it can apply policy as appropriate.
+GIT_PROTOCOL_FROM_USER=0
+export GIT_PROTOCOL_FROM_USER
command=
branch=
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)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=$proto &&
@@ -18,7 +15,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (enabled)" '
+ test_expect_success "fetch $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -27,7 +24,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (enabled)" '
+ test_expect_success "push $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -36,7 +33,7 @@ test_proto () {
)
'
- test_expect_success "push $1 (disabled)" '
+ test_expect_success "push $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -45,7 +42,7 @@ test_proto () {
)
'
- test_expect_success "fetch $1 (disabled)" '
+ test_expect_success "fetch $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -54,7 +51,7 @@ test_proto () {
)
'
- test_expect_success "clone $1 (disabled)" '
+ test_expect_success "clone $desc (disabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=none &&
@@ -64,6 +61,119 @@ test_proto () {
'
}
+test_config () {
+ desc=$1
+ proto=$2
+ url=$3
+
+ # Test clone/fetch/push with protocol.<type>.allow config
+ test_expect_success "clone $desc (enabled with config)" '
+ rm -rf tmp.git &&
+ git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git -c protocol.$proto.allow=never clone --bare "$url" tmp.git
+ '
+
+ # Test clone/fetch/push with protocol.user.allow and its env var
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user push origin HEAD:pushed
+ )
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user fetch
+ )
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ (
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ )
+ '
+
+ # Test clone/fetch/push with protocol.allow user defined default
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git config --global protocol.allow always &&
+ git clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ git config --global protocol.allow never &&
+ test_must_fail git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git clone --bare "$url" tmp.git
+ '
+}
+
+# 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 "$@"
+}
+
# set up an ssh wrapper that will access $host/$repo in the
# trash directory, and enable it for subsequent tests.
setup_ssh_wrapper () {
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 &&
(
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index b7a7f9d..c6c2661 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -4,6 +4,7 @@ test_description='ext::cmd remote "connect" helper'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git commit --allow-empty -m initial &&
test_tick &&
diff --git a/transport.c b/transport.c
index d57e8de..c2d0538 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,79 @@ static const struct string_list *protocol_whitelist(void)
return enabled ? &allowed : NULL;
}
+enum protocol_allow_config {
+ PROTOCOL_ALLOW_NEVER = 0,
+ PROTOCOL_ALLOW_USER_ONLY,
+ PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+ const char *value)
+{
+ if (!strcasecmp(value, "always"))
+ return PROTOCOL_ALLOW_ALWAYS;
+ else if (!strcasecmp(value, "never"))
+ return PROTOCOL_ALLOW_NEVER;
+ else if (!strcasecmp(value, "user"))
+ return PROTOCOL_ALLOW_USER_ONLY;
+
+ die("unknown value for config '%s': %s", key, value);
+}
+
+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 */
+ if (!strcmp(type, "http") ||
+ !strcmp(type, "https") ||
+ !strcmp(type, "git") ||
+ !strcmp(type, "ssh") ||
+ !strcmp(type, "file"))
+ return PROTOCOL_ALLOW_ALWAYS;
+
+ /* known scary; err on the side of caution */
+ if (!strcmp(type, "ext"))
+ return PROTOCOL_ALLOW_NEVER;
+
+ /* unknown; by default let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
+
+ die("BUG: invalid protocol_allow_config type");
}
void transport_check_allowed(const char *type)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: Regarding "git log" on "git series" metadata
From: Jeff King @ 2016-11-04 19:49 UTC (permalink / raw)
To: Jacob Keller; +Cc: Junio C Hamano, Josh Triplett, Git mailing list
In-Reply-To: <CA+P7+xq0LLFBJRNNvCMQ4QR7XBg9H7NSsifiqOYqr+PUBqYRGQ@mail.gmail.com>
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).
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.
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).
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.
-Peff
^ permalink raw reply
* Re: Regarding "git log" on "git series" metadata
From: Christian Couder @ 2016-11-04 20:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Triplett, git, Shawn O. Pierce, Jeff King
In-Reply-To: <xmqqa8dfdt6y.fsf@gitster.mtv.corp.google.com>
On Fri, Nov 4, 2016 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Imagine we invent a new tree entry type, "gitref", that is similar
> to "gitlink" in that it can record a commit object name in a tree,
> but unlike "gitlink" it does imply reachability. And you do not add
> phony parents to your commit object. A tree that has "gitref"s in
> it is about annotating the commits in the same repository (e.g. the
> tree references two commits, "base" and "tip", to point into a slice
> of the main history). And it is perfectly sensible for such a
> pointer to imply reachability---after all it serves different
> purposes from "gitlink".
The more I think about this (and also about how to limit ref
advertisements as recently discussed in
https://public-inbox.org/git/20161024132932.i42rqn2vlpocqmkq@sigill.intra.peff.net/),
the more I think about Shawn's RefTree:
https://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/
Couldn't a RefTree be used to store refs that point to the base
commit, the tip commit and the blob that contains the cover letter,
and maybe also a ref pointing to the RefTree of the previous version
of the series?
^ permalink raw reply
* [PATCH 0/5] git check-ref-format --stdin --report-errors
From: Ian Jackson @ 2016-11-04 19:13 UTC (permalink / raw)
To: git
I wanted to be able to syntax check lots of proposed refs quickly
(please don't ask why - it's complicated!)
So I added a --stdin option to git-check-ref-format. Also it has
--report-errors now too so you can get some kind of useful error
message if it complains.
It's still not really a good batch mode but it's good enough for my
use case. To improve it would involve a new command line option to
offer a suitable stdout output format.
There are three small refactoring patches and the two patches with new
options and corresponding docs.
Thanks for your attention.
FYI I am not likely to need this again in the near future: it's a
one-off use case. So my effort for rework is probably limited. I
thought I'd share what I'd done in what I hope is a useful form,
anyway.
Regards,
Ian.
^ permalink raw reply
* [PATCH 4/5] check-ref-format: New --report-errors option
From: Ian Jackson @ 2016-11-04 19:13 UTC (permalink / raw)
To: git; +Cc: Ian Jackson
In-Reply-To: <20161104191358.28812-1-ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
Documentation/git-check-ref-format.txt | 8 ++++++--
builtin/check-ref-format.c | 10 ++++++++--
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index 8611a99..e9a2657 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -8,10 +8,10 @@ git-check-ref-format - Ensures that a reference name is well formed
SYNOPSIS
--------
[verse]
-'git check-ref-format' [--normalize]
+'git check-ref-format' [--report-errors] [--normalize]
[--[no-]allow-onelevel] [--refspec-pattern]
<refname>
-'git check-ref-format' --branch <branchname-shorthand>
+'git check-ref-format' [--report-errors] --branch <branchname-shorthand>
DESCRIPTION
-----------
@@ -105,6 +105,10 @@ OPTIONS
with a status of 0. (`--print` is a deprecated way to spell
`--normalize`.)
+--report-errors::
+ If any ref does not check OK, print a message to stderr.
+ (By default, git check-ref-format is silent.)
+
EXAMPLES
--------
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 020ebe8..559d5c2 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -9,7 +9,7 @@
static const char builtin_check_ref_format_usage[] =
"git check-ref-format [--normalize] [<options>] <refname>\n"
-" or: git check-ref-format --branch <branchname-shorthand>";
+" or: git check-ref-format [<options>] --branch <branchname-shorthand>";
/*
* Return a copy of refname but with leading slashes removed and runs
@@ -51,6 +51,7 @@ static int check_ref_format_branch(const char *arg)
static int normalize = 0;
static int check_branch = 0;
static int flags = 0;
+static int report_errors = 0;
static int check_one_ref_format(const char *refname)
{
@@ -61,8 +62,11 @@ static int check_one_ref_format(const char *refname)
got = check_branch
? check_ref_format_branch(refname)
: check_refname_format(refname, flags);
- if (got)
+ if (got) {
+ if (report_errors)
+ fprintf(stderr, "bad ref format: %s\n", refname);
return 1;
+ }
if (normalize) {
printf("%s\n", refname);
free((void*)refname);
@@ -87,6 +91,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
flags |= REFNAME_REFSPEC_PATTERN;
else if (!strcmp(argv[i], "--branch"))
check_branch = 1;
+ else if (!strcmp(argv[i], "--report-errors"))
+ report_errors = 1;
else
usage(builtin_check_ref_format_usage);
}
--
2.10.1
^ permalink raw reply related
* [PATCH 5/5] check-ref-format: New --stdin option
From: Ian Jackson @ 2016-11-04 19:13 UTC (permalink / raw)
To: git; +Cc: Ian Jackson
In-Reply-To: <20161104191358.28812-1-ijackson@chiark.greenend.org.uk>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
Documentation/git-check-ref-format.txt | 10 ++++++++--
builtin/check-ref-format.c | 34 +++++++++++++++++++++++++++++++---
2 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index e9a2657..5a213ce 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -10,8 +10,9 @@ SYNOPSIS
[verse]
'git check-ref-format' [--report-errors] [--normalize]
[--[no-]allow-onelevel] [--refspec-pattern]
- <refname>
-'git check-ref-format' [--report-errors] --branch <branchname-shorthand>
+ <refname> | --stdin
+'git check-ref-format' [--report-errors] --branch
+ <branchname-shorthand> | --stdin
DESCRIPTION
-----------
@@ -109,6 +110,11 @@ OPTIONS
If any ref does not check OK, print a message to stderr.
(By default, git check-ref-format is silent.)
+--stdin::
+ Instead of checking on ref supplied on the command line,
+ read refs, one per line, from stdin. The exit status is
+ 0 if all the refs were OK.
+
EXAMPLES
--------
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 559d5c2..87f52fa 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -76,6 +76,7 @@ static int check_one_ref_format(const char *refname)
int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
{
int i;
+ int use_stdin = 0;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(builtin_check_ref_format_usage);
@@ -93,6 +94,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
check_branch = 1;
else if (!strcmp(argv[i], "--report-errors"))
report_errors = 1;
+ else if (!strcmp(argv[i], "--stdin"))
+ use_stdin = 1;
else
usage(builtin_check_ref_format_usage);
}
@@ -100,8 +103,33 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
if (check_branch && (flags || normalize))
usage(builtin_check_ref_format_usage);
- if (! (i == argc - 1))
- usage(builtin_check_ref_format_usage);
+ if (!use_stdin) {
+ if (! (i == argc - 1))
+ usage(builtin_check_ref_format_usage);
+
+ return check_one_ref_format(argv[i]);
+ } else {
+ char buffer[2048];
+ int worst = 0;
- return check_one_ref_format(argv[i]);
+ if (! (i == argc))
+ usage(builtin_check_ref_format_usage);
+
+ while (fgets(buffer, sizeof(buffer), stdin)) {
+ char *newline = strchr(buffer, '\n');
+ if (!newline) {
+ fprintf(stderr, "%s --stdin: missing final newline or line too long\n", *argv);
+ exit(127);
+ }
+ *newline = 0;
+ int got = check_one_ref_format(buffer);
+ if (got > worst)
+ worst = got;
+ }
+ if (!feof(stdin)) {
+ perror("reading from stdin");
+ exit(127);
+ }
+ return worst;
+ }
}
--
2.10.1
^ permalink raw reply related
* [PATCH 2/5] check-ref-format: Refactor to make --branch code more common
From: Ian Jackson @ 2016-11-04 19:13 UTC (permalink / raw)
To: git; +Cc: Ian Jackson
In-Reply-To: <20161104191358.28812-1-ijackson@chiark.greenend.org.uk>
We are going to want to permit other options with --branch.
So, replace the special case with just an entry for --branch in the
parser for ordinary options, and check for option compatibility at the
end.
No overall functional change.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
builtin/check-ref-format.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 4d56caa..f12c19c 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)
}
static int normalize = 0;
+static int check_branch = 0;
static int flags = 0;
static int check_one_ref_format(const char *refname)
{
+ int got;
+
if (normalize)
refname = collapse_slashes(refname);
- if (check_refname_format(refname, flags))
+ got = check_branch
+ ? check_ref_format_branch(refname)
+ : check_refname_format(refname, flags);
+ if (got)
return 1;
if (normalize)
printf("%s\n", refname);
@@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(builtin_check_ref_format_usage);
- if (argc == 3 && !strcmp(argv[1], "--branch"))
- return check_ref_format_branch(argv[2]);
-
for (i = 1; i < argc && argv[i][0] == '-'; i++) {
if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], "--print"))
normalize = 1;
@@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
flags &= ~REFNAME_ALLOW_ONELEVEL;
else if (!strcmp(argv[i], "--refspec-pattern"))
flags |= REFNAME_REFSPEC_PATTERN;
+ else if (!strcmp(argv[i], "--branch"))
+ check_branch = 1;
else
usage(builtin_check_ref_format_usage);
}
+
+ if (check_branch && (flags || normalize))
+ usage(builtin_check_ref_format_usage);
+
if (! (i == argc - 1))
usage(builtin_check_ref_format_usage);
--
2.10.1
^ permalink raw reply related
* [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format
From: Ian Jackson @ 2016-11-04 19:13 UTC (permalink / raw)
To: git; +Cc: Ian Jackson
In-Reply-To: <20161104191358.28812-1-ijackson@chiark.greenend.org.uk>
We are going to want to reuse this. No functional change right now.
It currently has a hidden memory leak if --normalize is used.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
builtin/check-ref-format.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac4994..4d56caa 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -48,12 +48,22 @@ static int check_ref_format_branch(const char *arg)
return 0;
}
+static int normalize = 0;
+static int flags = 0;
+
+static int check_one_ref_format(const char *refname)
+{
+ if (normalize)
+ refname = collapse_slashes(refname);
+ if (check_refname_format(refname, flags))
+ return 1;
+ if (normalize)
+ printf("%s\n", refname);
+}
+
int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
{
int i;
- int normalize = 0;
- int flags = 0;
- const char *refname;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(builtin_check_ref_format_usage);
@@ -76,13 +86,5 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
if (! (i == argc - 1))
usage(builtin_check_ref_format_usage);
- refname = argv[i];
- if (normalize)
- refname = collapse_slashes(refname);
- if (check_refname_format(refname, flags))
- return 1;
- if (normalize)
- printf("%s\n", refname);
-
- return 0;
+ return check_one_ref_format(argv[i]);
}
--
2.10.1
^ permalink raw reply related
* [PATCH 3/5] check-ref-format: Abolish leak of collapsed refname
From: Ian Jackson @ 2016-11-04 19:13 UTC (permalink / raw)
To: git; +Cc: Ian Jackson
In-Reply-To: <20161104191358.28812-1-ijackson@chiark.greenend.org.uk>
collapse_slashes always returns a value from xmallocz.
Right now this leak is not very interesting, since we only call
check_one_ref_format once.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
builtin/check-ref-format.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index f12c19c..020ebe8 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -63,8 +63,10 @@ static int check_one_ref_format(const char *refname)
: check_refname_format(refname, flags);
if (got)
return 1;
- if (normalize)
+ if (normalize) {
printf("%s\n", refname);
+ free((void*)refname);
+ }
}
int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
--
2.10.1
^ permalink raw reply related
* Re: Regarding "git log" on "git series" metadata
From: Jacob Keller @ 2016-11-04 19:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Triplett, Git mailing list
In-Reply-To: <xmqqa8dfdt6y.fsf@gitster.mtv.corp.google.com>
On Fri, Nov 4, 2016 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I think this is backwards. The root cause of the issue you have
> with "gitk" is because you added something that is *NOT* a parent to
> your commit. We shouldn't have to add a mechanism to filter
> something that shouldn't have been added there in the first place.
>
> I am wondering if an alternative approach would work better.
>
> Imagine we invent a new tree entry type, "gitref", that is similar
> to "gitlink" in that it can record a commit object name in a tree,
> but unlike "gitlink" it does imply reachability. And you do not add
> phony parents to your commit object. A tree that has "gitref"s in
> it is about annotating the commits in the same repository (e.g. the
> tree references two commits, "base" and "tip", to point into a slice
> of the main history). And it is perfectly sensible for such a
> pointer to imply reachability---after all it serves different
> purposes from "gitlink".
>
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.
It would of course mean some work for people who previously used git
series as you would want to re-write the commits to drop the parent
links and become gitrefs instead of gitlinks. However, this can
(probably?) be solved by some sort of use of the filter-branch code.
I don't think you've hit upon any trivially obvious unworkable things.
It is probably somewhat complex to make the reachability checks detect
in-tree gitrefs but I don't think it would be impossible.
Thanks,
Jake
^ permalink raw reply
* Regarding "git log" on "git series" metadata
From: Junio C Hamano @ 2016-11-04 17:57 UTC (permalink / raw)
To: Josh Triplett; +Cc: git
After your talk at LPC2016, I was thinking about your proposal to
give an option to hide certain parents from "git log" traversal.
While I do not think we would terribly mind a new feature in the
core to support third-party additions like "git series" better, I
think this particular one is a big mistake that we shouldn't take.
For those listening from sidelines, here is a version of my
understanding of "git series":
* "git series" wants to represent a patch series evolution. It is
a history of history, and each element of this evolution is
represented by:
- a commit object, that is used to describe what this reroll of
the topic is about, and its parent links point at previous
rerolls (it could be a merge of two independent incarnations of
a series).
- the tree contained in the commit object records the base commit
where the topic forks from the main history, and the tip commit
where the topic ends. These are pointers into the main history
DAG.
- the tree may have other metadata, an example of which is the
cover letter contents to be used when the topic becomes ready
for re-submission. There may be more metadata you would want
to add in the future versions of "git series".
Needless to say, the commits that represent the history of a
series record a tree that is completely differently shaped. The
only relation between the series history and main history is that
the former has pointers into the latter.
* You chose to represent the base and tip commit object as gitlinks
in the tree of a series commit, simply because it was a way that
was already implemented to record a commit object name in a tree.
* However, because gitlink is designed to be used for "external"
things (the prominent example is submodule), recording these as
gitlinks would guarantee that they will get GCed as a series
progresses, the main history rewound and rewritten thereby making
the base and tip recorded in the older part of the series history
unreachable from the main history. Because you want to make sure
that base and tip objects will stay in the repository even after
the topic branch in the main history gets rewound, this is not
what you want.
* In order to workaround that reachability issue, the hack you
invented is to add the tip commit as a "parent" of a commit that
represents one step in the series. This may guarantee the
reachability---as long as a commit in a series history is
reachable from a ref, the tip and base commits will be reachable
from there even if they are rebased away from the main history.
But of course, there are downsides.
* Due to this hack, feeding "gitk" (or "git log") a commit in the
series history will give you nonsense results. You are not
interested in traversing or viewing the commits in the main
history.
* Because of the above, you propose another hack to tell the
revision traversal machinery to optionally omit a parent commit
that appear as a gitlink in the tree.
I think this is backwards. The root cause of the issue you have
with "gitk" is because you added something that is *NOT* a parent to
your commit. We shouldn't have to add a mechanism to filter
something that shouldn't have been added there in the first place.
I am wondering if an alternative approach would work better.
Imagine we invent a new tree entry type, "gitref", that is similar
to "gitlink" in that it can record a commit object name in a tree,
but unlike "gitlink" it does imply reachability. And you do not add
phony parents to your commit object. A tree that has "gitref"s in
it is about annotating the commits in the same repository (e.g. the
tree references two commits, "base" and "tip", to point into a slice
of the main history). And it is perfectly sensible for such a
pointer to imply reachability---after all it serves different
purposes from "gitlink".
Another alternative that I am negative about (but is probably a
better hack than how you abused the "parent" link) might be to add a
new commit object header field that behaves similarly to "parent"
only in that it implies reachability. But recording the extra
parent in commit object was not something you wanted to do in the
first place (i.e. your series processing is done solely on the
contents of the tree, and you do not read this extra parent). If you
need to add an in-tree reference to another commit in your future
versions of "git series", with either this variant or your original
implementation, you would end up needing adding more "parent" (or
pseudo parent) only to preserve reachability. At that point, I
think it makes more sense to have entries in the tree to directly
ensure reachability, if you want these entries to always point at an
in-tree object.
I am afraid that I probably am two steps ahead of myself, because I
am reasonably sure that it is quite possible that I have overlooked
something trivially obvious that makes the "gitref" approach
unworkable.
^ permalink raw reply
* Re: [PATCH] diffcore-delta: remove unused parameter to diffcore_count_changes()
From: Tobias Klauser @ 2016-11-04 16:46 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20161104163714.zwzfbmrklzwuaw6u@sigill.intra.peff.net>
On 2016-11-04 at 17:37:14 +0100, Jeff King <peff@peff.net> wrote:
> On Fri, Nov 04, 2016 at 11:24:36AM +0100, Tobias Klauser wrote:
>
> > The delta_limit parameter to diffcore_count_changes() has been unused
> > since commit c06c79667c95 ("diffcore-rename: somewhat optimized.").
> > Remove the parameter and adjust all callers.
>
> Sounds like a good idea to get rid of an unused parameter, but I think
> this went away in ba23bbc8e (diffcore-delta: make change counter to byte
> oriented again., 2006-03-04).
Ugh, I must have fat-fingered the commit id. Will update the description
accordingly for v2.
Thanks!
Tobias
^ permalink raw reply
* Re: [PATCH] branch: remove unused parameter to create_branch()
From: Tobias Klauser @ 2016-11-04 16:52 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20161104163012.5r3uivnub3bdkqgr@sigill.intra.peff.net>
On 2016-11-04 at 17:30:12 +0100, Jeff King <peff@peff.net> wrote:
> On Fri, Nov 04, 2016 at 04:19:49PM +0100, Tobias Klauser wrote:
>
> > The name parameter to create_branch() has been unused since commit
> > 55c4a673070f ("Prevent force-updating of the current branch"). Remove
> > the parameter and adjust the callers accordingly. Also remove the
> > parameter from the function's documentation comment.
>
> This seemed eerily familiar, and it turns out I wrote this as a
> preparatory step for a different topic a while back, but never finished
> it.
>
> So clearly a good change, though we might want to explain a bit more why
> it's correct that the parameter is unused. Here's what I wrote:
>
> This function used to have the caller pass in the current value of
> HEAD, in order to make sure we didn't clobber HEAD. In 55c4a6730,
> that logic moved to validate_new_branchname(), which just resolves
> HEAD itself. The parameter to create_branch is now unused.
Ah, I didn't know about the history of this parameter. It clearly makes
sense to explain this in the patch description.
> I also ended up reformatting the documentation comment, but that's
> purely optional. My patch is below for reference. Feel free to grab any
> bits of it that you agree with.
I like your documentation comment much better as IMO it's easier to read
and to identify the individual parameters.
Given these facts, I guess it's better if my patch is dropped and yours
is applied instead :)
Thanks a lot!
^ permalink raw reply
* Re: [PATCH] diffcore-delta: remove unused parameter to diffcore_count_changes()
From: Jeff King @ 2016-11-04 16:37 UTC (permalink / raw)
To: Tobias Klauser; +Cc: git, gitster
In-Reply-To: <20161104102436.23892-1-tklauser@distanz.ch>
On Fri, Nov 04, 2016 at 11:24:36AM +0100, Tobias Klauser wrote:
> The delta_limit parameter to diffcore_count_changes() has been unused
> since commit c06c79667c95 ("diffcore-rename: somewhat optimized.").
> Remove the parameter and adjust all callers.
Sounds like a good idea to get rid of an unused parameter, but I think
this went away in ba23bbc8e (diffcore-delta: make change counter to byte
oriented again., 2006-03-04).
The patch itself looks good.
-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