Git development
 help / color / mirror / Atom feed
* [PATCH v2] Introduce receive.denyDeletes
From: Jan Krüger @ 2008-11-01 14:42 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20081031143022.GQ14786@spearce.org>

Occasionally, it may be useful to prevent branches from getting deleted from
a centralized repository, particularly when no administrative access to the
server is available to undo it via reflog. It also makes
receive.denyNonFastForwards more useful if it is used for access control
since it prevents force-updating by deleting and re-creating a ref.

Signed-off-by: Jan Krüger <jk@jk.gs>
---
Like this, then?

 Documentation/config.txt |    4 ++++
 builtin-receive-pack.c   |   12 ++++++++++++
 t/t5400-send-pack.sh     |   11 +++++++++++
 3 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 29369d0..965ed74 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1188,6 +1188,10 @@ receive.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+receive.denyDeletes::
+	If set to true, git-receive-pack will deny a ref update that deletes
+	the ref. Use this to prevent such a ref deletion via a push.
+
 receive.denyNonFastForwards::
 	If set to true, git-receive-pack will deny a ref update which is
 	not a fast forward. Use this to prevent such an update via a push,
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 9f60f31..2c0225c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -11,6 +11,7 @@
 
 static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
 
+static int deny_deletes = 0;
 static int deny_non_fast_forwards = 0;
 static int receive_fsck_objects;
 static int receive_unpack_limit = -1;
@@ -23,6 +24,11 @@ static int capabilities_sent;
 
 static int receive_pack_config(const char *var, const char *value, void *cb)
 {
+	if (strcmp(var, "receive.denydeletes") == 0) {
+		deny_deletes = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.denynonfastforwards") == 0) {
 		deny_non_fast_forwards = git_config_bool(var, value);
 		return 0;
@@ -185,6 +191,12 @@ static const char *update(struct command *cmd)
 		      "but I can't find it!", sha1_to_hex(new_sha1));
 		return "bad pack";
 	}
+	if (deny_deletes && is_null_sha1(new_sha1) &&
+	    !is_null_sha1(old_sha1) &&
+	    !prefixcmp(name, "refs/heads/")) {
+		error("denying ref deletion for %s", name);
+		return "deletion prohibited";
+	}
 	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
 	    !is_null_sha1(old_sha1) &&
 	    !prefixcmp(name, "refs/heads/")) {
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 544771d..6fe2f87 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -103,6 +103,17 @@ unset GIT_CONFIG GIT_CONFIG_LOCAL
 HOME=`pwd`/no-such-directory
 export HOME ;# this way we force the victim/.git/config to be used.
 
+test_expect_failure \
+	'pushing a delete should be denied with denyDeletes' '
+	cd victim &&
+	git config receive.denyDeletes true &&
+	git branch extra master &&
+	cd .. &&
+	test -f victim/.git/refs/heads/extra &&
+	test_must_fail git send-pack ./victim/.git/ :extra master
+'
+rm -f victim/.git/refs/heads/extra
+
 test_expect_success \
         'pushing with --force should be denied with denyNonFastforwards' '
 	cd victim &&
-- 
1.6.0.3.524.g86cf.dirty

^ permalink raw reply related

* Re: libgit2 - a true git library
From: Pierre Habouzit @ 2008-11-01 17:01 UTC (permalink / raw)
  To: Nicolas Pitre
  Cc: Johannes Schindelin, Shawn O. Pearce, Junio C Hamano,
	Pieter de Bie, git, Scott Chacon
In-Reply-To: <alpine.LFD.2.00.0811010944000.13034@xanadu.home>

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Sat, Nov 01, 2008 at 01:50:45PM +0000, Nicolas Pitre wrote:
> On Sat, 1 Nov 2008, Pierre Habouzit wrote:
> 
> > Well, you can't return _sanely_ an error through a pointer. The &1
> > method is broken as soon as you return a char* (there is an alignment
> > requirement for malloc, not for any pointer out there), hence shall not
> > be used, as it would not be the sole way to test for error.
> > 
> > Another option, that is _theorically_ not portable, but is ttbomk on all
> > the platforms we intend to support (IOW POSIX-ish and windows), is to
> > use "small" values of the pointers for errors. [NULL .. (void *)(PAGE_SIZE - 1)[
> > cannot exist, which gives us probably always 512 different errors, and
> 
> 4095 actually.  You don't need to align error codes.

Sure, I'm just not sure there isn't an arch where a page would be 512
octets. And you really have 4096 errors, from 0 to 4095 *included* :)

> > the test is ((uintptr_t)ptr < (PAGE_SIZE)) which is cheap. It's butt
> > ugly, but encoding errors into pointers is butt ugly in the first place.
> 
> Or use "negative" pointers.  Again, please have a look at 
> include/linux/err.h.  The pointer range from 0xffffffff (or 
> 0xffffffffffffffff on 64-bit machines) down to the range you want is for 
> errors, and the top of the address range is almost certain to never be 
> valid in user space either.

Indeed.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: [PATCH 2/3] git send-email: do not ask questions when  --compose is used.
From: Pierre Habouzit @ 2008-11-01 17:08 UTC (permalink / raw)
  To: Ian Hilt; +Cc: git
In-Reply-To: <20081101130033.GD17961@sys-0.hiltweb.site>

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

On Sat, Nov 01, 2008 at 01:00:33PM +0000, Ian Hilt wrote:
> On Sat, Nov 01, 2008 at 12:04:39PM +0100, Pierre Habouzit wrote:
> > I didn't do it for a very good reason: the To field is tricker to parse
> > because very fast it's multiline, and must be split along the ',' when
> > parsed back and so on.
> 
> Right.  So my patch is broken in that it doesn't parse the addresses
> correctly.  This _should_ be easy to fix.  I knew my patch sucked, but I
> wanted to get the idea out there.  For me, I don't like specifying all
> that information on the command-line.  It would be nice to be able to
> edit the To and Cc fields in the editor.
> 
> > And even moreuseful than the To is the Cc list that git-send-email
> > bloats to death and that I would like to reduce very often.
> 
> You mean git-send-email adds too many addresses to the Cc list, or the
> code for those fields is already bloated to death?

The former, I've not looked at the code I can't really say.

> > But sadly that needs an expertise of perl I absolutely don't have. We
> > probably even want to depend on some MIME perl library that knows about
> > those kind of issues and do it for us well.
> 
> I'm confused here.  Why would a MIME library help?

Hmm maybe I'm wrong, but the idea would be to do what mutt does and be
able to parse:

To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
  Superman <batman@nyc.us>,
  "Someone with a comma, inside its tag name" <a@b.com>

And that needs to know how to do that with perl, and _really_ I hate
perl enough for not being able to do that well. Splitting on ',' is just
not going to fly.

> > But yeah, I knew I left out those, and this was the reason.
> 
> Anyway, do you, or does anyone else, think it's even worth coding the
> possibility for the user to edit the To and Cc fields?

*YES*

I would love to see git-send-email work like mutt does: it fills the
field like it does now, and you are allowed to fix that, and it parses
the buffer back to guess what you wanted. It allow to drop most of the
interactive prompting that is so annoying (since it's not in-shell and
has no history and stuff like that, unlike my $EDITOR).

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: [PATCH] Teach/Fix pull/fetch -q/-v options
From: Tuncer Ayaz @ 2008-11-01 17:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nanako Shiraishi, git
In-Reply-To: <7vprllh0ao.fsf@gitster.siamese.dyndns.org>

On Tue, Oct 28, 2008 at 4:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> Quoting Tuncer Ayaz <tuncer.ayaz@gmail.com>:
>>
>>> After fixing clone -q I noticed that pull -q does not do what
>>> it's supposed to do and implemented --quiet/--verbose by
>>> adding it to builtin-merge and fixing two places in builtin-fetch.
>>
>> Junio, may I ask what the status of this patch is? Maybe the patch was
>> lost in the noise? The commit log message is written very differently from
>> existing commits in the history of git, and I am thinking that maybe that is
>> why you did not like the whole patch? Or is it lack of any test script?
>
> Well, perhaps you've been with us long enough to get too picky like
> myself, but this was genuinely lost in the noise and my scrambling to get
> back to normal.  We do not typically say "I did this, I did that", but the
> first paragraph in Tuncer's message is perfectly fine.  I would probably
> liked the second paragraph better if it were after --- lines (it is more
> about the possible enhancements in other areas, and does not belong to the
> commit log message for this change), but it is not a grave enough offence
> to get the patch rejected.

Should I resend the patch with a short and simple commit message
like the following?
--8<--
Implement git-pull --quiet and --verbose by adding the
options to git-pull and fixing verbosity handling in git-fetch.
-->8--

> The patch itself looks Ok; the lack of test script additions does indeed
> bother me somewhat, but it is not too big a deal.

I took a look at t/ and am not quite sure whether I should test -v/-q
by analyzing stdout output's length & content.

I think testing -q and -v makes more sense on a global and not
git-pull or git-fetch level. For that to be possible I envision building
common verbosity-based logging functions/macros.

I don't like the idea of scanning stdout for length or content as
long as we're not sure that all errors and warnings are sent
to stderr and stdout is for info and verbose only.

> P.S. We are having fun at GitTogether'08 in the first half of this week,
> so please expect slower response than usual.

^ permalink raw reply

* Re: libgit2 - a true git library
From: Pierre Habouzit @ 2008-11-01 17:30 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git, Scott Chacon
In-Reply-To: <20081031184154.GV14786@spearce.org>

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Fri, Oct 31, 2008 at 06:41:54PM +0000, Shawn O. Pearce wrote:
> How about this?
> 
> http://www.spearce.org/projects/scm/libgit2/apidocs/CONVENTIONS

FWIW I've read what you say about types, while this is good design to
make things abstract, accessors are slower _and_ disallow many
optimizations as it's a function call and that it may clobber all your
pointers values.

For types that _will_ be in the tight loops, we must make the types
explicit or it'll bite us hard performance-wise. I'm thinking what is
"struct object" or "struct commit" in git.git. It's likely that we will
loose a *lot* of those types are opaque.

struct object in git has not changed since 2006.06. struct commit hasn't
since 2005.04 if you ignore { unsigned int indegree; void *util; } that
if I'm correct are annotations, and is a problem we (I think) have to
address differently anyways (I gave my proposal on this, I'm eager to
hear about what other think on the subject). So if in git.git that _is_
a moving target we have had a 2 year old implementation for those types,
it's that they're pretty well like this.

It's IMNSHO on the matter that core structures of git _will_ have to be
made explicit. I'm thinking objects and their "subtypes" (commits,
trees, blobs). Maybe a couple of things on the same vein.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: [PATCH 2/3] git send-email: do not ask questions when  --compose is used.
From: Francis Galiegue @ 2008-11-01 17:34 UTC (permalink / raw)
  To: git
In-Reply-To: <20081101170817.GC26229@artemis.corp>

Le Saturday 01 November 2008 18:08:17 Pierre Habouzit, vous avez écrit :
[...]
>
> To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
>   Superman <batman@nyc.us>,
>   "Someone with a comma, inside its tag name" <a@b.com>
>

fg@erwin ~ $ cat t.txt
To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>, 
Superman <batman@nyc.us>, "Someone with a comma, inside its tag name" 
<a@b.com>
fg@erwin ~ $ <t.txt perl -ne 's,^To:\s*,,i; @mails = m/\s*((?:"[^"]+")?
\s*<[^@]+@[^@]+>)\s*,?/g; END { print join("\n", @mails) . "\n"}'
<some.address@some.tld>
<random.joe@abc.def>
<batman@nyc.us>
"Someone with a comma, inside its tag name" <a@b.com>

That's regex, not especially perl ;)

-- 
Francis Galiegue
ONE2TEAM
Ingénieur système
Mob : +33 (0) 6 83 87 78 75
Tel : +33 (0) 1 78 94 55 52
fge@one2team.com
40 avenue Raymond Poincaré
75116 Paris

^ permalink raw reply

* Re: [PATCH 2/3] git send-email: do not ask questions when  --compose is used.
From: Pierre Habouzit @ 2008-11-01 17:43 UTC (permalink / raw)
  To: Francis Galiegue; +Cc: git
In-Reply-To: <200811011834.32702.fg@one2team.com>

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

On Sat, Nov 01, 2008 at 05:34:32PM +0000, Francis Galiegue wrote:
> Le Saturday 01 November 2008 18:08:17 Pierre Habouzit, vous avez écrit :
> [...]
> >
> > To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
> >   Superman <batman@nyc.us>,
> >   "Someone with a comma, inside its tag name" <a@b.com>
> >
> 
> fg@erwin ~ $ cat t.txt
> To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>, 
> Superman <batman@nyc.us>, "Someone with a comma, inside its tag name" 
> <a@b.com>
> fg@erwin ~ $ <t.txt perl -ne 's,^To:\s*,,i; @mails = m/\s*((?:"[^"]+")?
> \s*<[^@]+@[^@]+>)\s*,?/g; END { print join("\n", @mails) . "\n"}'
> <some.address@some.tld>
> <random.joe@abc.def>
> <batman@nyc.us>
> "Someone with a comma, inside its tag name" <a@b.com>
> 
> That's regex, not especially perl ;)

Your regex fails to parse:

"Someone with a comma, and an escape double quote \" in its name"
  <regex.cant.be.used.for.serious.parsing@i.told.you.so>

That's why I first hinted at some MIME library that has probably made
that right.

Not to mention that you don't fix the multiline issue, but that is quite
less of a problem, it merely needs an accumulator and a 1 line lookahead
in the parser code while in the header parts. Nothing unrealistic for me
to write.

But I just can't resolve myself to parse anything with regex because I
just know it's horribly broken and wrong.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: Git/Mercurial interoperability (and what about bzr?)
From: Linus Torvalds @ 2008-11-01 17:51 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Florian Weimer, Jakub Narebski, git
In-Reply-To: <20081101133931.GC8134@mit.edu>



On Sat, 1 Nov 2008, Theodore Tso wrote:
> 
> .hgtags is stored as a versioned file in Mercurial.  That's one of the
> problems, and it leads to no shortage of headaches.

I told people this was insane long long ago, and I thought the hg people 
had learnt to use local tags. They act sanely, as far as I know (ie they 
act the same way git tags do).

Of course, the problem with hg local tags is that hg apparently has no 
sane way to _propagate_ such local tag-space information from one 
repository to another. But that's purely a problem with hg itself. I don't 
know why that hasn't gotten fixed.

			Linus

^ permalink raw reply

* Re: [PATCH 2/3] git send-email: do not ask questions when  --compose is used.
From: Ian Hilt @ 2008-11-01 17:54 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: git
In-Reply-To: <20081101170817.GC26229@artemis.corp>

On Sat, 1 Nov 2008, Pierre Habouzit wrote:
> On Sat, Nov 01, 2008 at 01:00:33PM +0000, Ian Hilt wrote:
> > I'm confused here.  Why would a MIME library help?
> 
> Hmm maybe I'm wrong, but the idea would be to do what mutt does and be
> able to parse:
> 
> To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>,
>   Superman <batman@nyc.us>,
>   "Someone with a comma, inside its tag name" <a@b.com>

No, you're absolutely right for this reason: MIME-encoded addresses.

Ugh.

Although, to my knowledge, git-send-email doesn't understand that now. 
So unless we want to support MIME-encoded addresses, this won't be
necessary.

> And that needs to know how to do that with perl, and _really_ I hate
> perl enough for not being able to do that well. Splitting on ',' is just
> not going to fly.

Yea, it's going to be a bit trickier than that.

> > > But yeah, I knew I left out those, and this was the reason.
> > 
> > Anyway, do you, or does anyone else, think it's even worth coding the
> > possibility for the user to edit the To and Cc fields?
> 
> *YES*
> 
> I would love to see git-send-email work like mutt does: it fills the
> field like it does now, and you are allowed to fix that, and it parses
> the buffer back to guess what you wanted. It allow to drop most of the
> interactive prompting that is so annoying (since it's not in-shell and
> has no history and stuff like that, unlike my $EDITOR).

Hmm, I'll look into this; especially the MIME library.  I'm not a perl
monk, but I can give it a shot, no?


	Ian

^ permalink raw reply

* Re: [PATCH v2] Introduce receive.denyDeletes
From: Shawn O. Pearce @ 2008-11-01 18:07 UTC (permalink / raw)
  To: Jan Krrrger; +Cc: Junio C Hamano, git
In-Reply-To: <20081101154216.45021eee@perceptron>

Jan Krrrger <jk@jk.gs> wrote:
> Occasionally, it may be useful to prevent branches from getting deleted from
> a centralized repository, particularly when no administrative access to the
> server is available to undo it via reflog. It also makes
> receive.denyNonFastForwards more useful if it is used for access control
> since it prevents force-updating by deleting and re-creating a ref.
> 
> Signed-off-by: Jan Krüger <jk@jk.gs>
> ---
> Like this, then?

Acked-by: Shawn O. Pearce <spearce@spearce.org>
 
>  Documentation/config.txt |    4 ++++
>  builtin-receive-pack.c   |   12 ++++++++++++
>  t/t5400-send-pack.sh     |   11 +++++++++++
>  3 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 29369d0..965ed74 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1188,6 +1188,10 @@ receive.unpackLimit::
>  	especially on slow filesystems.  If not set, the value of
>  	`transfer.unpackLimit` is used instead.
>  
> +receive.denyDeletes::
> +	If set to true, git-receive-pack will deny a ref update that deletes
> +	the ref. Use this to prevent such a ref deletion via a push.
> +
>  receive.denyNonFastForwards::
>  	If set to true, git-receive-pack will deny a ref update which is
>  	not a fast forward. Use this to prevent such an update via a push,
> diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
> index 9f60f31..2c0225c 100644
> --- a/builtin-receive-pack.c
> +++ b/builtin-receive-pack.c
> @@ -11,6 +11,7 @@
>  
>  static const char receive_pack_usage[] = "git-receive-pack <git-dir>";
>  
> +static int deny_deletes = 0;
>  static int deny_non_fast_forwards = 0;
>  static int receive_fsck_objects;
>  static int receive_unpack_limit = -1;
> @@ -23,6 +24,11 @@ static int capabilities_sent;
>  
>  static int receive_pack_config(const char *var, const char *value, void *cb)
>  {
> +	if (strcmp(var, "receive.denydeletes") == 0) {
> +		deny_deletes = git_config_bool(var, value);
> +		return 0;
> +	}
> +
>  	if (strcmp(var, "receive.denynonfastforwards") == 0) {
>  		deny_non_fast_forwards = git_config_bool(var, value);
>  		return 0;
> @@ -185,6 +191,12 @@ static const char *update(struct command *cmd)
>  		      "but I can't find it!", sha1_to_hex(new_sha1));
>  		return "bad pack";
>  	}
> +	if (deny_deletes && is_null_sha1(new_sha1) &&
> +	    !is_null_sha1(old_sha1) &&
> +	    !prefixcmp(name, "refs/heads/")) {
> +		error("denying ref deletion for %s", name);
> +		return "deletion prohibited";
> +	}
>  	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
>  	    !is_null_sha1(old_sha1) &&
>  	    !prefixcmp(name, "refs/heads/")) {
> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> index 544771d..6fe2f87 100755
> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -103,6 +103,17 @@ unset GIT_CONFIG GIT_CONFIG_LOCAL
>  HOME=`pwd`/no-such-directory
>  export HOME ;# this way we force the victim/.git/config to be used.
>  
> +test_expect_failure \
> +	'pushing a delete should be denied with denyDeletes' '
> +	cd victim &&
> +	git config receive.denyDeletes true &&
> +	git branch extra master &&
> +	cd .. &&
> +	test -f victim/.git/refs/heads/extra &&
> +	test_must_fail git send-pack ./victim/.git/ :extra master
> +'
> +rm -f victim/.git/refs/heads/extra
> +
>  test_expect_success \
>          'pushing with --force should be denied with denyNonFastforwards' '
>  	cd victim &&
> -- 
> 1.6.0.3.524.g86cf.dirty

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH] connect.c: add a way for git-daemon to pass an error back to client
From: Shawn O. Pearce @ 2008-11-01 18:10 UTC (permalink / raw)
  To: Tom Preston-Werner; +Cc: Junio C Hamano, git
In-Reply-To: <b97024a40810312329o53e37fd5td82aa69634ff1e6b@mail.gmail.com>

Tom Preston-Werner <tom@github.com> wrote:
> 
> I saw several methods of testing for a specific prefix in connect.c.
> Looking more closely at the source, the closest similar call is
> actually the test for ACK:
> 
> 	if (!prefixcmp(line, "ACK ")) {
> 		if (!get_sha1_hex(line+4, result_sha1)) {
> 			if (strstr(line+45, "continue"))
> 				return 2;
> 			return 1;
> 		}
> 	}
> 
> Explicitly testing for "ERR " (including the space) does seem like the
> more correct thing to do. Would you like me to resubmit a modified
> patch that uses prefixcmp()?

Yes, I think that is what Junio was hinting at.  The pattern above
is much more typical in Git sources, so keeping the new "ERR "
check consistent would be appreciated.

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH] Documentation: add a planning document for the next CLI revamp
From: Elijah Newren @ 2008-11-01 18:36 UTC (permalink / raw)
  To: Sam Vilain; +Cc: git, Sam Vilain
In-Reply-To: <1225338485-11046-1-git-send-email-sam@vilain.net>

Hi,

Good list..  I agree with others that the 'undo' name doesn't sound
right (and will discuss other issues with it in response to another
email) but otherwise nice work.

On Wed, Oct 29, 2008 at 9:48 PM, Sam Vilain <sam@vilain.net> wrote:
> +  * 'git push' to checked out branch of non-bare repository not
> +    allowed without special configuration.  Configuration available
> +    that allows working directory to be updated, known caveats
> +    notwithstanding.  Ideally, it would refuse only in situations
> +    where a broken working copy would be left (because you couldn't
> +    fix it), and work when it can be known to be safe.

Configuration of remote repository, special command-line override, or both?


Some food for thought: One thing I did in EasyGit was to disallow
pushes to non-bare repositories* unless both source and destinations
references were explicitly specified.  For example:

$ eg push origin master    # or 'eg push', in this case
Aborting: You are trying to push to a repository with an associated working
copy, which will leave its working copy out of sync with its repository.
Rather than pushing changes to that repository, you should go to where that
repository is located and pull changes into it (using eg pull).  If you
know what you are doing and know how to deal with the consequences, you can
override this check by explicitly specifying source and destination
references, e.g.
  eg push REMOTE BRANCH:REMOTE_BRANCH
Please refer to
  eg help topic refspecs
to learn what this syntax means and what the consequences of overriding this
check are.

$ eg push origin master:master
Counting objects: 5, done.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 260 bytes, done.
Total 3 (delta 1), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /home/newren/testing/dumb/.git
   852ffee..f5596e4  master -> master


This seems to prevent errors for new users, while still allowing
people to work around firewall issues.

* The big problem was that I was only able to detect if a remote
repository was bare or not if it was accessed via the local filesystem
or via ssh; for git:// (or rsync://) repositories I didn't know how to
perform such a check and so I simply omitted it.


Elijah

^ permalink raw reply

* Re: libgit2 - a true git library
From: Andreas Ericsson @ 2008-11-01 18:44 UTC (permalink / raw)
  To: Pierre Habouzit; +Cc: Shawn O. Pearce, git, Scott Chacon
In-Reply-To: <20081101173042.GE26229@artemis.corp>

Pierre Habouzit wrote:
> On Fri, Oct 31, 2008 at 06:41:54PM +0000, Shawn O. Pearce wrote:
>> How about this?
>>
>> http://www.spearce.org/projects/scm/libgit2/apidocs/CONVENTIONS
> 
> FWIW I've read what you say about types, while this is good design to
> make things abstract, accessors are slower _and_ disallow many
> optimizations as it's a function call and that it may clobber all your
> pointers values.
> 

Accessors are very nifty for one thing though; With a debugging flag,
you can use an accessor-function, while without that debugging flag you
can use a macro instead of a function. In other words, you use the
compiler as a sort of sanity-checker that you're only accessing the
variables through the proper macros.

This method introduces a bit of extra code (50% of which is always
dead) for each struct it's used on, but it makes debugging large-ish
pieces of software relatively simple, since access to all object types
is controlled through the use of macros.

> For types that _will_ be in the tight loops, we must make the types
> explicit or it'll bite us hard performance-wise. I'm thinking what is
> "struct object" or "struct commit" in git.git. It's likely that we will
> loose a *lot* of those types are opaque.
> 

The last sentence doesn't parse. I assume you mean "if those types are..",
in which case it'll be solved by using accessor-macros and forward-declaring
the structs.

> struct object in git has not changed since 2006.06. struct commit hasn't
> since 2005.04 if you ignore { unsigned int indegree; void *util; } that
> if I'm correct are annotations, and is a problem we (I think) have to
> address differently anyways (I gave my proposal on this, I'm eager to
> hear about what other think on the subject). So if in git.git that _is_
> a moving target we have had a 2 year old implementation for those types,
> it's that they're pretty well like this.
> 
> It's IMNSHO on the matter that core structures of git _will_ have to be
> made explicit. I'm thinking objects and their "subtypes" (commits,
> trees, blobs). Maybe a couple of things on the same vein.
> 

I agree. "git_commit", "git_tree", "git_blob" and "git_tag" can almost
certainly be set in stone straight away.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

^ permalink raw reply

* [PATCH v2] connect.c: add a way for git-daemon to pass an error back to client
From: Tom Preston-Werner @ 2008-11-01 18:44 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

The current behavior of git-daemon is to simply close the connection on
any error condition. This leaves the client without any information as
to the cause of the failed fetch/push/etc.

This patch allows get_remote_heads to accept a line prefixed with "ERR"
that it can display to the user in an informative fashion. Once clients
can understand this ERR line, git-daemon can be made to properly report
"repository not found", "permission denied", or other errors.

Example

S: ERR No matching repository.
C: fatal: remote error: No matching repository.

Signed-off-by: Tom Preston-Werner <tom@github.com>
---
Use prefixcmp instead of memcmp and test for "ERR " instead of "ERR"

 connect.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/connect.c b/connect.c
index 0c50d0a..584e04c 100644
--- a/connect.c
+++ b/connect.c
@@ -70,6 +70,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		if (buffer[len-1] == '\n')
 			buffer[--len] = 0;

+		if (len > 4 && !prefixcmp(buffer, "ERR "))
+			die("remote error: %s", buffer + 4);
+
 		if (len < 42 || get_sha1_hex(buffer, old_sha1) || buffer[40] != ' ')
 			die("protocol error: expected sha/ref, got '%s'", buffer);
 		name = buffer + 41;
-- 
1.6.0.2

^ permalink raw reply related

* Re: libgit2 - a true git library
From: Pierre Habouzit @ 2008-11-01 18:48 UTC (permalink / raw)
  To: Andreas Ericsson; +Cc: Shawn O. Pearce, git, Scott Chacon
In-Reply-To: <490CA37C.1070107@op5.se>

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On Sat, Nov 01, 2008 at 06:44:12PM +0000, Andreas Ericsson wrote:
> Pierre Habouzit wrote:

> >For types that _will_ be in the tight loops, we must make the types
> >explicit or it'll bite us hard performance-wise. I'm thinking what is
> >"struct object" or "struct commit" in git.git. It's likely that we will
> >loose a *lot* of those types are opaque.
> 
> The last sentence doesn't parse. I assume you mean "if those types 
> are..",

This was a typo, indeed s/of/if/

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: libgit2 - a true git library
From: Andreas Ericsson @ 2008-11-01 19:18 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git, Scott Chacon
In-Reply-To: <20081031170704.GU14786@spearce.org>

Shawn O. Pearce wrote:
> During the GitTogether we were kicking around the idea of a ground-up
> implementation of a Git library.  This may be easier than trying
> to grind down git.git into a library, as we aren't tied to any
> of the current global state baggage or the current die() based
> error handling.
> 
> I've started an _extremely_ rough draft.  The code compiles into a
> libgit.a but it doesn't even implement what it describes in the API,
> let alone a working Git implementation.  Really what I'm trying to
> incite here is some discussion on what the API looks like.
> 
> API Docs:
> http://www.spearce.org/projects/scm/libgit2/apidocs/html/modules.html
> 
> Source Code Clone URL:
> http://www.spearce.org/projects/scm/libgit2/libgit2.git
> 

Having looked briefly at the code, I've got a couple of comments:
* GIT_EXTERN() does nothing. Ever. It's noise and should be removed.
  Instead it would be better to have GIT_PRIVATE(), which could
  set visibility to "internal" or "hidden", meaning the symbol it's
  attached to can be used for lookups when creating a shared library
  but won't be usable from programs linking to that shared library
  (visibility-attributes have zero effect on static libraries). At
  least on all archs anyone really cares about.
* Prefixing the files themselves with git_ is useless and only leads
  to developer frustration. I imagine we'd be installing any header
  files in a git/ directory anyway, so we're gaining absolutely
  nothing with the git_ prefix on source-files.

Apart from that, it seems you've been designing a lot rather than
trying to use the API to actually do something. It would, imo, be
a lot better to start development with adding functionality shared
between all programs and then expand further on that, such as
incorporating all functions needed for manipulating tags into the
library and then modify existing code to use the library to get
tag-ish things done. That would also mean that the library would
quickly get used by core git, as once a certain part of it is
complete patches can be fitted to the library rather than to the
current non-libish dying() functions.


I also think it's quite alright to not strive *too* hard to make
all functions thread-safe, as very few of them will actually need
that. It's unlikely that a user program will spawn one thread to
write a lot of tags while another is trying to parse them, for
example.

By adding an init routine that determines the workdir and the
gitdir, one could start using the library straight away.

int git_init(const char *db, const char *worktree)
{
    if (git_set_db_dir(db))
        return -1;
    git_set_worktree((worktree))
        return -1;

    return 0;
}

and already you have a some few small helpers that are nifty to
to have around:
int git_is_gitdir(const char *path);  /* returns 1 on success */
int git_has_gitdir(const char *path); /* returns 1 on success */
const char *git_mkpath(const char *fmt, ...)

This way one will notice rather quickly what's needed (making it
easy to keep a more-or-less public TODO available, with small stuff
on it for the most part), and one can then go look for it in the
existing git code and, if possible, convert stuff or, best case
scenario, steal it straight off so that more apps can benefit from
tried and tested code.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

^ permalink raw reply

* Re: [PATCH] Documentation: add a planning document for the next CLI revamp
From: Elijah Newren @ 2008-11-01 19:26 UTC (permalink / raw)
  To: Theodore Tso; +Cc: Sam Vilain, git, Sam Vilain
In-Reply-To: <20081030143918.GB14744@mit.edu>

Hi,

On Thu, Oct 30, 2008 at 8:39 AM, Theodore Tso <tytso@mit.edu> wrote:
> Here are my favorites:
>
> * Add the command "git revert-file <files>" which is syntactic sugar for:
>
>        git checkout HEAD -- <files>
>
>  Rationale: Many other SCM's have a way of undoing local edits to a
>  file very simply, i.e."hg revert <file>" or "svn revert <file>", and
>  for many developers's workflow, it's useful to be able to undo local
>  edits to a single file, but not to everything else in the working
>  directory.  And "git checkout HEAD -- <file>" is rather cumbersome
>  to type, and many beginning users don't find it intuitive to look in
>  the "git-checkout" man page for instructions on how to revert a
>  local file.

I agree with the rationale, but the suggested implementation (as with
the original suggestion for "git undo") is somewhat problematic.  I
have a write-up somewhere documenting the ways various individual git
commands fail to be an appropriate replacement for svn/hg/bzr
revert[1], but in short the "git checkout HEAD -- <file>"
implementation for svn/hg/bzr-like revert fails in the following ways:
  * It does not work for the initial commit
  * It won't untrack or remove files (this is related to the previous
and following items)
  * It doesn't allow reverting a file or directory to a revision prior
to HEAD (making it like svn; note though that both bzr and hg have
such an option and I have found it handy a few times)
  * It's inappropriate to use during an incomplete merge.

The incomplete merge case is particularly interesting.  If the user
specifies a file or subdirectory, they should also specify a branch to
revert relative to (and it should be an error if they don't).  If the
user specifies "." then there's the question of whether they are
attempting to undo the merge (meaning that .git/MERGE_MSG and
.git/MERGE_HEAD should be removed).

Just as food for thought, here's what eg does in the incomplete merge case:

$ eg revert foo
Aborting: Cannot revert the changes since the last commit, since you are in
the middle of a merge and there are multiple last commits.  Please add
  --since BRANCH
to your flags to eg revert, where BRANCH is one of
  master, devel
If you simply want to abort your merge and undo its conflicts, run
  eg revert --since HEAD


There's a couple more issues here that I could go on about, but I'll
mention just one more thing for this email:  Since users often get
confused between different kinds of "reverting" or "undoing", a plain
'eg revert' is also pretty helpful in a wide variety of circumstances
(it always aborts with an error message, but one that detects what the
user might want and suggests appropriate commands in the various
cases.)

Elijah


[1] There are a number of different commands that people suggest for
new users to replace other systems' revert behavior, but each has
areas in which it will fail to do what users expect or do additional
things users don't want (including discarding data)  Interestingly,
I've tried four different alternative git porcelains and each one
implemented their svn/hg/bzr-like revert incorrectly.  One of these
was EasyGit, in which I got it wrong not once but three separate
times.  (And if alternative porcelain authors can't easily get it
right, we clearly can't expect normal users to know how to do so; I
think this is a pretty good argument for providing a function for this
behavior in core git.)  I think I finally have it implemented
correctly now in EasyGit, after my fourth try...

^ permalink raw reply

* git/lib and git/git-gui/lib merge mis-hap?
From: Andreas Ericsson @ 2008-11-01 19:30 UTC (permalink / raw)
  To: Junio C Hamano, Shawn Pearce, Git Mailing List

Settling down to get some libgit2 hacking done (adding build-rules
to git.git), I noticed that there's a file in git.git called
lib/remote_add.tcl, which looks as if it belongs in git-gui/lib.

I don't know how this happened, but since I assume it's subtree
merged (thus requiring more work to correct than a simple patch),
it would be nifty if it could get corrected so as to make space
for the up-and-coming git library :-)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

^ permalink raw reply

* Re: [PATCH] Documentation: add a planning document for the next CLI revamp
From: Elijah Newren @ 2008-11-01 19:42 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Theodore Tso, Sam Vilain, git
In-Reply-To: <vpqmygmw1mr.fsf@bauges.imag.fr>

Hi,

On Thu, Oct 30, 2008 at 9:20 AM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> Theodore Tso <tytso@mit.edu> writes:
>
>> * Add the command "git revert-file <files>" which is syntactic sugar for:
>>
>>         git checkout HEAD -- <files>
>
> I don't think "revert-file" is a good name for this: although other
> SCM often call this "revert", what Git calls "revert" is about
> reverting an existing commit (it's "backout" in hg for example). The
> terminology to revert the working tree to the last commited version is
> already here in Git, it's "reset".

git's reset is _not_ the same as svn/bzr/hg's revert; and it's overlap
in functionality is smaller than you realize.

> I've already argued in favor of allowing "git reset --hard <files>",
> which is consistant with existing terminology and doesn't add an extra
> command, but without success.

Such a command would
  * delete newly added files instead of simply untracking them.
That's the right thing for reset, but not for a svn/bzr/hg-like revert
(and unfortunately means unintended data loss.)
  * doesn't make sense during an incomplete merge, unless you add some
way of specifying which branch users want to revert their files back
to.  Aren't there enough confusing flags for reset already?  (One
surprisingly common comment I get about eg is that git reset is too
hard to understand and eg reset fixes it -- despite the fact that I
merely renamed two flags and hid the second form of the command.)
  * doesn't work for the initial commit
  * provides no way to revert files or subdirectories back to their
state at some previous revision (hg and bzr revert have a flag to
provide this; it's not useful all the time but is on occasion)  Also,
if users try to modify their command slightly to get such behavior,
say 'git reset --hard REVISION' instead of 'git reset --hard .', then
they're in trouble.  (Recoverable, but they need an 'expert' to help
them now.)


Also, there's three different kinds of "undo": switching to an old
revision (git checkout REVISION, or svn/hg update -r, etc.),
forgetting or discarding commits/merges/rebases (git reset), and
modifying files to undo previous modifications without touching HEAD
(svn/bzr/hg revert).  Users get confused enough between these
different kinds of undo; overloading them further would be really bad,
IMO.

(Part of the reason for users getting confused between these kinds of
undo is the fact that git doesn't implemented svn/bzr/hg-like revert,
and tends to steer them towards git checkout and git reset, which are
commands typically meant for the *other* kinds of undo.)


Just my $0.02,
Elijah

^ permalink raw reply

* Git remote status
From: Gonsolo @ 2008-11-01 19:52 UTC (permalink / raw)
  To: git

Hi!

If I switch branches with "git checkout master" git tells me something 
like "Your branch is ahead of the tracked remote branch 'origin/master' 
by 39 commits".
Is there a "git remote status" or git-status switch to get the same 
information without switching branches?
Sometimes it's valuable whether one should push changes (for example 
before installing a new Ubuntu version ;) ).

Thank you,

g

^ permalink raw reply

* Re: [PATCH 2/3] git send-email: do not ask questions when  --compose is used.
From: Francis Galiegue @ 2008-11-01 19:56 UTC (permalink / raw)
  To: git
In-Reply-To: <20081101174352.GG26229@artemis.corp>

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

Le Saturday 01 November 2008 18:43:52 Pierre Habouzit, vous avez écrit :
[...]

> Your regex fails to parse:
>
> "Someone with a comma, and an escape double quote \" in its name"

Easy fix: replace "[^"]+" with "[^"]+(?:\\"[^"]*)*".

>   <regex.cant.be.used.for.serious.parsing@i.told.you.so>

Oh yes. Regexes _are_ the way to do serious parsing. All MIME packages you 
will find floating around use regexes to parse mail headers correctly.

Granted, adhering to the RFC822 to the letter is rather hard. But I have a 
sample program here that can not only parse the escaped double quote, but 
also take account for the multiple line stuff and multiple headers of the 
same type where email addresse are valid (To:, Cc:, Bcc:). See attachment. 
Feel free to use the code.

----

fg@erwin ~ $ cat t.txt
To: John Doe <some.address@some.tld>, Random Joe <random.joe@abc.def>, 
Superman <batman@nyc.us>, "Someone with a comma, inside its tag name" 
<a@b.com>
To: bbr@one2team.com,
 u1@whatever.org,
  u2@wherever.ru,
   u3@blah.com
fg@erwin ~ $ perl t.pl <t.txt
Found mail: John Doe <some.address@some.tld>
Found mail: Random Joe <random.joe@abc.def>
Found mail: Superman <batman@nyc.us>
Found mail: "Someone with a comma, inside its tag name" <a@b.com>
Found mail: bbr@one2team.com
Found mail: u1@whatever.org
Found mail: u2@wherever.ru
Found mail: u3@blah.com
----


-- 
fge

[-- Attachment #2: t.pl --]
[-- Type: application/x-perl, Size: 1101 bytes --]

^ permalink raw reply

* Re: [PATCH] Documentation: add a planning document for the next CLI revamp
From: Elijah Newren @ 2008-11-01 19:57 UTC (permalink / raw)
  To: Andreas Ericsson; +Cc: Theodore Tso, Sam Vilain, git, Sam Vilain
In-Reply-To: <4909CC85.1080803@op5.se>

Hi,

On Thu, Oct 30, 2008 at 9:02 AM, Andreas Ericsson <ae@op5.se> wrote:
> I like it, although I guess one would have to add a "--staged" flag to
> git revert-file to be able to checkout files from index as well, or people
> will wonder why that can't be done.

Ew.  'git revert-file --staged foo'?  If you want to revert the
*unstaged* changes of a file, it should be 'git revert-file --unstaged
foo'.
I would expect 'git revert-file --staged foo' to revert the staged
changes in foo, i.e. it should do what 'git reset -- foo' does (except
that it should also work for the initial commit).  Thus, there'd be
little need for a --staged flag to revert-file, unless we allowed
reverting individual files back to some revision prior to HEAD (like
bzr and hg do)...

^ permalink raw reply

* Re: git reset --hard w/o touching every file
From: Edward Z. Yang @ 2008-11-01 20:03 UTC (permalink / raw)
  To: git
In-Reply-To: <20081101110529.GC3819@artemis.corp>

Pierre Habouzit wrote:
> git checkout HEAD -- <list of the files>

What if I do not know a priori which files *do* need to be updated? Is
there a command that I can get this information from? Also, I may not
necessarily be checking out HEAD.

^ permalink raw reply

* Re: Git remote status
From: Miklos Vajna @ 2008-11-01 20:11 UTC (permalink / raw)
  To: Gonsolo; +Cc: git
In-Reply-To: <490CB390.9000206@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

On Sat, Nov 01, 2008 at 08:52:48PM +0100, Gonsolo <gonsolo@gmail.com> wrote:
> Is there a "git remote status" or git-status switch to get the same 
> information without switching branches?

Just 'git checkout' should print the info for the current branch.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply

* Re: libgit2 - a true git library
From: Johannes Schindelin @ 2008-11-01 20:26 UTC (permalink / raw)
  To: Nicolas Pitre
  Cc: Pierre Habouzit, Shawn O. Pearce, Junio C Hamano, Pieter de Bie,
	git, Scott Chacon
In-Reply-To: <alpine.LFD.2.00.0811010944000.13034@xanadu.home>

Hi,

On Sat, 1 Nov 2008, Nicolas Pitre wrote:

> Again, please have a look at include/linux/err.h.  The pointer range 
> from 0xffffffff (or 0xffffffffffffffff on 64-bit machines) down to the 
> range you want is for errors, and the top of the address range is almost 
> certain to never be valid in user space either.

How certain may I be of that assumption?  Because an assumption it is.

Ciao,
Dscho

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox