Git development
 help / color / mirror / Atom feed
* Re: [PATCH] setup.py: fix error message when running with python-2.3
From: Karl Hasselström @ 2008-07-12  5:19 UTC (permalink / raw)
  To: Miklos Vajna; +Cc: Catalin Marinas, Thomas Rast, Petr Baudis, Git Mailing List
In-Reply-To: <1215806972-18713-1-git-send-email-vmiklos@frugalware.org>

On 2008-07-11 22:09:31 +0200, Miklos Vajna wrote:

> When setup.py tries to check the python version, the check actually
> won't give a usable error message but it'll raise a SyntaxError. Fix
> this by not using generator expressions.

> -    pyver = '.'.join(str(n) for n in sys.version_info)
> +    pyver = '.'.join(map(lambda x: str(x), sys.version_info))

Thanks. And by not using a list comprehension, you stay compatible
with pre-2.0 versions as well (though I guess we'd need to test that,
since there may be other 2.0 things we rely on in this code path).

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository
From: Junio C Hamano @ 2008-07-12  3:44 UTC (permalink / raw)
  To: Petr Baudis; +Cc: git
In-Reply-To: <20080711150707.GE32184@machine.or.cz>

Thanks.  Queued on 'maint'.

^ permalink raw reply

* Re: [JGIT PATCH 1/1] jgit: create a tag command
From: Shawn O. Pearce @ 2008-07-12  3:42 UTC (permalink / raw)
  To: Mike Ralphson; +Cc: Robin Rosenberg, Marek Zawirski, git
In-Reply-To: <20080712030104.GB15838@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Mike Ralphson <mike.ralphson@gmail.com> wrote:
> 
> > > I think we are at the point where we need to either write a
> > > #@!*(!@(! command line option parser, import one, or stop writing
> > > command line programs.  I would certainly appreciate any opinion
> > > you might have on the matter.
> > 
> > a) is a distraction, c) is a backwards step, so maybe b) wins.
> 
> So I looked at GNU getopt and its at least smaller than Apache
> Commons,

Probably the state-of-the-arg is args4j:

  https://args4j.dev.java.net/

It uses Java 5 annotations to setup the argument parsing:

  public class SampleMain {

    @Option(name="-r",usage="recursively run something")
    private boolean recursive;

    @Option(name="-o",usage="output to this file",metaVar="OUTPUT")
    private File out = new File(".");

    @Option(name="-str")        // no usage
    private String str = "(default value)";
  ...

I'm usually not a big fan of reflection, but it may make sense to
take advantage of it in a case like this, and just import args4j
into our command line tools.

args4j is provided under the MIT license.

-- 
Shawn.

^ permalink raw reply

* Re: [PATCH 3/3 FIXED] help (Windows): Display HTML in default browser using Windows' shell API
From: Junio C Hamano @ 2008-07-12  3:26 UTC (permalink / raw)
  To: Steffen Prohaska; +Cc: git, Johannes Schindelin, Johannes Sixt
In-Reply-To: <1215761822-21356-1-git-send-email-prohaska@zib.de>

Steffen Prohaska <prohaska@zib.de> writes:

> diff --git a/compat/mingw.c b/compat/mingw.c
> index 3a05fe7..0ca73f7 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1017,3 +1017,24 @@ sig_handler_t mingw_signal(int sig, sig_handler_t handler)
> ...
> +void mingw_open_html(const char *unixpath)
> +{
> +	const char *htmlpath = make_backslash_path(unixpath);
> +	printf("Launching default browser to display HTML ...\n");
> +	ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
> +}

Do you mean to have that printf() or is it a leftover debugging statement?

^ permalink raw reply

* Re: [JGIT PATCH 1/1] jgit: create a tag command
From: Shawn O. Pearce @ 2008-07-12  3:01 UTC (permalink / raw)
  To: Mike Ralphson; +Cc: Robin Rosenberg, Marek Zawirski, git
In-Reply-To: <e2b179460807110145n4182978awd3aa3f97c3caac95@mail.gmail.com>

Mike Ralphson <mike.ralphson@gmail.com> wrote:
> 2008/7/11 Shawn O. Pearce <spearce@spearce.org>:
> > Mike Ralphson <mike.ralphson@gmail.com> wrote:
> >>
> >> Loving the make_jgit stuff.
> >
> > So making jgit a single stand-alone, portable shell script for
> > command line usage was a good idea?  ;-)
> 
> It certainly seems so to me. It's a nice quick way of seeing what's
> implemented (I was toying with adding a jgit help command which would
> reflect over the TextBuiltins).

Yea, sadly our builtins don't register themselves into a table so its
a bit difficult to enumerate them at this time.  But it probably would
not be a difficult thing to change at all, and since we don't have a
lot of them yet it wouldn't be that much work to do the conversion.
 
> I'm not sure which if any platforms would eventually be better off
> with a commandline jgit than trying to port c-git though.

Not all platforms can compile c git, e.g. they are missing a
c compiler, but have a JRE handy.  Odd, I know, but sometimes
that's how it goes.  Or maybe you have a JRE handy, and want to
just download the single stand-alone jgit jar to get some sort of a
basic git implementation available, so you can clone and build the
real thing direct from Junio's sources.

Or maybe you are using a transport (Amazon S3) that C Git just
doesn't support.  :-)

> > I think we are at the point where we need to either write a
> > #@!*(!@(! command line option parser, import one, or stop writing
> > command line programs.  I would certainly appreciate any opinion
> > you might have on the matter.
> 
> a) is a distraction, c) is a backwards step, so maybe b) wins.

I don't disagree, I thought the very same thing myself.
 
> I don't know what the state of the art of Java option parsers is but
> there is a port of GNU getopt [1] which might drop in quite easily.

So I looked at GNU getopt and its at least smaller than Apache
Commons, and doesn't have this damned-if-you-do, damned-if-you-do
version selection choice they offer their end-users.  But it really
makes building something like an automatic --help difficult if not
impossible, and certainly makes handling numeric vs. ObjectId vs.
String vs. File arguments annoying.

I'd almost rather just do a).  It wouldn't take long to get something
small rolled out and working.  We don't need to support every goddamn
weird syntax that C git supports for backward compatability reasons.

> PS apologies for the patch format, I'm stuck with Outlook or gmail,
> and a recalcitrant firewall for the moment.

Some people are terrified of that system of tubes.  I feel for you.
Day-job is that way.  :-(
 
> [1] http://www.urbanophile.com/arenn/hacking/download.html

-- 
Shawn.

^ permalink raw reply

* Re: [RFC/PATCH (WIP)] Git.pm: Add get_config() method and related subroutines
From: Petr Baudis @ 2008-07-12  1:47 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: git, Lea Wiemann
In-Reply-To: <200807100133.38163.jnareb@gmail.com>

On Thu, Jul 10, 2008 at 01:33:36AM +0200, Jakub Narebski wrote:
> On Wed, Jul 09, 2008, Petr "Pasky" Baudis wrote:
> > On Thu, Jul 03, 2008 at 06:24:53PM +0200, Jakub Narebski wrote:
> > >  * Should config_val_to_bool and config_val_to_int throw error or
> > >    just return 'undef' on invalid values?  One can check if variable
> > >    is defined using "exists($config_hash{'varname'})".
> > 
> >   I think that it's more reasonable to throw an error here (as long as
> > you don't throw an error on undef argument). This particular case is
> > clearly a misconfiguration by the user and you rarely need to handle
> > this more gracefully, I believe.
> 
> If we follow git-config convention (and I guess we should), it would be
> value of appropriate type if variable value is of appropriate type,
> 'undef' (no output in the case of git-config) when variable does not 
> exists, and throwing error (status and "fatal: ..." message on STDERR
> in the case of git-config) if variable is not of given type.

Yes, this seems to be in agreement with what I suggested.

> > >  * What should ->get_config() have as an optional parameter:
> > >    PREFIX (/^$prefix/o), or simply SECTION (/^(?:$section)\./o)?
> > 
> >   Do we even _need_ a parameter like that? I don't understand what is
> > this interface trying to address.
> 
> For example if one wants to access _all_ variables in gitweb.* section
> (or in gitcvs.* section), and _only_ config variables in given section.

But what is the practical benefit? Does anyone use a config file long
enough that this makes any difference?

Or if you mean foreach use, it's trivial to

	foreach (grep { /^gitweb\./ } keys %$config)

or provide a method of Git::Config that will return this list, but it
does not seem appropriate to have specific Git::Config instances for
this. (Besides, if the script also needs to access a single variable
from a different section, it will need to re-parse the config again.)

So I think your approach would be good only if there are multiple
methods of Git::Config that would operate on the whole config and needed
a way to be restricted; is that the case?

> BTW. what should non-typecasting should be named? $c->get(<VAR>), 
> $c->value(<VAR>), $c->param(<VAR>), or something yet different?

I would prefer 'get' since it's the shortest and most clear, but 'value'
would be fine too, I suppose (and more in line with bool etc.).

> > Also, having accessors for special types lets you return undef when
> > the type really isn't defined, instead of e.g. true with current
> > config_val_bool, which is clearly bogus and requires complicated code
> > on the user side. 
>  
> I don't follow you.  Didn't we agree on casting an error when variable
> is not of given type?

Sorry, s/type really/variable really/. According to your original code,

	config_val_bool(undef)

would return true, while the undef could be from both non-existent and
unassigned variable. (This 'unassigned variables' case is really
annoying to handle.)

> > -- 
> > 				Petr "Pasky" Baudis
> > The last good thing written in C++ was the Pachelbel Canon. 
> >                                                  -- J. Olson 
> 
> Eh? Isn't it written C# rather?

Well, the quote is a bit dated and changing it would be too problematic.
;-)

-- 
				Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce

^ permalink raw reply

* Re: Git call graph
From: Miklos Vajna @ 2008-07-12  1:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd4ljj4u1.fsf@gitster.siamese.dyndns.org>

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

On Fri, Jul 11, 2008 at 05:58:46PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > Here is the result:
> >
> > http://vmiklos.hu/pic/git-call-graph.png
> 
> Your pull does not seem to call either fetch nor merge...  Strange.

My bad. I tracked shell calls using a wrapper around 'git' so it missed
calls without the 'remove-dashes' patches I posted yesterday.

http://vmiklos.hu/pic/git-call-graph-v2.png

This one now shows that.

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

^ permalink raw reply

* [PATCHv4] Fix backwards-incompatible handling of core.sharedRepository
From: Petr Baudis @ 2008-07-12  1:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Heikki Orsila
In-Reply-To: <7vhcawhs1e.fsf@gitster.siamese.dyndns.org>

06cbe85 (Make core.sharedRepository more generic, 2008-04-16) broke the
traditional setting of core.sharedRepository to true, which was to make
the repository group writable: with umask 022, it would clear the
permission bits for 'other'. (umask 002 did not exhibit this behaviour
since pre-chmod() check in adjust_shared_perm() fails in that case.)

The call to adjust_shared_perm() should only loosen the permission.
If the user has umask like 022 or 002 that allow others to read, the
resulting files should be made readable and writable by group, without
restricting the readability by others.

This patch fixes the adjust_shared_perm() mode tweak based on Junio's
suggestion and adds the appropriate tests to t/t1301-shared-repo.sh.

Cc: Heikki Orsila <heikki.orsila@iki.fi>
Signed-off-by: Petr Baudis <pasky@suse.cz>
---

  Fourth iteration. The problem is in fact triggered with umask 022,
not 002; fixed the description and extended the testsuite appropriately.
Improved patch description as well as the adjust_shared_perm() fix based
on Junio's suggestions here and on #git, so by now it's more of his
patch than mine. :-)

 path.c                 |    2 +-
 t/t1301-shared-repo.sh |   22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)


diff --git a/path.c b/path.c
index 5983255..504eae0 100644
--- a/path.c
+++ b/path.c
@@ -272,7 +272,7 @@ int adjust_shared_perm(const char *path)
 		int tweak = shared_repository;
 		if (!(mode & S_IWUSR))
 			tweak &= ~0222;
-		mode = (mode & ~0777) | tweak;
+		mode |= tweak;
 	} else {
 		/* Preserve old PERM_UMASK behaviour */
 		if (mode & S_IWUSR)
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 6c78c8b..4c6d0b6 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -17,6 +17,28 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	test $ret != "0"
 '
 
+for u in 002 022; do
+	test_expect_success "shared=1 does not clear bits preset by umask $u" '
+		mkdir sub &&
+		cd sub &&
+		umask $u &&
+		git init --shared=1 &&
+		test 1 = $(git config core.sharedrepository) &&
+		actual="$(ls -l .git/HEAD)" &&
+		cd .. &&
+		rm -rf sub &&
+		case "$actual" in
+		-rw-rw-r--*)
+			: happy
+			;;
+		*)
+			echo Oops, .git/HEAD is not 0664 but $actual
+			false
+			;;
+		esac
+	'
+done
+
 test_expect_success 'shared=all' '
 	mkdir sub &&
 	cd sub &&

^ permalink raw reply related

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Linus Torvalds @ 2008-07-12  1:07 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Junio C Hamano, Steffen Prohaska, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <alpine.LFD.1.10.0807111653500.2875@woody.linux-foundation.org>




On Fri, 11 Jul 2008, Linus Torvalds wrote:
> 
> If it's something that should be merged, and if it concerns code that I'm 
> interested in, I want to know about it. It's that simple.

Btw, an example of where I think we need to look at both windows and unix 
behavior and not try to make them two different camps is in that
"start_command()" thing.

It was changed to have a totally separate __MINGW32__ part, but the thing 
is, the unix side could really be improved - and actually made more like 
the MINGW32 code at the same time!

For example, on many systems it is rather noticeably faster to use 
"vfork+execve" than it is to do "fork+execve", because you avoid a whole 
"duplicate and tear down page tables" sequence. So the UNIX code would 
actually be better off using "vfork()" instead of "fork()" there.

But it can't right now - because if "cmd->env" changes the environment, it 
would change it both in the caller and in the result.

It turns out that Windows has the exact same issue (because it uses a 
spawn thing), and already does a "copy_env() + change-in-copy + free" 
model for that reason.

If that was shared, the UNIX side could just use vfork, I believe. In 
fact, the following trivial - but horribly ugly - patch passes all the 
tests, by doing the vfork() in all cases except when the environment 
changes. But I don't know what coverage that has, though (maybe env is 
effectively always set?).

And I suspect there are other cases where we'd actually be better off 
trying to share things than having all the differences hidden away in 
compat layers.

			Linus

---
 builtin-grep.c |    2 +-
 run-command.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index ef29910..5d3053a 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -168,7 +168,7 @@ static int exec_grep(int argc, const char **argv)
 	int status;
 
 	argv[argc] = NULL;
-	pid = fork();
+	pid = vfork();
 	if (pid < 0)
 		return pid;
 	if (!pid) {
diff --git a/run-command.c b/run-command.c
index 6e29fdf..200ba7b 100644
--- a/run-command.c
+++ b/run-command.c
@@ -68,7 +68,7 @@ int start_command(struct child_process *cmd)
 	trace_argv_printf(cmd->argv, "trace: run_command:");
 
 #ifndef __MINGW32__
-	cmd->pid = fork();
+	cmd->pid = cmd->env ? fork() : vfork();
 	if (!cmd->pid) {
 		if (cmd->no_stdin)
 			dup_devnull(0);

^ permalink raw reply related

* Git call graph
From: Miklos Vajna @ 2008-07-12  0:47 UTC (permalink / raw)
  To: git

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

Hi,

I just did this for fun, if you are busy, you can stop reading now,
this mail is not about anything important. :-)

I thought it would worth a try to make a graph about git calls, when a
script calls an other git program. I tracked two types of calls:

- when a bultin calls an other builtin (like git reset calls
  git read-tree)

- when a script calls a builtin (like git merge-resolve calls
  git write-tree)

Here is the result:

http://vmiklos.hu/pic/git-call-graph.png

The code that was used to generate the graph is available at the 'graph'
branch of git://repo.or.cz/git/vmiklos.git.

If you check out that branch, you can simply type 'make' to generate the
picture, or you can read some minimal documentation (README).

And now I should stop playing with it, in fact it took much more time
than I expected. ;-)

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

^ permalink raw reply

* Re: [PATCHv3] Fix backwards-incompatible handling of core.sharedRepository
From: Junio C Hamano @ 2008-07-12  0:20 UTC (permalink / raw)
  To: Petr Baudis; +Cc: git, Heikki Orsila
In-Reply-To: <20080711234257.16449.85447.stgit@rover.dkm.cz>

Petr Baudis <pasky@suse.cz> writes:

> The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
> handling extension broke backwards compatibility; before, shared=1 meant
> that Git merely ensured the repository is group-writable, not that it's
> _only_ group-writable, which is the current behaviour. Thus, with umask 002,
> Git creates repository with /rw.rw.--./, this patch fixes it back to
> /rw.rw.r-./.

Is it just me who finds the above unreadable blob of black ink?

	06cbe85 (Make core.sharedRepository more generic, 2008-04-16)
	broke the traditional setting core.sharedRepository to true,
        which was to make the repository group writable.

	The call to adjust_shared_perm() should only loosen the
	permission.  If the user has umask 002 (or 022) that allow others
	to read, the resulting files should be made readable and writable
	by group, without restricting the readability by others.

> Maybe it makes sense to provide the current semantics in some way too,
> but that cannot be done at the expense of ditching backwards
> compatibility; this bug has just wasted me two hours and broke
> repo.or.cz pushing for several hours.

I do not think this gripe after the semicolon belongs before the
three-dash lines.

> Cc: Heikki Orsila <heikki.orsila@iki.fi>
> Signed-off-by: Petr Baudis <pasky@rover.dkm.cz>
> ---
>
>   Oops, this testcase wouldn't really remove its test subrepository
> after itself properly, though the testsuite would still pass; of course
> this bug slipped through all the previous visual inspections of mine.
>
>   Sorry for the continuous noise. :-)

Hmm, I am very puzzled.

I am afraid I have to ask you for another round.  I applied only your
update to t/t1301 without the change to path.c, and the test passes, which
means either (1) the test is incorrect and does not exposing the breakage,
or (2) the breakage is not real and existing code is correct.

^ permalink raw reply

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Johannes Schindelin @ 2008-07-12  0:06 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Junio C Hamano, Steffen Prohaska, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <alpine.LFD.1.10.0807111653500.2875@woody.linux-foundation.org>

Hi,

On Fri, 11 Jul 2008, Linus Torvalds wrote:

> On Sat, 12 Jul 2008, Johannes Schindelin wrote:
> > 
> > But we are talking about 4msysgit.git, no?
> 
> I'm not. I'm just talking about Windows-related changes in git in 
> general, that are expected to be merged into regular git one way or 
> another.
>
> [...]
> 
> But I'd certainly _hope_ that future windows work is incremental, and at 
> that point it's no longer a "drop the end result on people" situation.

Fair enough.  You'll get all the crap now, I expect.

Seriously again, I really hope that things do not change very much.  Even 
if the occasional patch will now come in directly to git@vger.

Ciao,
Dscho

^ permalink raw reply

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Linus Torvalds @ 2008-07-11 23:58 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Junio C Hamano, Steffen Prohaska, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <alpine.DEB.1.00.0807120043150.8950@racer>




On Sat, 12 Jul 2008, Johannes Schindelin wrote:
> 
> But we are talking about 4msysgit.git, no?

I'm not. I'm just talking about Windows-related changes in git in 
general, that are expected to be merged into regular git one way or 
another.

If it's something that should be merged, and if it concerns code that I'm 
interested in, I want to know about it. It's that simple.

The fact that _all_ windows discussion used to be in a different area and 
not on the -git list is past. It was definitely the case that it didn't 
affect any normal git code, since it was all maintained in a separate tree 
and the normal git tree was simply not even _relevant_, and didn't even 
try to be.

But that's changed. I think the windows support merge was really quite 
well done, and was a rather clean series, and no, I don't think the unix 
people cared about it when there was so much fundamental stuff needed to 
be done - we didn't have any relevant input.

But I'd certainly _hope_ that future windows work is incremental, and at 
that point it's no longer a "drop the end result on people" situation.

		Linus

^ permalink raw reply

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Johannes Schindelin @ 2008-07-11 23:47 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Junio C Hamano, Steffen Prohaska, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <alpine.LFD.1.10.0807111638130.3459@woody.linux-foundation.org>

Hi,

On Fri, 11 Jul 2008, Linus Torvalds wrote:

> On Sat, 12 Jul 2008, Johannes Schindelin wrote:
> > 
> > I think that is a perfect example, since Hannes worked on it in 
> > mingw.git.  AFAIR a few comments came through msysGit, and were 
> > incorporated.  When everything was good for a first go at git@vger, it 
> > was sent, and the interface finalized.
> 
> Umm. Dscho - that was before the thing was merged.
> 
> Now that the basic mingw support is part of standard git, the situation 
> has changed.
> 
> That's the main issue here - if mingw support is in standard git (and it 
> is), then mingw issues that touch any non-mingw code should be discussed 
> where all the git developers are.
> 
> Can't you see the difference between the pre-merge and the post-merge 
> situation?

Sure I can.

But we are talking about 4msysgit.git, no?

At least the patches that Steffen sent were all from 4msysgit.git, and for 
some reason or other not necessary for Hannes' mingw.git.

We are talking about stuff like that putty thing, where people feel it 
would be a better idea to avoid scripts, at the cost of a higher 
maintenance burden.

We are talking about patches that were necessary a long time ago, but are 
no longer, and we should have that sorted out on the msysGit list before 
sending them to a list where many people could not care less for Windows, 
and are probably annoyed to even read as much as _this_ thread about it, 
let alone be bothered by patches that turn out to be stale in the first 
place.

_That_ is what I am arguing should be sorted out, at least the early 
stages, on msysGit.  As we did in the past.

And that worked out rather well so far, do you disagree?

Ciao,
Dscho

^ permalink raw reply

* [PATCHv3] Fix backwards-incompatible handling of core.sharedRepository
From: Petr Baudis @ 2008-07-11 23:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Heikki Orsila
In-Reply-To: <20080711233841.30916.75885.stgit@rover.dkm.cz>

The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
handling extension broke backwards compatibility; before, shared=1 meant
that Git merely ensured the repository is group-writable, not that it's
_only_ group-writable, which is the current behaviour. Thus, with umask 002,
Git creates repository with /rw.rw.--./, this patch fixes it back to
/rw.rw.r-./.

Maybe it makes sense to provide the current semantics in some way too,
but that cannot be done at the expense of ditching backwards
compatibility; this bug has just wasted me two hours and broke
repo.or.cz pushing for several hours.

Cc: Heikki Orsila <heikki.orsila@iki.fi>
Signed-off-by: Petr Baudis <pasky@rover.dkm.cz>
---

  Oops, this testcase wouldn't really remove its test subrepository
after itself properly, though the testsuite would still pass; of course
this bug slipped through all the previous visual inspections of mine.

  Sorry for the continuous noise. :-)

 path.c                 |    2 +-
 t/t1301-shared-repo.sh |   20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletions(-)


diff --git a/path.c b/path.c
index 5983255..75c5915 100644
--- a/path.c
+++ b/path.c
@@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)
 	mode = st.st_mode;
 
 	if (shared_repository) {
-		int tweak = shared_repository;
+		int tweak = (mode & 0777) | shared_repository;
 		if (!(mode & S_IWUSR))
 			tweak &= ~0222;
 		mode = (mode & ~0777) | tweak;
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 6c78c8b..3fe485c 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -17,6 +17,26 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	test $ret != "0"
 '
 
+test_expect_success 'shared=1 does not override sane umask' '
+	mkdir sub &&
+	cd sub &&
+	umask 002 &&
+	git init --shared=1 &&
+	test 1 = $(git config core.sharedrepository) &&
+	actual="$(ls -l .git/HEAD)" &&
+	cd .. &&
+	rm -rf sub &&
+	case "$actual" in
+	-rw-rw-r--*)
+		: happy
+		;;
+	*)
+		echo Oops, .git/HEAD is not 0664 but $actual
+		false
+		;;
+	esac
+'
+
 test_expect_success 'shared=all' '
 	mkdir sub &&
 	cd sub &&

^ permalink raw reply related

* Re: [PATCH] Add pretty format %aN which gives the author name, respecting .mailmap
From: Johannes Schindelin @ 2008-07-11 23:42 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: gitster, Git Mailinglist, David Symonds
In-Reply-To: <bd6139dc0807111630j306f0225m90b501296a508552@mail.gmail.com>

Hi,

On Sat, 12 Jul 2008, Sverre Rabbelier wrote:

> On Sat, Jul 12, 2008 at 1:28 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > The pretty format %an does not respect .mailmap, but gives the exact 
> > author name recorded in the commit.  Sometimes it is more desirable, 
> > however, to look if the email has another name mapped to it in 
> > .mailmap.
> >
> > This commit adds %aN (and %cN for the committer name) to do exactly 
> > that.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> Whoah, that's fast ;).

Heh.  The price is that my patches are usually more buggy than other 
contributors' patches.

> I'm not sure what to do though, if I use this new %cN GitStats will
> only work with the latest git version... :(

Yes, that is correct.  But my impression was that GitStats was never meant 
as a pure add-on, but rather some integral part of Git, no?  IOW at least 
contrib/ stuff.

Ciao,
Dscho

^ permalink raw reply

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Linus Torvalds @ 2008-07-11 23:40 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Junio C Hamano, Steffen Prohaska, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <alpine.DEB.1.00.0807120033490.8950@racer>




On Sat, 12 Jul 2008, Johannes Schindelin wrote:
> 
> I think that is a perfect example, since Hannes worked on it in mingw.git.  
> AFAIR a few comments came through msysGit, and were incorporated.  When 
> everything was good for a first go at git@vger, it was sent, and the 
> interface finalized.

Umm. Dscho - that was before the thing was merged.

Now that the basic mingw support is part of standard git, the situation 
has changed.

That's the main issue here - if mingw support is in standard git (and it 
is), then mingw issues that touch any non-mingw code should be discussed 
where all the git developers are.

Can't you see the difference between the pre-merge and the post-merge 
situation?

		Linus

^ permalink raw reply

* [PATCHv2] Fix backwards-incompatible handling of core.sharedRepository
From: Petr Baudis @ 2008-07-11 23:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Heikki Orsila

The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
handling extension broke backwards compatibility; before, shared=1 meant
that Git merely ensured the repository is group-writable, not that it's
_only_ group-writable, which is the current behaviour. Thus, with umask 002,
Git creates repository with /rw.rw.--./, this patch fixes it back to
/rw.rw.r-./.

Maybe it makes sense to provide the current semantics in some way too,
but that cannot be done at the expense of ditching backwards
compatibility; this bug has just wasted me two hours and broke
repo.or.cz pushing for several hours.

Cc: Heikki Orsila <heikki.orsila@iki.fi>
Signed-off-by: Petr Baudis <pasky@rover.dkm.cz>
---

  The patch description is clarified and a test is added to t1301.

 path.c                 |    2 +-
 t/t1301-shared-repo.sh |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletions(-)


diff --git a/path.c b/path.c
index 5983255..75c5915 100644
--- a/path.c
+++ b/path.c
@@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)
 	mode = st.st_mode;
 
 	if (shared_repository) {
-		int tweak = shared_repository;
+		int tweak = (mode & 0777) | shared_repository;
 		if (!(mode & S_IWUSR))
 			tweak &= ~0222;
 		mode = (mode & ~0777) | tweak;
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 6c78c8b..d26191b 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -17,6 +17,25 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	test $ret != "0"
 '
 
+test_expect_success 'shared=1 does not override sane umask' '
+	mkdir sub &&
+	cd sub &&
+	umask 002 &&
+	git init --shared=1 &&
+	test 1 = $(git config core.sharedrepository) &&
+	actual="$(ls -l .git/HEAD)" &&
+	rm -rf sub &&
+	case "$actual" in
+	-rw-rw-r--*)
+		: happy
+		;;
+	*)
+		echo Oops, .git/HEAD is not 0664 but $actual
+		false
+		;;
+	esac
+'
+
 test_expect_success 'shared=all' '
 	mkdir sub &&
 	cd sub &&

^ permalink raw reply related

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Johannes Schindelin @ 2008-07-11 23:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Steffen Prohaska, Linus Torvalds, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <7vmykohvfy.fsf@gitster.siamese.dyndns.org>

Hi,

On Fri, 11 Jul 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > And if the separation is not always that clear, why not discuss those 
> > things on msysGit first, and then come to git@vger with our minds (and 
> > possibly our patches) made up?
> 
> I know you mean well, but my impression from reading the kernel list is 
> that often a message that says "we already debated this issue on our 
> list to death and here is our conclusion, take it or leave it" to the 
> audience is not accepted warmly, and more central kernel people may even 
> say "do not hide the discussion in the subsystem list, do it in the 
> open, discussion on the subsystem list does not matter".

I never meant on insisting that we find the final solution on the msysGit 
list, and I would _never_ suggest that anything sent to git@vger is final.  
It is always subject to discussion.

Anyway, enough said (since I think that you and me are in agreement here),
Dscho

^ permalink raw reply

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Johannes Schindelin @ 2008-07-11 23:35 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Linus Torvalds, Steffen Prohaska, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <7v4p6wjcgm.fsf@gitster.siamese.dyndns.org>


Hi,

On Fri, 11 Jul 2008, Junio C Hamano wrote:

> Another example that has already happened was our move away from direct 
> use of fork/exec but abstracting it out to run_command() layer.  This 
> would not have settled in a shape usable by both Windows and POSIX if 
> people from both camps did not participate in the design and review.

I think that is a perfect example, since Hannes worked on it in mingw.git.  
AFAIR a few comments came through msysGit, and were incorporated.  When 
everything was good for a first go at git@vger, it was sent, and the 
interface finalized.

And I agree that it is a pretty beautiful interface.

Ciao,
Dscho

^ permalink raw reply

* Re: [GitStats] Bling bling or some statistics on the git.git repository
From: Johannes Schindelin @ 2008-07-11 23:33 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: Git Mailinglist, David Symonds
In-Reply-To: <bd6139dc0807111550w1f4a9e5fl636df73572bd27de@mail.gmail.com>

Hi,

On Sat, 12 Jul 2008, Sverre Rabbelier wrote:

> On Sat, Jul 12, 2008 at 12:07 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >> On Fri, Jul 11, 2008 at 11:22 PM, Johannes Schindelin Yeah, I wish 
> >> 'git log -C -C -M --numstat --sacrifice-chicken --pretty=format:%ae 
> >> --' would take care of that... That is, a git-blame like mechanism 
> >> that would detect such moves on a per-commit basis and report them 
> >> would be very useful to me.
> >
> > Well, the chicken (or better, a goat) should be sacrificed by you...  
> > The option I would call "--code-moves".
> 
> If you suggest I write up a patch to 'git log' I am afraid that would 
> require quite a bit more skill && knowledge of 'git log' than I have 
> (which is about Null :P).

If you were suspecting that I would write the patch once the semantics are 
finalized, you would be absolutely correct.

> > But the semantics of that need to be sorted out in a shell script 
> > first; maybe like I outlined (if that was not coherent, please say 
> > so).
> 
> Python is one big shell script :P, so if you meant that it should be 
> part of GitStats (instead of part of 'git log', which I commented on 
> above), python would be just fine :). The concept was clear enough 
> though, I think I understand what you mean.

Fair enough.  As long as you use a language that is easy to prototype 
quickly and dirtily in.  Such as Python.

> >> Very much so, but the former I figure can be easily done with 'git 
> >> log -C -C -M' I discovered (I need to parse it's output though, and 
> >> also determine what to do with moves statistics wise. Should changes 
> >> made due to moves just be ignored?)
> >
> > That is not very interesting, as we often move so small parts (think 
> > "one function") that -C -C -M does not trigger.
> 
> Right, why aim for the stuff when there's much more interesting fun out 
> there? If there was a --code-moves I agree with you that it would be a 
> lot more interesting to have than going with the current approach and 
> throwing in '-C -C -M'.

Let's go for it, then!

> >> That sounds interesting, I won't need to actually do that though, I 
> >> already have a diff parser that gives me the lines added VS lines 
> >> deleted on a hunk-by-hunk basis. If it is a true move (e.g., code 
> >> removed in file X and added in file Y) it should be trivial to detect 
> >> that.
> >>
> >> Something along the lines of:
> >>  for hunk in added:
> >>   if hunk in deleted:
> >>     print("Over here!!")
> >
> > I think that is not enough, as a code move can mean that part of a
> > function was refactored into a function.  The consequence is often a
> > reindent, and possibly rewrapping.
> 
> Mhhh, such would be beyond the scope of implementing manually indeed,
> and should be left to the likes of a diff tool instead in order to
> prevent reinventing the wheel :).

That is why I was suggesting using the diff tool with munged input to find 
out what works best.

When that is done, I'll turn it into C.

> > BTW I realized something else: your 
> > http://alturin.googlepages.com/full_activity.txt lists only 
> > "gitk-git/po/es.po" under git-git/po/.  And it has as many added as 
> > deleted lines.
> 
> Correct, that's because that is what 'git log' tells me.

I suspect that one big "git log" will not tell you enough.  You probably 
need to make your tool aware (at least a little) about merges, just as you 
probably made it aware about parent/child relationships (to track the 
changes along renames)...

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH] Add pretty format %aN which gives the author name, respecting .mailmap
From: Sverre Rabbelier @ 2008-07-11 23:30 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: gitster, Git Mailinglist, David Symonds
In-Reply-To: <alpine.DEB.1.00.0807120027330.8950@racer>

On Sat, Jul 12, 2008 at 1:28 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> The pretty format %an does not respect .mailmap, but gives the exact
> author name recorded in the commit.  Sometimes it is more desirable,
> however, to look if the email has another name mapped to it in .mailmap.
>
> This commit adds %aN (and %cN for the committer name) to do exactly that.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Whoah, that's fast ;).
I'm not sure what to do though, if I use this new %cN GitStats will
only work with the latest git version... :(

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* [PATCH] Add pretty format %aN which gives the author name, respecting .mailmap
From: Johannes Schindelin @ 2008-07-11 23:28 UTC (permalink / raw)
  To: Sverre Rabbelier, gitster; +Cc: Git Mailinglist, David Symonds
In-Reply-To: <alpine.DEB.1.00.0807120000580.8950@racer>


The pretty format %an does not respect .mailmap, but gives the exact
author name recorded in the commit.  Sometimes it is more desirable,
however, to look if the email has another name mapped to it in .mailmap.

This commit adds %aN (and %cN for the committer name) to do exactly that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sat, 12 Jul 2008, Johannes Schindelin wrote:

	> On Sat, 12 Jul 2008, Sverre Rabbelier wrote:
	> 
	> > On Sat, Jul 12, 2008 at 12:11 AM, Johannes Schindelin
	> > <Johannes.Schindelin@gmx.de> wrote:
	> > > The mechanism is this: you look up the email in .mailmap 
	> > > (actually you parse that once, but the idea stays the same), and if 
	> > > there is a name for it, you use that _instead of_ the given author 
	> > > name.
	> > 
	> > Ah, so you suggest changing "format:%ae" to "format:%ae %an" 
	> > and falling back to the latter id specified on that line if the 
	> > former is not in .mailmap? That would work I guess, I'll put it on my 
	> > TODO list :).
	> 
	> Hmm.  I missed the fact that you used pretty formats.  Seems 
	> like %an does not respect .mailmap; I'm on it.

 Documentation/pretty-formats.txt |    2 ++
 pretty.c                         |   27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 69e6d2f..c11d495 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -101,6 +101,7 @@ The placeholders are:
 - '%P': parent hashes
 - '%p': abbreviated parent hashes
 - '%an': author name
+- '%aN': author name (respecting .mailmap)
 - '%ae': author email
 - '%ad': author date
 - '%aD': author date, RFC2822 style
@@ -108,6 +109,7 @@ The placeholders are:
 - '%at': author date, UNIX timestamp
 - '%ai': author date, ISO 8601 format
 - '%cn': committer name
+- '%cN': committer name (respecting .mailmap)
 - '%ce': committer email
 - '%cd': committer date
 - '%cD': committer date, RFC2822 style
diff --git a/pretty.c b/pretty.c
index 8eb39e9..862364b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -3,6 +3,8 @@
 #include "utf8.h"
 #include "diff.h"
 #include "revision.h"
+#include "path-list.h"
+#include "mailmap.h"
 
 static char *user_format;
 
@@ -288,6 +290,25 @@ static char *logmsg_reencode(const struct commit *commit,
 	return out;
 }
 
+static int mailmap_name(struct strbuf *sb, const char *email)
+{
+	static struct path_list *mail_map;
+	char buffer[1024];
+
+	if (!mail_map) {
+		mail_map = xcalloc(1, sizeof(*mail_map));
+		read_mailmap(mail_map, ".mailmap", NULL);
+	}
+
+	if (!mail_map->nr)
+		return -1;
+
+	if (!map_email(mail_map, email, buffer, sizeof(buffer)))
+		return -1;
+	strbuf_addstr(sb, buffer);
+	return 0;
+}
+
 static size_t format_person_part(struct strbuf *sb, char part,
                                const char *msg, int len)
 {
@@ -309,10 +330,12 @@ static size_t format_person_part(struct strbuf *sb, char part,
 	if (end >= len - 2)
 		goto skip;
 
-	if (part == 'n') {	/* name */
+	if (part == 'n' || part == 'N') {	/* name */
 		while (end > 0 && isspace(msg[end - 1]))
 			end--;
-		strbuf_add(sb, msg, end);
+		if (part != 'N' || !msg[end] || !msg[end + 1] ||
+				mailmap_name(sb, msg + end + 2) < 0)
+			strbuf_add(sb, msg, end);
 		return placeholder_len;
 	}
 	start = ++end; /* save email start position */
-- 
1.5.6.2.509.g109edf

^ permalink raw reply related

* Re: Should we discuss Windows-related changes on git@vger.kernel.org?
From: Junio C Hamano @ 2008-07-11 23:06 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Steffen Prohaska, Linus Torvalds, Johannes Sixt, msysGit,
	Git Mailing List
In-Reply-To: <alpine.DEB.1.00.0807112226190.8950@racer>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> And if the separation is not always that clear, why not discuss those 
> things on msysGit first, and then come to git@vger with our minds (and 
> possibly our patches) made up?

I know you mean well, but my impression from reading the kernel list is
that often a message that says "we already debated this issue on our list
to death and here is our conclusion, take it or leave it" to the audience
is not accepted warmly, and more central kernel people may even say "do
not hide the discussion in the subsystem list, do it in the open,
discussion on the subsystem list does not matter".

Taking that lesson from the other community, I'd suggest that it is
probably a good idea to start discussion on your list, but once (some)
people in the discussion think the topic is releavant here, move it here
sooner rather than later.

^ permalink raw reply

* Re: [GitStats] Bling bling or some statistics on the git.git repository
From: Johannes Schindelin @ 2008-07-11 23:02 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: Git Mailinglist, David Symonds
In-Reply-To: <bd6139dc0807111514j75d1ae6dl3c3f5dbfb55961c7@mail.gmail.com>

Hi,

On Sat, 12 Jul 2008, Sverre Rabbelier wrote:

> On Sat, Jul 12, 2008 at 12:11 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > The mechanism is this: you look up the email in .mailmap (actually you
> > parse that once, but the idea stays the same), and if there is a name for
> > it, you use that _instead of_ the given author name.
> 
> Ah, so you suggest changing "format:%ae" to "format:%ae %an" and
> falling back to the latter id specified on that line if the former is
> not in .mailmap? That would work I guess, I'll put it on my TODO list
> :).

Hmm.  I missed the fact that you used pretty formats.  Seems like %an does 
not respect .mailmap; I'm on it.

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