* RE: will git provide `submodule remove` option ?
From: Lingcha X @ 2013-02-07 5:45 UTC (permalink / raw)
To: Jens Lehmann; +Cc: git@vger.kernel.org
In-Reply-To: <5112CAD4.8060801@web.de>
Hi Jens,
`git rm -r submodule` will not do the same thing, i do NOT know if `git submodule deinit`, i will have some tests.
----
Thanks and Best Regards,
lingchax
----------------------------------------
> Date: Wed, 6 Feb 2013 22:27:48 +0100
> From: Jens.Lehmann@web.de
> To: douglarek@outlook.com
> CC: git@vger.kernel.org
> Subject: Re: will git provide `submodule remove` option ?
>
> Am 05.02.2013 11:32, schrieb Lingcha X:
> > As we all know, git provides `submodule add , init, update, sync, sumary, foreach, status`, but where is `submodule remove`?
> >
> > will
> > I not delete one of them sometime in the future? Although most people
> > will not use submodule or one who uses it can remove submodule by hand, providing complete options may be a good idea.
>
> Is assume either "git rm <submodule>" (available since 1.8.1) or the upcoming
> "git submodule deinit" (currently in pu) will do what you want?
^ permalink raw reply
* Re: [PATCH 2/3] combine-diff: suppress a clang warning
From: Miles Bader @ 2013-02-07 4:12 UTC (permalink / raw)
To: John Keeping; +Cc: Junio C Hamano, git, Antoine Pelisse
In-Reply-To: <20130203231549.GV1342@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> I generally like to get rid of the pointless warnings so that the useful
> ones can't hide in the noise. Perhaps "CFLAGS += -Wno-string-plus-int"
> would be better for this particular warning, but when there's only one
> bit of code that triggers it, tweaking that seemed simpler.
An even better approach would be to file a bug against clang ... it
really is a very ill-considered warning -- PTR + OFFS is not just
valid C, it's _idiomatic_ in C for getting interior pointers into
arrays -- and such a warning should never be enabled by default, or by
any standard warning options.
-miles
--
永日の 澄んだ紺から 永遠へ
^ permalink raw reply
* Re: [RFC/PATCH 1/4] show: obey --textconv for blobs
From: Junio C Hamano @ 2013-02-07 0:26 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
In-Reply-To: <20130207001013.GA28970@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> git-show should probably have the same option. I doubt it should be on
> by default, but I can see it being useful for:
>
> git show --to-filesystem HEAD:Makefile >Makefile
>
> or whatever. I don't think those features necessarily need to come as
> part of Michael's series.
Yeah, all makes sense (including the part that it might be useful
but it does not belong to this series).
Thanks for sanity checking.
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Jeff King @ 2013-02-07 0:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Duy Nguyen,
Michael Haggerty, Jonathan Nieder, git, Shawn Pearce
In-Reply-To: <7vmwvh9e3p.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 04:12:10PM -0800, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > I think there's a simpler way to do this, which is that:
> >
> > * New clients supporting v2 of the protocol send some piece of data
> > that would break old servers.
> >
> > * If that fails the new client goes "oh jeeze, I guess it's an old
> > server", and try again with the old protocol.
> >
> > * The client then saves a date (or the version the server gave us)
> > indicating that it tried the new protocol on that remote, tries
> > again sometime later.
>
> For that to work, the new server needs to wait for the client to
> speak first. How would that server handle old clients who expect to
> be spoken first? Wait with a read timeout (no timeout is the right
> timeout for everybody)?
If the new client can handle the old-style server's response, then the
server can start blasting out refs (optionally after a timeout) and stop
when the client interrupts with "hey, wait, I can speak the new
protocol". The server just has to include "you can interrupt me" in its
capability advertisement (obviously it would have to send out at least
the first ref with the capabilities before the timeout).
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe
From: Junio C Hamano @ 2013-02-07 0:14 UTC (permalink / raw)
To: Jeff King; +Cc: Michal Nazarewicz, Ted Zlatanov, Matthieu Moy, git
In-Reply-To: <20130206230406.GK27507@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote:
>
>> From: Michal Nazarewicz <mina86@mina86.com>
>>
>> The command_close_bidi_pipe() function will insist on closing both
>> input and output pipes returned by command_bidi_pipe(). With this
>> change it is possible to close one of the pipes in advance and
>> pass undef as an argument.
>>
>> This allows for something like:
>>
>> my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);
>> print $out "write data";
>> close $out;
>> # ... do stuff with $in
>> command_close_bidi_pipe($pid, $in, undef, $ctx);
>
> Should this part go into the documentation for command_close_bidi_pipe
> in Git.pm?
Yeah, it probably should.
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Junio C Hamano @ 2013-02-07 0:12 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Duy Nguyen, Michael Haggerty, Jonathan Nieder, git, Jeff King,
Shawn Pearce
In-Reply-To: <CACBZZX6xLvuMEhPnfYLj8W9pMLwdoS7Zb+mTtn+3DanJPiWfXw@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> I think there's a simpler way to do this, which is that:
>
> * New clients supporting v2 of the protocol send some piece of data
> that would break old servers.
>
> * If that fails the new client goes "oh jeeze, I guess it's an old
> server", and try again with the old protocol.
>
> * The client then saves a date (or the version the server gave us)
> indicating that it tried the new protocol on that remote, tries
> again sometime later.
For that to work, the new server needs to wait for the client to
speak first. How would that server handle old clients who expect to
be spoken first? Wait with a read timeout (no timeout is the right
timeout for everybody)?
> We already covered in previous discussions how this would be simpler
> with the HTTP protocol,...
Yes, that is a solved problem.
^ permalink raw reply
* Re: [RFC/PATCH 1/4] show: obey --textconv for blobs
From: Jeff King @ 2013-02-07 0:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vr4kt9f53.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 03:49:44PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > And stating it that way makes it clear that there may be other missing
> > steps (3 and up) to apply other gitattributes. For example, should "git
> > show $blob" respect crlf attributes? Smudge filters?
>
> Yeah, I think applying these when path is availble may make sense.
>
> Are we going to teach cat-file to honor them with "--textconv" and
> possibly other new options?
>
> Or should the "--textconv" imply application of these other filters?
> I am tempted to say yes, but I haven't thought things through...
For diff, we should already recognize them (because we feed the diff
machinery the path name, and it does the usual attributes lookup). Of
course it only uses things like funcname, not any of the
convert-to-filesystem options (hmm, actually, I guess it may use
convert-from-filesystem, but in such a case it would not be reading from
a blob anyway, but from a filesystem path, so it is not related to this
code).
For cat-file, I don't think --textconv should necessarily imply it. The
original motivation for "cat-file --textconv" was for external blame to
be able to access the converted contents. But it would not want to do
filesystem-level conversion; it wants the canonical version of the file.
I think a better option name for smudge/crlf would be "--to-filesystem"
or something like that. And probably it should not be the default.
git-show should probably have the same option. I doubt it should be on
by default, but I can see it being useful for:
git show --to-filesystem HEAD:Makefile >Makefile
or whatever. I don't think those features necessarily need to come as
part of Michael's series. They can wait for people who care to implement
them. But I do think the refactoring of passing along the path from the
object_context should keep in mind that we will probably want to go that
way eventually.
Are there other attributes that we might care about when showing a blob?
The only ones I can think of are the ones for converting to the
filesystem representation.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 1/4] show: obey --textconv for blobs
From: Junio C Hamano @ 2013-02-06 23:49 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
In-Reply-To: <20130206221240.GC27507@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> And stating it that way makes it clear that there may be other missing
> steps (3 and up) to apply other gitattributes. For example, should "git
> show $blob" respect crlf attributes? Smudge filters?
Yeah, I think applying these when path is availble may make sense.
Are we going to teach cat-file to honor them with "--textconv" and
possibly other new options?
Or should the "--textconv" imply application of these other filters?
I am tempted to say yes, but I haven't thought things through...
^ permalink raw reply
* Re: [PATCH] connect.c: Tell *PLink to always use ssh protocol
From: Jeff King @ 2013-02-06 23:22 UTC (permalink / raw)
To: Sven Strickroth; +Cc: msysgit, git, gitster
In-Reply-To: <5112D219.3020604@tu-clausthal.de>
On Wed, Feb 06, 2013 at 10:58:49PM +0100, Sven Strickroth wrote:
> Default values for *plink can be set using PuTTY. If a user makes
> telnet the default in PuTTY this breaks ssh clones in git.
>
> Since git clones of the type user@host:path use ssh, tell *plink
> to use ssh and override PuTTY defaults for the protocol to use.
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
Makes sense to me, though I'd expect to see this cc'd to the msysgit
list (which I'm doing on this response) for comment from people who
might be more familiar with the area.
Quoted patch follows.
-Peff
> ---
> connect.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/connect.c b/connect.c
> index 49e56ba..d337b6f 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -625,6 +625,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
> if (!ssh) ssh = "ssh";
>
> *arg++ = ssh;
> + if (putty)
> + *arg++ = "-ssh";
> if (putty && !strcasestr(ssh, "tortoiseplink"))
> *arg++ = "-batch";
> if (port) {
> --
> Best regards,
> Sven Strickroth
> PGP key id F5A9D4C4 @ any key-server
--
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
---
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
^ permalink raw reply
* Re: [PATCH 4/4] git-send-email: Use git credential to obtain password.
From: Jeff King @ 2013-02-06 23:19 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Ted Zlatanov, Matthieu Moy, git
In-Reply-To: <1d13f9fb587b8ea740cdd774173b8f63f45f86af.1360183427.git.mina86@mina86.com>
On Wed, Feb 06, 2013 at 09:47:06PM +0100, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> If smtp_user is provided but smtp_pass is not, instead of prompting
> for password, make git-send-email use git credential command
> instead.
>
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
> Documentation/git-send-email.txt | 4 +--
> git-send-email.perl | 59 +++++++++++++++++++++++-----------------
> 2 files changed, 36 insertions(+), 27 deletions(-)
Nice. I don't see anything obviously wrong with the code, but I didn't
try it myself. I wonder how hard it would be to have some tests in
t9001. It looks like we don't test the smtp code paths at all, since we
would have to implement a fake smtp server. Which probably means the
answer is is "pretty hard", unless there is an easy-to-use CPAN smtp
server module we can plug in.
-Peff
^ permalink raw reply
* Re: [PATCH 3/4] Git.pm: Add interface for git credential command.
From: Jeff King @ 2013-02-06 23:16 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Ted Zlatanov, Matthieu Moy, git
In-Reply-To: <f4b5413b0a55474346daa7b0866c7a4fed55778d.1360183427.git.mina86@mina86.com>
On Wed, Feb 06, 2013 at 09:47:05PM +0100, Michal Nazarewicz wrote:
> +sub _credential_read {
> + my %credential;
> + my ($reader, $op) = (@_);
> + while (<$reader>) {
> + chomp;
> + my ($key, $value) = /([^=]*)=(.*)/;
Empty keys are not valid. Can we make this:
/^([^=]+)=(.*)/
to fail the regex? Otherwise, I think this check:
> + if (not defined $key) {
> + throw Error::Simple("unable to parse git credential $op response:\n$_\n");
> + }
would not pass because $key would be the empty string.
> +sub _credential_write {
> + my ($credential, $writer) = @_;
> +
> + for my $key (sort {
> + # url overwrites other fields, so it must come first
> + return -1 if $a eq 'url';
> + return 1 if $b eq 'url';
> + return $a cmp $b;
> + } keys %$credential) {
> + if (defined $credential->{$key} && length $credential->{$key}) {
> + print $writer $key, '=', $credential->{$key}, "\n";
> + }
> + }
There are a few disallowed characters, like "\n" in key or value, and
"=" in a key. They should never happen unless the caller is buggy, but
should we check and catch them here?
> +In the second form, C<CODE> needs to be a reference to a subroutine.
> +The function will execute C<git credential fill> to fill provided
> +credential hash, than call C<CODE> with C<CREDENTIAL> as the sole
> +argument, and finally depending on C<CODE>'s return value execute
> +C<git credential approve> (if return value yields true) or C<git
> +credential reject> (otherwise). The return value is the same as what
> +C<CODE> returned. With this form, the usage might look as follows:
This is a nice touch. It makes the normal calling code a lot simpler.
-Peff
^ permalink raw reply
* Re: [PATCH] git-send-email: add ~/.authinfo parsing
From: Ted Zlatanov @ 2013-02-06 23:12 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, Junio C Hamano, Michal Nazarewicz, git,
Krzysztof Mazur, Michal Nazarewicz
In-Reply-To: <20130206215724.GA27507@sigill.intra.peff.net>
On Wed, 6 Feb 2013 16:57:24 -0500 Jeff King <peff@peff.net> wrote:
JK> On Wed, Feb 06, 2013 at 10:58:13AM -0500, Ted Zlatanov wrote:
MM> I don't know about the netrc credential helper, but I guess that's
MM> another layer. The git-remote-mediawiki code is the code to call the
MM> credential C API, that in turn may (or may not) call a credential
MM> helper.
>>
>> Yup. But what you call "read" and "write" are, to the credential
>> helper, "write" and "read" but it's the same protocol :) So maybe the
>> names should be changed to reflect that, e.g. "query" and "response."
JK> Is that true? As a user of the credential system, git-remote-mediawiki
JK> would want to "write" to git-credential, then "read" the response. As a
JK> helper, git-credential-netrc would want to "read" the query then
JK> "write" the response. The order is different, but the operations should
JK> be the same in both cases.
Logically they are different steps (query and response), even though the
data protocol is the same. But it's really not a big deal, I know what
it means either way.
JK> The big difference is that mediawiki would want an additional function
JK> to open a pipe to "git credential" and operate on that, whereas the
JK> helper will be reading/writing stdio.
Yup.
Ted
^ permalink raw reply
* Re: [PATCH 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe
From: Jeff King @ 2013-02-06 23:04 UTC (permalink / raw)
To: Michal Nazarewicz; +Cc: Junio C Hamano, Ted Zlatanov, Matthieu Moy, git
In-Reply-To: <c0966644278b0addbef6a03289ef9c553addf573.1360183427.git.mina86@mina86.com>
On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
>
> The command_close_bidi_pipe() function will insist on closing both
> input and output pipes returned by command_bidi_pipe(). With this
> change it is possible to close one of the pipes in advance and
> pass undef as an argument.
>
> This allows for something like:
>
> my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);
> print $out "write data";
> close $out;
> # ... do stuff with $in
> command_close_bidi_pipe($pid, $in, undef, $ctx);
Should this part go into the documentation for command_close_bidi_pipe
in Git.pm?
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Jeff King @ 2013-02-06 22:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Michael Haggerty, Jonathan Nieder, git, Shawn Pearce
In-Reply-To: <7v4nhpckwd.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 11:17:06AM -0800, Junio C Hamano wrote:
> Let me help unconfuse this thread.
>
> I think the series as 8-patch series was poorly presented, and
> separating it into two will help understanding what they are about.
>
> The first three:
>
> upload-pack: share more code
> upload-pack: simplify request validation
> upload/receive-pack: allow hiding ref hierarchies
>
> is _the_ topic of the series. As far as I am concerned (I am not
> speaking for Gerrit users, but am speaking as the Git maintainer),
> the topic is solely about uncluttering. There may be refs that the
> server end may need to keep for its operation, but that remote users
> have _no_ business knowing about. Allowing the server to keep these
> refs in the repository, while not showing these refs over the wire,
> is the problem the series solves.
>
> In other words, it is not about "these are *usually* not wanted by
> clients, so do not show them by default". It is about "these are
> not to be shown, ever".
>
> OK?
Right. I am not opposed to this series, as it does have a use-case. And
if it helps Gerrit folks or other users unclutter, great. The fact that
I could throw away the custom receive.hiderefs patch we use at GitHub is
a bonus. If people want fancier things, they can do them separately.
_But_. As a potential user of the feature (to hide refs/pull/*), I do
not think it is sufficiently flexible for me to use transfer.hiderefs
(or uploadpack.hiderefs). We use "fetch" internally to migrate objects
between forks and our alternates repos. And in that case, we really do
want to see all refs. In other words, all fetches are not the same: we
would want upload-pack to understand the difference between a client
fetch and an internal administrative fetch. But this feature does not
provide that lee-way. Even if you tried:
git fetch -u 'git -c uploadpack.hiderefs= upload-pack'
the list nature of the config variable means you cannot reset it.
This isn't a show-stopper for the series; it may just mean that it is
not a good fit for GitHub's use case, but others (like Gerrit) may
benefit. But since refs/pull is used as an example of where this could
be applied, I wanted to point out that it does not achieve that goal.
-Peff
^ permalink raw reply
* Re: [PATCH] Verify Content-Type from smart HTTP servers
From: Jeff King @ 2013-02-06 22:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Schubert, Shawn Pearce, git
In-Reply-To: <7v4nhpo2qv.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 07:56:08AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Is it worth having a strbuf_set* family of functions to match the
> > strbuf_add*? We seem to have these sorts of errors with strbuf from time
> > to time, and I wonder if that would make it easier (and more readable)
> > to do the right thing.
>
> Possibly.
>
> The callsite below may be a poor example, though; you would need the
> _reset() even if you change the _addstr() we can see in the context
> to _setstr() to make sure later strbuf_*(type) will start from a
> clean slate when !t anyway, no?
Ah, true. Let's not worry about it, then.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
From: Jeff King @ 2013-02-06 22:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vvca59j4n.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 02:23:36PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Which made me wonder: what happens with:
> >
> > git cat-file --textconv HEAD
> >
> > It looks like we die just before textconv-ing, because we have no
> > obj_context.path. But that is also unlike all of the other --textconv
> > switches, which mean "turn on textconv if you are showing a blob that
> > supports it" and not "the specific operation is --textconv, apply it to
> > this blob". I don't know if that is worth changing or not.
>
> OK, so in that sense, "cat-file --textconv HEAD" (or HEAD:) should
> die with "Hey, that is not a blob", in other words, Michael's patch
> does what we want without further tweaks, right?
Right, it will die because we do not find a path in the object_context.
For the same reason that "cat-file --textconv $sha1" would die.
A more interesting case is "cat-file --textconv HEAD:Documentation",
which does have a path, but not a blob. And I think that speaks to your
point that we want to fall-through to the pretty-print case, not the
blob case.
> By the way are we sure textconv_object() barfs and dies if fed a non
> blob? Otherwise the above does not hold, and the semantics become
> "turn on textconv if the object you are showing supports it,
> otherwise it has to be a blob.", I think.
I'm not sure. The sha1 would get passed all the way down to
fill_textconv. I think because sha1_valid is set, it will not try to
reuse the working tree file, so we will end up in
diff_populate_filespec, and we could actually textconv the tree object
itself.
So yeah, I think we want a check that makes sure we are working with a
blob before we even call that function, and "--textconv" should just be
"you must feed me a blob of the form $treeish:$path". In practice nobody
wants to do anything else anyway, so let's keep the code paths simple;
we can always loosen it later if there is a good reason to do so.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path
From: Jeff King @ 2013-02-06 22:36 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <a2808975f00bac5de0902e6830f254e4b064e8a5.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:53PM +0100, Michael J Gruber wrote:
> - add_object_array(object, arg, &list);
> + add_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context)));
If we go this route, this new _with_context variant should be used in
patch 1, too.
> @@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj
> objects[nr].item = obj;
> objects[nr].name = name;
> objects[nr].mode = mode;
> + objects[nr].context = context;
> array->nr = ++nr;
> }
This seems a little gross. Who is responsible for allocating the
context? Who frees it? It looks like we duplicate it in cmd_grep. Which
I think is OK, but it means all of this context infrastructure in
object.[ch] is just bolted-on junk waiting for somebody to use it wrong
or get confused. It does not get set, for example, by the regular
setup_revisions code path.
It would be nice if we could just always have the context available,
then setup_revisions could set it up by default (and replace the "mode"
parameter entirely). But we'd need to do something to avoid the
PATH_MAX-sized buffer for each entry, as some code paths may have a
large number of pending objects.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Ævar Arnfjörð Bjarmason @ 2013-02-06 22:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: Duy Nguyen, Michael Haggerty, Jonathan Nieder, git, Jeff King,
Shawn Pearce
In-Reply-To: <7v4nhpckwd.fsf@alter.siamese.dyndns.org>
On Wed, Feb 6, 2013 at 8:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
Maybe this should be split up into a different thread, but:
> The upload-pack-2 service sits on a port different from today's
> [...].
I think there's a simpler way to do this, which is that:
* New clients supporting v2 of the protocol send some piece of data
that would break old servers.
* If that fails the new client goes "oh jeeze, I guess it's an old
server", and try again with the old protocol.
* The client then saves a date (or the version the server gave us)
indicating that it tried the new protocol on that remote, tries
again sometime later.
We already covered in previous discussions how this would be simpler
with the HTTP protocol, since you could just send an extra header
inviting the server to speak the new protocol.
But for the other transports we can just try the new protocol and
retry with the old one as a fallback if it doesn't work. That'll allow
us to gracefully migrate without needing to change the git:// port.
Besides, I think the vast majority of users are using Git via http://
or ssh://, where we can't just change the port, but even so making
people change the port when we could handle this more gracefully would
be a big PITA. Adding new firewall holes is often a big bureaucratic
nightmare in some organizations.
^ permalink raw reply
* Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
From: Junio C Hamano @ 2013-02-06 22:23 UTC (permalink / raw)
To: Jeff King; +Cc: Michael J Gruber, git
In-Reply-To: <20130206221912.GD27507@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Which made me wonder: what happens with:
>
> git cat-file --textconv HEAD
>
> It looks like we die just before textconv-ing, because we have no
> obj_context.path. But that is also unlike all of the other --textconv
> switches, which mean "turn on textconv if you are showing a blob that
> supports it" and not "the specific operation is --textconv, apply it to
> this blob". I don't know if that is worth changing or not.
OK, so in that sense, "cat-file --textconv HEAD" (or HEAD:) should
die with "Hey, that is not a blob", in other words, Michael's patch
does what we want without further tweaks, right?
By the way are we sure textconv_object() barfs and dies if fed a non
blob? Otherwise the above does not hold, and the semantics become
"turn on textconv if the object you are showing supports it,
otherwise it has to be a blob.", I think.
^ permalink raw reply
* Re: [RFC/PATCH 3/4] grep: allow to use textconv filters
From: Jeff King @ 2013-02-06 22:23 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <da8c01b918c94c84ab61859b1b1453885bff5b06.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:52PM +0100, Michael J Gruber wrote:
> From: Jeff King <peff@peff.net>
>
> Recently and not so recently, we made sure that log/grep type operations
> use textconv filters when a userfacing diff would do the same:
>
> ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
> b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
> 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
>
> "git grep" currently does not use textconv filters at all, that is
> neither for displaying the match and context nor for the actual grepping.
>
> Introduce an option "--textconv" which makes git grep use any configured
> textconv filters for grepping and output purposes. It is off by default.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Jeff King <peff@peff.net>
I'd really love to see the refactoring I talked about in my earlier
message. But as I'm not willing to devote the time to do it right now,
and I do not think this patch has any particular bugs, I think it is OK
as it gets the job done, and does not make the later refactoring any
harder.
The one ugliness that still remains is:
> + if (opt->allow_textconv) {
> + grep_source_load_driver(gs);
> + /*
> + * We might set up the shared textconv cache data here, which
> + * is not thread-safe.
> + */
> + grep_attr_lock();
> + textconv = userdiff_get_textconv(gs->driver);
> + grep_attr_unlock();
> + }
We lock/unlock the grep_attr_lock twice here: once in
grep_source_load_driver, and then immediately again to call
userdiff_get_textconv. I don't know if it is worth doing the two under
the same lock or not (I guess it should not increase lock contention,
since we do the same amount of work, so it is really just the extra lock
instructions).
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
From: Jeff King @ 2013-02-06 22:19 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <b20e91bc71e59b5390005f2e6428e69a467e80b5.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:51PM +0100, Michael J Gruber wrote:
> When a command is supposed to use textconv filters (by default or with
> "--textconv") and none are configured then the blob is output without
> conversion; the only exception to this rule is "cat-file --textconv".
>
> Make it behave like the rest of textconv aware commands.
Makes sense.
> - if (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
> - die("git cat-file --textconv: unable to run textconv on %s",
> - obj_name);
> - break;
> + if (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
> + break;
The implication here is that textconv_object should be handling its own
errors and dying, and the return is always "yes, I converted" or "no, I
did not". Which I think is the case.
> +
> + /* otherwise expect a blob */
> + exp_type = "blob";
>
> case 0:
> if (type_from_string(exp_type) == OBJ_BLOB) {
I wondered at first why we needed to set exp_type here; shouldn't we
already be expecting a blob if we are doing textconv? But then I see
this is really about the fall-through in the switch (which we might want
an explicit comment for).
Which made me wonder: what happens with:
git cat-file --textconv HEAD
It looks like we die just before textconv-ing, because we have no
obj_context.path. But that is also unlike all of the other --textconv
switches, which mean "turn on textconv if you are showing a blob that
supports it" and not "the specific operation is --textconv, apply it to
this blob". I don't know if that is worth changing or not.
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Junio C Hamano @ 2013-02-06 22:12 UTC (permalink / raw)
To: Michael Haggerty
Cc: Duy Nguyen, Jonathan Nieder, git, Jeff King, Shawn Pearce
In-Reply-To: <5112D028.4050005@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 02/06/2013 08:17 PM, Junio C Hamano wrote:
> ...
>
> Yes, the first three patches sound much more reasonable if this is the
> goal.
> ...
> I think that a more useful interim solution would be to make it easy to
> have two URLs accessing a single git repository, with different levels
> of reference visibility applied to each.
I think you said "more reasonable" without understanding what you
are saying is "more reasonable", then. The mechanism is about
server side wanting to use refs to protect its own metadata from gc
without having to expose them; there is no "different levels".
> ...
> GIT_CONFIG=config-with-hidden-refs git upload-pack ...
> or
> git -c transfer.hiderefs=refs/pull upload-pack ...
>
> But this is a bit awkward ...
It is awkward to use hammer to drive screws in wood, too. You want
to use a screwdriver. The first three patches are to drive a nail
with hammer, OK? Screws you keep bringing up is to be handled by
delayed ref advertisement.
^ permalink raw reply
* Re: [RFC/PATCH 1/4] show: obey --textconv for blobs
From: Jeff King @ 2013-02-06 22:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git
In-Reply-To: <7vmwvhmli7.fsf@alter.siamese.dyndns.org>
On Wed, Feb 06, 2013 at 08:53:52AM -0800, Junio C Hamano wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
> > Currently, "diff" and "cat-file" for blobs obey "--textconv" options
> > (with the former defaulting to "--textconv" and the latter to
> > "--no-textconv") whereas "show" does not obey this option, even though
> > it takes diff options.
> >
> > Make "show" on blobs behave like "diff", i.e. obey "--textconv" by
> > default and "--no-textconv" when given.
>
> What does "log -p" do currently, and what should it do? Does/should
> it also use --textconv?
>
> The --textconv is a natural extension of what --ext-diff provides us,
> so I think it should trigger the same way as how --ext-diff triggers.
>
> We apply "--ext-diff" for "diff" by default but not for "log -p" and
> "show"; I suspect this may have been for a good reason but I do not
> recall the discussion that led to the current behaviour offhand.
I think Michael's commit message explains the situation badly.
--textconv is already on for "git show" (and for "git log") by default.
Diffs already use it.
This is more about the fact that when showing a single blob, we do not
bother to remember the context of the sha1 lookup, including its
pathname. Therefore we were not previously able to apply any
.gitattributes to the output. So this patch really does two things:
1. Pass the information along to show_blob_object so that it can
look up .gitattributes.
2. Apply the textconv attribute (if ALLOW_TEXTCONV is on, of course).
And stating it that way makes it clear that there may be other missing
steps (3 and up) to apply other gitattributes. For example, should "git
show $blob" respect crlf attributes? Smudge filters?
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Michael Haggerty @ 2013-02-06 22:01 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git, Jeff King, Shawn Pearce
In-Reply-To: <20130206195515.GC21003@google.com>
On 02/06/2013 08:55 PM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> [...]
>> But now every time I do a "gitk --all" or "git log --decorate", the
>> output is cluttered with all of his references (most of which are just
>> old versions of references from the upstream repository that we both
>> use). I would like to be able to hide his references most of the time
>> but turn them back on when I need them.
>>
>> Scenario 5: Our upstream repository has gazillions of release tags under
>> "refs/tags/releases/...", sometimes including customer-specific
>> releases. In my daily life these are just clutter.
>
> For both of these use cases, putting the refs somewhere other than
> refs/heads, refs/tags, and refs/remotes should be enough to avoid
> clutter.
Thanks, yes, for release tags in particular your suggestion might be
workable. But I also like the idea of being able to turn subsets of
references on and off easily, and have the choice persist until I change it.
> [...]
>> * Some small improvements (e.g. allowing *multiple* views to be
>> defined) would provide much more benefit for about the same effort,
>> and would be a better base for building other features in the future
>> (e.g., local views).
>
> Would advertising GIT_CONFIG_PARAMETERS and giving examples for server
> admins to set it in inetd et al to provide different kinds of access
> to a same repository through different URLs work?
>
>> Thanks for listening.
>> Michael
>>
>> [1] Theoretically one could support multiple views of a single
>> repository by using something like "GIT_CONFIG=view_1_config git
>> upload-pack ..." or "git -c transfer.hiderefs=... git upload-pack ...",
>> but this would be awkward.
>
> Ah, I missed this comment before. What's awkward about that? I
> think it's a clean way to make many aspects of how a repository is
> presented (including hook actions) configurable.
Awkwardness using GIT_CONFIG: the admin would have to maintain two
separate config files with mostly overlapping content.
Awkwardness using GIT_CONFIG_PARAMETERS or "-c transfer.hiderefs=...":
the hiderefs configuration would have to be maintained in some Apache
config or inetd or ... (or multiple places!) rather than in the
repository's config file, where it belongs.
Additional awkwardness using "-c transfer.hiderefs=...": AFAIK there is
no way to turn *off* a configuration variable via a command-line option.
It's all doable, but I find it awkward.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [RFC/PATCH 1/4] show: obey --textconv for blobs
From: Jeff King @ 2013-02-06 22:06 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <883f0163cb732932061a368ea9bc187c13e4ecca.1360162813.git.git@drmicha.warpmail.net>
On Wed, Feb 06, 2013 at 04:08:50PM +0100, Michael J Gruber wrote:
> diff --git a/builtin/log.c b/builtin/log.c
> index 8f0b2e8..f83870d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
> strbuf_release(&out);
> }
>
> -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev)
> +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name)
Should this maybe just take the whole object_array_entry as a cleanup?
> {
> + unsigned char sha1c[20];
> + struct object_context obj_context;
> + char *buf;
> + unsigned long size;
> +
> fflush(stdout);
> - return stream_blob_to_fd(1, sha1, NULL, 0);
> + if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
> + return stream_blob_to_fd(1, sha1, NULL, 0);
> +
> + if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context))
> + die("Not a valid object name %s", obj_name);
It seems a little hacky that we have to look up the sha1 again. What
should happen in the off chance that "hashcmp(sha1, sha1c) != 0" due to
a race with a simultaneous update of a ref?
Would it be better if object_array_entry replaced its "mode" member with
an object_context? The only downside I see is that we might waste a
significant amount of memory (each context has a PATH_MAX buffer in it).
-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