Git development
 help / color / mirror / Atom feed
* Re: [PATCH] Add more instructions about how to install git.
From: Thiago Farina @ 2009-09-08 14:52 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Matthieu Moy, Alex Riesen, git
In-Reply-To: <4AA66C54.4060101@viscovery.net>

On Tue, Sep 8, 2009 at 10:38 AM, Johannes Sixt<j.sixt@viscovery.net> wrote:
> Thiago Farina schrieb:
>> I tried to being more explicit about what the new users has to do. The
>> first instruction that the text says is: "Normaly you can just do
>> "make"".
>> But the new user, cloning git won't have a configure file, so he can't
>> just do "make".
>>
>> And then, the text says: "so you can write instead". This brings me to
>> one alternative (not what I really should do), when in fact it should
>> be (I guess) the first instructions that new users need to follow.
>
> Then guessed wrong. You really only have to say "make". The use of
> autoconf and ./configure is purely optional and not necessary.
I don't think so.
Running only "make".
$ make
GIT_VERSION = 1.6.5.rc0
    * new build flags or prefix
    CC fast-import.o
In file included from builtin.h:4,
                 from fast-import.c:143:
git-compat-util.h:129:25: error: openssl/ssl.h: No such file or directory
git-compat-util.h:130:25: error: openssl/err.h: No such file or directory
In file included from builtin.h:6,
                 from fast-import.c:143:
cache.h:8:21: error: openssl/sha.h: No such file or directory
In file included from fast-import.c:152:
csum-file.h:10: error: expected specifier-qualifier-list before ‘SHA_CTX’
fast-import.c: In function ‘create_index’:
fast-import.c:850: error: ‘SHA_CTX’ undeclared (first use in this function)
fast-import.c:850: error: (Each undeclared identifier is reported only once
fast-import.c:850: error: for each function it appears in.)
fast-import.c:850: error: expected ‘;’ before ‘ctx’
fast-import.c:886: warning: implicit declaration of function ‘SHA1_Init’
fast-import.c:886: error: ‘ctx’ undeclared (first use in this function)
fast-import.c:891: warning: implicit declaration of function ‘SHA1_Update’
fast-import.c:896: warning: implicit declaration of function ‘SHA1_Final’
fast-import.c: In function ‘store_object’:
fast-import.c:1035: error: ‘SHA_CTX’ undeclared (first use in this function)
fast-import.c:1035: error: expected ‘;’ before ‘c’
fast-import.c:1040: error: ‘c’ undeclared (first use in this function)
fast-import.c: In function ‘validate_raw_date’:
fast-import.c:1750: warning: ignoring return value of ‘strtoul’,
declared with attribute warn_unused_result
fast-import.c:1758: warning: ignoring return value of ‘strtoul’,
declared with attribute warn_unused_result
make: *** [fast-import.o] Error 1

Running configure and then make
$ make configure
GIT_VERSION = 1.6.5.rc0.dirty
    GEN configure
$ make
    CC fast-import.o
In file included from builtin.h:4,
                 from fast-import.c:143:
git-compat-util.h:129:25: error: openssl/ssl.h: No such file or directory
git-compat-util.h:130:25: error: openssl/err.h: No such file or directory
In file included from builtin.h:6,
                 from fast-import.c:143:
cache.h:8:21: error: openssl/sha.h: No such file or directory
In file included from fast-import.c:152:
csum-file.h:10: error: expected specifier-qualifier-list before ‘SHA_CTX’
fast-import.c: In function ‘create_index’:
fast-import.c:850: error: ‘SHA_CTX’ undeclared (first use in this function)
fast-import.c:850: error: (Each undeclared identifier is reported only once
fast-import.c:850: error: for each function it appears in.)
fast-import.c:850: error: expected ‘;’ before ‘ctx’
fast-import.c:886: warning: implicit declaration of function ‘SHA1_Init’
fast-import.c:886: error: ‘ctx’ undeclared (first use in this function)
fast-import.c:891: warning: implicit declaration of function ‘SHA1_Update’
fast-import.c:896: warning: implicit declaration of function ‘SHA1_Final’
fast-import.c: In function ‘store_object’:
fast-import.c:1035: error: ‘SHA_CTX’ undeclared (first use in this function)
fast-import.c:1035: error: expected ‘;’ before ‘c’
fast-import.c:1040: error: ‘c’ undeclared (first use in this function)
fast-import.c: In function ‘validate_raw_date’:
fast-import.c:1750: warning: ignoring return value of ‘strtoul’,
declared with attribute warn_unused_result
fast-import.c:1758: warning: ignoring return value of ‘strtoul’,
declared with attribute warn_unused_result
make: *** [fast-import.o] Error 1

Now autconf, ./configure, make.
$ autoconf
$ ./configure
configure: Setting lib to 'lib' (the default)
configure: Will try -pthread then -lpthread to enable POSIX Threads.
configure: CHECKS for site configuration
configure: CHECKS for programs
checking for cc... cc
checking for C compiler default output file name... a.out
checking whether the C compiler works... yes
checking whether we are cross compiling... no
checking for suffix of executables...
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether cc accepts -g... yes
checking for cc option to accept ISO C89... none needed
checking if linker supports -R... no
checking if linker supports -Wl,-rpath,... yes
checking for gar... no
checking for ar... ar
checking for gtar... no
checking for tar... tar
checking for asciidoc... asciidoc
checking for asciidoc version... asciidoc 8.2.7 > 7
configure: CHECKS for libraries
checking for SHA1_Init in -lcrypto... no
checking for SHA1_Init in -lssl... no
checking for curl_global_init in -lcurl... yes
checking for XML_ParserCreate in -lexpat... yes
checking for iconv in -lc... yes
checking for deflateBound in -lz... yes
checking for socket in -lc... yes
checking for hstrerror in -lc... yes
checking for basename in -lc... yes
configure: CHECKS for header files
checking how to run the C preprocessor... cc -E
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking sys/select.h usability... yes
checking sys/select.h presence... yes
checking for sys/select.h... yes
checking for old iconv()... no
configure: CHECKS for typedefs, structures, and compiler characteristics
checking for struct dirent.d_ino... yes
checking for struct dirent.d_type... yes
checking for struct sockaddr_storage... yes
checking for struct addrinfo... yes
checking for getaddrinfo... yes
checking for library containing getaddrinfo... none required
checking whether formatted IO functions support C99 size specifiers... yes
checking whether system succeeds to read fopen'ed directory... no
checking whether snprintf() and/or vsnprintf() return bogus value... no
configure: CHECKS for library functions
checking libgen.h usability... yes
checking libgen.h presence... yes
checking for libgen.h... yes
checking for strcasestr... yes
checking for library containing strcasestr... none required
checking for memmem... yes
checking for library containing memmem... none required
checking for strlcpy... no
checking for uintmax_t... yes
checking for strtoumax... yes
checking for library containing strtoumax... none required
checking for setenv... yes
checking for library containing setenv... none required
checking for unsetenv... yes
checking for library containing unsetenv... none required
checking for mkdtemp... yes
checking for library containing mkdtemp... none required
checking for mkstemps... no
checking Checking for POSIX Threads with '-pthread'... yes
configure: creating ./config.status
config.status: error: cannot find input file: config.mak.in
$ make

Now it compiles everything.

^ permalink raw reply

* Re: [PATCH] Add more instructions about how to install git.
From: Mikael Magnusson @ 2009-09-08 14:56 UTC (permalink / raw)
  To: Thiago Farina; +Cc: Johannes Sixt, Matthieu Moy, Alex Riesen, git
In-Reply-To: <a4c8a6d00909080752p5b663fc8r8bf1c60023ef39b4@mail.gmail.com>

2009/9/8 Thiago Farina <tfransosi@gmail.com>:
> On Tue, Sep 8, 2009 at 10:38 AM, Johannes Sixt<j.sixt@viscovery.net> wrote:
>> Thiago Farina schrieb:
>>> I tried to being more explicit about what the new users has to do. The
>>> first instruction that the text says is: "Normaly you can just do
>>> "make"".
>>> But the new user, cloning git won't have a configure file, so he can't
>>> just do "make".
>>>
>>> And then, the text says: "so you can write instead". This brings me to
>>> one alternative (not what I really should do), when in fact it should
>>> be (I guess) the first instructions that new users need to follow.
>>
>> Then guessed wrong. You really only have to say "make". The use of
>> autoconf and ./configure is purely optional and not necessary.
> I don't think so.
> Running only "make".
> $ make
> GIT_VERSION = 1.6.5.rc0
>    * new build flags or prefix
>    CC fast-import.o
> In file included from builtin.h:4,
>                 from fast-import.c:143:
> git-compat-util.h:129:25: error: openssl/ssl.h: No such file or directory
> git-compat-util.h:130:25: error: openssl/err.h: No such file or directory
[snip]

You need to edit the Makefile if your system needs tweaks, there are tons
of comments.

> Running configure and then make
> $ make configure
> GIT_VERSION = 1.6.5.rc0.dirty
>    GEN configure
> $ make
>    CC fast-import.o
> In file included from builtin.h:4,
>                 from fast-import.c:143:
> git-compat-util.h:129:25: error: openssl/ssl.h: No such file or directory
> git-compat-util.h:130:25: error: openssl/err.h: No such file or directory

You forgot to run configure.

> Now autconf, ./configure, make.
> $ autoconf
> $ ./configure
> configure: Setting lib to 'lib' (the default)
> configure: Will try -pthread then -lpthread to enable POSIX Threads.
> configure: CHECKS for site configuration
> configure: CHECKS for programs
[...]
> checking for mkstemps... no
> checking Checking for POSIX Threads with '-pthread'... yes
> configure: creating ./config.status
> config.status: error: cannot find input file: config.mak.in
> $ make
>
> Now it compiles everything.

-- 
Mikael Magnusson

^ permalink raw reply

* Re: [PATCH] Add more instructions about how to install git.
From: Brian Gernhardt @ 2009-09-08 15:19 UTC (permalink / raw)
  To: Thiago Farina; +Cc: Johannes Sixt, Matthieu Moy, Alex Riesen, git
In-Reply-To: <a4c8a6d00909080752p5b663fc8r8bf1c60023ef39b4@mail.gmail.com>


On Sep 8, 2009, at 10:52 AM, Thiago Farina wrote:

> On Tue, Sep 8, 2009 at 10:38 AM, Johannes Sixt<j.sixt@viscovery.net>  
> wrote:
>> Then guessed wrong. You really only have to say "make". The use of
>> autoconf and ./configure is purely optional and not necessary.
> I don't think so.
> Running only "make".
> $ make
> GIT_VERSION = 1.6.5.rc0
>    * new build flags or prefix
>    CC fast-import.o
> In file included from builtin.h:4,
>                 from fast-import.c:143:
> git-compat-util.h:129:25: error: openssl/ssl.h: No such file or  
> directory
> git-compat-util.h:130:25: error: openssl/err.h: No such file or  
> directory

You don't have OpenSSL, a perquisite mentioned in the INSTALL file at  
line 55.  You can do the following:

make NO_OPENSSL=Yes

or save the option so you don't have to retype it each time:

echo "NO_OPENSSL=Yes" > config.mak
make

Or install OpenSSL or run the autoconf.

The Makefile tries it's best to be as smart as possible without being  
a configure script of it's own (see ll. 656-925), and it's highly  
commented at the beginning so that only a minimal knowledge of make is  
required (ll. 4-198).  I suppose we could  note that the defines  
should be put in a config.mak file.

> Running configure and then make
> $ make configure
> GIT_VERSION = 1.6.5.rc0.dirty
>    GEN configure

  $ ./configure
[lots of output]
> $ make
>    CC fast-import.o

As pointed out before, you forgot to run the generated configure  
script (as already documented at Makefile:20).

However, it's generally considered preferable to hand-create a  
config.mak for the few options you need on a regular basis and let the  
Makefile do it's work.  The autoconf files occasionally lag behind the  
rest of development as they're not widely used by git developers  
(AFAICT).

I'd personally argue that we should point out in INSTALL that git can  
be configured via a hand-written config.mak using the defines  
described at the top of the Makefile before we talk about using  
autoconf.  (Perhaps with quick notes about the most commonly used  
defines.)

For example, my config.mak contains:

prefix=/usr/local
ASCIIDOC8=Yes
MAN_BOLD_LITERAL=Yes
BLK_SHA1=Yes

The rest of the configuration for my OS (OS X/Darwin, specifically)  
are handled by the Makefile.

~~ Brian

^ permalink raw reply

* Re: [PATCHv5 00/14] git notes
From: Johannes Schindelin @ 2009-09-08 15:53 UTC (permalink / raw)
  To: Johan Herland
  Cc: git, Junio C Hamano, trast, tavestbo, git, chriscool, spearce
In-Reply-To: <200909081436.30761.johan@herland.net>

Hi,

On Tue, 8 Sep 2009, Johan Herland wrote:

> On Tuesday 08 September 2009, Johannes Schindelin wrote:

> > I can see that some people may think that date-based fan-out is the 
> > cat's ass, but I have to warn that we have no idea how notes will be 
> > used,
> 
> I don't agree. Although we will certainly see many more use cases for 
> notes, I believe that the vast majority of them can be placed in one of 
> two categories:

My experience with Git is that having beliefs how my work is used was a 
constant source of surprise.

> > - I find the restriction to commits rather limiting.
> 
> I see your point, but I don't agree until I see a compelling case for 
> annotating a non-commit.

My point is that it is too late by then, if you don't allow for a flexible 
and still efficient scheme.

> > - most of the performance difference between the date-based and the 
> >   SHA-1 based fan-out looks to me as if the issue was the top-level 
> >   tree. Basically, this tree has to be read _every_ time _anybody_ 
> >   wants to read a note.
> 
> Not sure what you're trying to say here. The top-level notes tree is 
> read (as in fill_tree_descriptor()) exactly _once_. After that, it is 
> cached by the internal data structure (until free_commit_notes() or 
> end-of-process).

By that reasoning, we do not need any fan-out scheme.

Keep in mind: reading a large tree object takes a long time.  That's why 
we started fan-out.  Reading a large number of tree objects also takes a 
long time.  That's why I propagated flexible fan-out that is only read-in 
on demand.

> > But I think that having a dynamic fan-out that can even put blobs into 
> > the top-level tree (nothing prevents us from doing that, right?)
> 
> Well, the "flexible" code does add the new requirement that all entries 
> in a notes (sub)tree object must follow the same scheme, i.e. you 
> cannot have:
> 
>   /12/34567890123456789012345678901234567890
>   /2345/678901234567890123456789012345678901
> 
> but you can have
> 
>   /12/34567890123456789012345678901234567890
>   /23/45/678901234567890123456789012345678901

Umm, why?  Is there any good technical reason?

> > The real question for me, therefore, is: what is the optimal way to 
> > strike the balance between size of the tree objects (which we want to 
> > be small, so that unpacking them is fast)  and depth of the fan-out 
> > (which we want to be shallow to avoid reading worst-case 39 tree 
> > objects to get at one single note).
> 
> s/39/19/ (each fanout must use at least 2 chars of the 40-char SHA1)

That is another unnecessary restriction that could cost you dearly.  Just 
think what happens if it turns out that the optimal number of tree items 
is closer to 16 than to 255...

> Yes, the challenge is indeed striking the correct balance. I believe 
> that the notes code should be taught to write (and automatically 
> re-organize) the notes tree so that it is optimized for the current 
> collection of notes.

Of course!  I never thought that the user should be allowed to make the 
choice how to organize the notes.

Ciao,
Dscho

^ permalink raw reply

* [RFC PATCH 2/2] INSTALL: Describe a few knobs from the Makefile
From: Brian Gernhardt @ 2009-09-08 15:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano
In-Reply-To: <1252425313-69793-1-git-send-email-brian@gernhardtsoftware.com>

We said that some of our dependencies were optional, but didn't say
how to turn them off.  Add information for that and mention where to
save the options close to the top of the file.

Also, reorder the list so the absolutely required ones are at the top.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 I don't know if anyone wants this level of detail in the INSTALL file, or
 if we'd prefer people actually RTFMakefile.  It didn't take long to write
 though, so I thought I'd throw it out and see if people liked it.

 INSTALL |   33 +++++++++++++++++++++++----------
 1 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/INSTALL b/INSTALL
index 4a57e47..e08b990 100644
--- a/INSTALL
+++ b/INSTALL
@@ -13,6 +13,10 @@ that uses $prefix, the built results have some paths encoded,
 which are derived from $prefix, so "make all; make prefix=/usr
 install" would not work.
 
+There are many options that can be configured in the makefile using either
+command line defines or a config.mak file.  These options are documented at
+the beginning of the Makefile.
+
 Alternatively you can use autoconf generated ./configure script to
 set up install paths (via config.mak.autogen), so you can write instead
 
@@ -48,12 +52,19 @@ Issues of note:
 	export GIT_EXEC_PATH PATH GITPERLLIB
 
  - Git is reasonably self-sufficient, but does depend on a few external
-   programs and libraries:
+   programs and libraries.  Git can be used without most of them by adding
+   the approriate "NO_<LIBRARY>=YesPlease" to the make command line or
+   config.mak file.
 
 	- "zlib", the compression library. Git won't build without it.
 
-	- "openssl".  Unless you specify otherwise, you'll get the SHA1
-	  library from here.
+	- "ssh" is used to push and pull over the net
+
+	- A POSIX-compliant shell is needed to use most of the bare-bones
+	  Porcelainish scripts.
+
+	- "openssl".  Unless you specify otherwise (with NO_OPENSSL),
+	  you'll get the SHA1 library from here.
 
 	  If you don't have openssl, you can use one of the SHA1 libraries
 	  that come with git (git includes one based on Mozilla's as well
@@ -62,18 +73,20 @@ Issues of note:
 	- libcurl library; git-http-fetch and git-fetch use them.  You
 	  might also want the "curl" executable for debugging purposes.
 	  If you do not use http transfer, you are probably OK if you
-	  do not have them.
+	  do not have them (use NO_CURL).
 
 	- expat library; git-http-push uses it for remote lock
-	  management over DAV.  Similar to "curl" above, this is optional.
+	  management over DAV.  Similar to "curl" above, this is optional
+	  (with NO_EXPAT).
 
         - "wish", the Tcl/Tk windowing shell is used in gitk to show the
-          history graphically, and in git-gui.
-
-	- "ssh" is used to push and pull over the net
+          history graphically, and in git-gui.  If you don't want gitk or
+          git-gui, you can use NO_TCLTK.
 
-	- "perl" and POSIX-compliant shells are needed to use most of
-	  the bare-bones Porcelainish scripts.
+	- "perl" is used for several scripts that are useful, but not
+	  required for git (e.g. "git add -i" and "git difftool").  If you
+	  don't need the *.perl scripts or the library contained in perl/,
+	  then use NO_PERL.
 
  - Some platform specific issues are dealt with Makefile rules,
    but depending on your specific installation, you may not
-- 
1.6.4.2.420.g30ecf

^ permalink raw reply related

* [PATCH 1/2] INSTALL: Update description of our SHA-1 code
From: Brian Gernhardt @ 2009-09-08 15:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

We haven't had Mozilla's code or an ARM optimized algorithm since
30ae47b.  Reword the paragraph to give credit but not authorship to
Mozilla.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 Minor nit I noticed while in the INSTALL file.

 INSTALL |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/INSTALL b/INSTALL
index ae7f750..4a57e47 100644
--- a/INSTALL
+++ b/INSTALL
@@ -56,8 +56,8 @@ Issues of note:
 	  library from here.
 
 	  If you don't have openssl, you can use one of the SHA1 libraries
-	  that come with git (git includes the one from Mozilla, and has
-	  its own PowerPC and ARM optimized ones too - see the Makefile).
+	  that come with git (git includes one based on Mozilla's as well
+	  as a PowerPC optimized one - see the Makefile).
 
 	- libcurl library; git-http-fetch and git-fetch use them.  You
 	  might also want the "curl" executable for debugging purposes.
-- 
1.6.4.2.420.g30ecf

^ permalink raw reply related

* Re: Improving merge failure message
From: Junio C Hamano @ 2009-09-08 16:34 UTC (permalink / raw)
  To: Mike Ralphson; +Cc: Jeff King, Sverre Rabbelier, Nanako Shiraishi, git
In-Reply-To: <e2b179460909080059x62eb9d30t4d723d185e6316e3@mail.gmail.com>

Mike Ralphson <mike.ralphson@gmail.com> writes:

> 2009/9/8 Junio C Hamano <gitster@pobox.com>:
>> I'll leave the exact wording up to other people.  My primary focus was to
>> end all of these messages with "Aborting."
>>
>> This turns out to be a continuation of an older discussion thread back in
>> May 2008, and I do not know if anybody took up the challenge back then.  I
>> wouldn't be surprised if "checkout", which was the topic of the old
>> thread, has some other scary plumbing message still seeping through to the
>> UI layer.  Perhaps there are some other commands that needs similar kind
>> of love.
>
> Just a note that Documentation/git-checkout.txt references this
> message in an example and should be kept in step with the final
> wording change. It would be ideal to be able to regression-test the
> examples in the documentation somehow but that might involve abusing
> the asciidoc markup somewhat.

Thanks.

> 2009/9/8 Jeff King <peff@peff.net>:
>> While we're picking apart your wording, is "clobbered" the word we want
>> to use?
>
> If we're debating 'clobbered', then maybe the non-word 'uptodate' is
> fair game too? 8-)

I think you added smiley because you already knew the answer, but if that
is not the case, please see

    http://thread.gmane.org/gmane.comp.version-control.git/81100/focus=82358

^ permalink raw reply

* Re: [PATCH] Add test for ignoring pushInsteadOf when remote has explicit pushurl
From: Junio C Hamano @ 2009-09-08 16:34 UTC (permalink / raw)
  To: Josh Triplett; +Cc: Junio C Hamano, git
In-Reply-To: <20090908074147.GB3236@feather>

Josh Triplett <josh@joshtriplett.org> writes:

> Done.  Please add this to the queue, optionally squashing it into patch
> 2/2 if you prefer.

Thanks.

^ permalink raw reply

* Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated
From: Junio C Hamano @ 2009-09-08 16:57 UTC (permalink / raw)
  To: Lars Noschinski; +Cc: git
In-Reply-To: <20090908092059.GA8207@lars.home.noschinski.de>

Lars Noschinski <lars@public.noschinski.de> writes:

> * Lars Noschinski <lars@public.noschinski.de> [09-08-28 19:39]:
>> contrib/hooks/post-receive-email used to call the send_mail function
>> (and thus, /usr/sbin/sendmail), even if generate_mail returned an error.
>> This is problematic, as the sendmail binary provided by exim4 generates
>> an error mail if provided with an empty input.
>> 
>> Therefore, this commit changes post-receive-email to only call sendmail
>> if generate_mail returned without error.
>> 
>> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
>
> Is anything wrong with this patch? Or is it just queued to be committed
> some time?

It is not queued anywhere as far as I am concerned.

I was waiting for others to review the patch and nothing happened, so the
patch was in limbo.  Thanks for sending a reminder message I am responding
to.  You did the right thing when nothing happened to a patch that did not
see any discussion.

You can avoid this by CC'ing people who have been involved in the past
with the parts of the system you are patching in the initial posting of
your patch (I am not one of them, so CC'ing me didn't help).

Here are my knee-jerk reactions to the patch after a quick glance, without
thinking deeply nor looking at the other parts of the file you did not
touch, but looking only at the parts shown in your patch:

 - Slurping generate_email's output into a shell variable is a bad taste.
   You said that the message is always small enough but _how_ do we know
   it?

 - If this is to save us from a quirk in some but not all implementations
   of /usr/lib/sendmail, then shouldn't the logic be made into a new
   conditional?

 - I do not see a direct link between "if generate_mail returned an error"
   and "if ... an empty input".  What if generate_mail started its output
   but then failed halfway?  We have some output so the send_mail won't be
   fed empty, but $? would be not zero, so the patch is testing a
   different condition from what the log message claims to be checking.

People who do use this script and people who have worked on it may have
other more useful comments.

Thanks.

^ permalink raw reply

* [PATCH] git-gui: store wm state and fix wm geometry
From: Alexey Borzenkov @ 2009-09-08 18:39 UTC (permalink / raw)
  To: git; +Cc: spearce

I often close git gui window when it is maximized, and when I reopen
it next time the it would usually become out of place (e.g. a huge
window with a top-left corner somewhere close to the center of the
screen). Fix it by storing and restoring wm state in config, as well
as setting wm state to normal before retrieving wm geometry info.

Signed-off-by: Alexey Borzenkov <snaury@gmail.com>
---
 I've been using this patch for almost a year on Windows now.
 If you have any comments/questions please don't forget to cc me
 as I'm not subscribed to this list.

 git-gui.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index eae1f81..88d1025 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2025,6 +2025,19 @@ proc do_quit {{rc {1}}} {

 		# -- Stash our current window geometry into this repository.
 		#
+		set cfg_wmstate [wm state .]
+		if {[catch {set rc_wmstate $repo_config(gui.wmstate)}]} {
+			set rc_wmstate {}
+		}
+		if {$cfg_wmstate ne $rc_wmstate} {
+			catch {git config gui.wmstate $cfg_wmstate}
+		}
+		if {$cfg_wmstate eq {zoomed}} {
+			# on Windows wm geometry will lie about window
+			# position (but not size) when window is zoomed
+			# restore the window before querying wm geometry
+			wm state . normal
+		}
 		set cfg_geometry [list]
 		lappend cfg_geometry [wm geometry .]
 		lappend cfg_geometry [lindex [.vpane sash coord 0] 0]
@@ -3264,6 +3277,14 @@ wm geometry . [lindex $gm 0]
 unset gm
 }

+# -- Load window state
+#
+catch {
+set gws $repo_config(gui.wmstate)
+wm state . $gws
+unset gws
+}
+
 # -- Key Bindings
 #
 bind $ui_comm <$M1B-Key-Return> {do_commit;break}
-- 
1.6.4.2

^ permalink raw reply related

* Re: [PATCH] git.el: Use git-add-file for unmerged files, remove git-resolve-file
From: Alexandre Julliard @ 2009-09-08 18:43 UTC (permalink / raw)
  To: Martin Nordholts; +Cc: git, Junio C Hamano
In-Reply-To: <4AA026AC.10907@gmail.com>

Martin Nordholts <enselic@gmail.com> writes:

> Use `git-add-file' to mark unmerged files as resolved in the
> *git-status* buffer to be consistent with git's CLI instructions. Also
> remove `git-resolve-file' to make it clear that that "R" is a now a
> free keybinding.
>
> Signed-off-by: Martin Nordholts <martinn@src.gnome.org>

Looks good, thanks.

Acked-by: Alexandre Julliard <julliard@winehq.org>

-- 
Alexandre Julliard
julliard@winehq.org

^ permalink raw reply

* [PATCH] gitk: restore wm state to normal before saving geometry  information
From: Alexey Borzenkov @ 2009-09-08 18:44 UTC (permalink / raw)
  To: git; +Cc: Paul Mackerras

gitk now includes patches for saving and restoring wm state, however
because it saves wm geometry when window can still be maximized the
maximize/restore button becomes useless after restarting gitk (you
will get a huge displaced window if you try to restore it). This
patch fixes this issue by storing window geometry in normal state.

Signed-off-by: Alexey Borzenkov <snaury@gmail.com>
---
 Please don't forget to cc me if you have comments/questions

 gitk |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/gitk b/gitk
index 8c08310..fedeb88 100755
--- a/gitk
+++ b/gitk
@@ -2555,8 +2555,11 @@ proc savestuff {w} {
 	puts $f [list set extdifftool $extdifftool]
 	puts $f [list set perfile_attrs $perfile_attrs]

-	puts $f "set geometry(main) [wm geometry .]"
 	puts $f "set geometry(state) [wm state .]"
+	if {[wm state .] eq {zoomed}} {
+		wm state . normal
+	}
+	puts $f "set geometry(main) [wm geometry .]"
 	puts $f "set geometry(topwidth) [winfo width .tf]"
 	puts $f "set geometry(topheight) [winfo height .tf]"
         puts $f "set geometry(pwsash0) \"[.tf.histframe.pwclist sash
coord 0]\""
-- 
1.6.4.2

^ permalink raw reply related

* Re: [PATCH 3/4] push: make non-fast-forward help message configurable
From: Uri Okrent @ 2009-09-08 18:51 UTC (permalink / raw)
  To: Jeff King
  Cc: Matthieu Moy, Nanako Shiraishi, Junio C Hamano, Teemu Likonen,
	Git
In-Reply-To: <20090907085405.GA17968@coredump.intra.peff.net>

On 09/07/2009 01:54 AM, Jeff King wrote:
> At that point, why not just get rid of "message.all" and simply say
> "manually turn off the messages you don't like". Then the user can
> either go through the config manually as above, or they can wait until
> they become annoyed with a particular message and turn it off (and
> hopefully our naming is good enough that they can easily figure out
> which one it was :) ).
>
> So I think "be verbose, but let the user quiet us" is probably
> better than "be quiet, but let the user make us louder", because it is
> easier to discover verbose things. Which implies to me that
> "message.all", if it exists at all, should be limited in scope to just
> advice.

That seems like the most sane solution, given that it may not be obvious 
to the user what messages he/she may be missing if "expert" mode is on, 
and also from a coding perspective.

I know that by default I'd like to see new messages, and in case I'm 
doing something adventurous I'd like to see a message I may not have 
seen before. If I had turned off all messages because I got sick of 
seeing one or two in particular, then I'd never know.

Also, this thread started due to people's dislike of one particular 
message, so I think it's likely that in general someone would really 
only want to turn off at most a handful of messages, which again points 
to turning them off individually as being the best and simplest solution.

P.S. I never really introduced myself to the list... Uri Okrent here 
from L.A. Keep up the great work everyone!
-- 
    Uri

Please consider the environment before printing this message.
http://www.panda.org/how_you_can_help/

^ permalink raw reply

* [PATCH] post-receive-email: do not call sendmail if no mail was generated
From: Lars Noschinski @ 2009-09-08 19:00 UTC (permalink / raw)
  To: git; +Cc: andyparkins, pape, gitster, Lars Noschinski
In-Reply-To: <20090908185555.GA3858@lars.home.noschinski.de>

contrib/hooks/post-receive-email used to call the send_mail function
(and thus, /usr/sbin/sendmail), even if generate_mail generated no
output.  This is problematic, as the sendmail binary provided by exim4
generates an error mail if provided with an empty input.

Therefore, we now read one line ourselves and use the result to decide
if we really want to call /usr/sbin/sendmail.
---
 contrib/hooks/post-receive-email |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

Two things changed:

 - we do not read the whole mail in a shell variable
 - the decision whether to call sendmail is based on the output generated
   by generate_mail, not its return code

diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index 2a66063..c855c31 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -637,6 +637,16 @@ show_new_revisions()
 
 send_mail()
 {
+	OIFS=$IFS
+	IFS='
+'
+	read FIRSTLINE || exit 1
+	(printf $FIRSTLINE'\n'; cat) | call_sendmail
+	IFS=$OLD_IFS
+}
+
+call_sendmail()
+{
 	if [ -n "$envelopesender" ]; then
 		/usr/sbin/sendmail -t -f "$envelopesender"
 	else
@@ -644,6 +654,7 @@ send_mail()
 	fi
 }
 
+
 # ---------------------------- main()
 
 # --- Constants
-- 
1.6.3.3

^ permalink raw reply related

* Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated
From: Lars Noschinski @ 2009-09-08 18:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Andy Parkins, Gerrit Pape
In-Reply-To: <7vk5098jcv.fsf@alter.siamese.dyndns.org>

* Junio C Hamano <gitster@pobox.com> [09-09-08 19:22]:
> Lars Noschinski <lars@public.noschinski.de> writes:
> > * Lars Noschinski <lars@public.noschinski.de> [09-08-28 19:39]:
> >> contrib/hooks/post-receive-email used to call the send_mail function
> >> (and thus, /usr/sbin/sendmail), even if generate_mail returned an error.
> >> This is problematic, as the sendmail binary provided by exim4 generates
> >> an error mail if provided with an empty input.
> >> 
> >> Therefore, this commit changes post-receive-email to only call sendmail
> >> if generate_mail returned without error.
> >> 
> >> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
[...]
>  - Slurping generate_email's output into a shell variable is a bad taste.
>    You said that the message is always small enough but _how_ do we know
>    it?

You are right; I overlooked that the revision formatting is configurable
and if set up to display the full patch, the mail could get pretty big.

I know found a solution which does neither store the full output in a
variable nor needs a temporary file. I will post it as a reply to this
mail.

>  - If this is to save us from a quirk in some but not all implementations
>    of /usr/lib/sendmail, then shouldn't the logic be made into a new
>    conditional?

I don't know if this a quirk in exim; I could not find a formal
specification of the sendmail behaviour and treating such an "input" as
an error seems at least not insane.

In any case, I think the overhead implied by new patch is small enough,
that a switch is unnecessary.

>  - I do not see a direct link between "if generate_mail returned an error"
>    and "if ... an empty input".  What if generate_mail started its output
>    but then failed halfway?  We have some output so the send_mail won't be
>    fed empty, but $? would be not zero, so the patch is testing a
>    different condition from what the log message claims to be checking.

Yeah, you are right. This is also fixed in the new patch.

> People who do use this script and people who have worked on it may have
> other more useful comments.

CCed some of them.

^ permalink raw reply

* [PATCH] gitk: restore wm state to normal before saving geometry information
From: Alexey Borzenkov @ 2009-09-08 19:22 UTC (permalink / raw)
  To: git; +Cc: paulus, Alexey Borzenkov

gitk now includes patches for saving and restoring wm state, however
because it saves wm geometry when window can still be maximized the
maximize/restore button becomes useless after restarting gitk (you
will get a huge displaced window if you try to restore it). This
patch fixes this issue by storing window geometry in normal state.

Signed-off-by: Alexey Borzenkov <snaury@gmail.com>
---
 My previous email didn't come out rights, I hope this one will
 Don't forget to cc me if you have any comments/questions

 gitk |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/gitk b/gitk
index 8c08310..fedeb88 100755
--- a/gitk
+++ b/gitk
@@ -2555,8 +2555,11 @@ proc savestuff {w} {
 	puts $f [list set extdifftool $extdifftool]
 	puts $f [list set perfile_attrs $perfile_attrs]
 
-	puts $f "set geometry(main) [wm geometry .]"
 	puts $f "set geometry(state) [wm state .]"
+	if {[wm state .] eq {zoomed}} {
+		wm state . normal
+	}
+	puts $f "set geometry(main) [wm geometry .]"
 	puts $f "set geometry(topwidth) [winfo width .tf]"
 	puts $f "set geometry(topheight) [winfo height .tf]"
         puts $f "set geometry(pwsash0) \"[.tf.histframe.pwclist sash coord 0]\""
-- 
1.6.4.2

^ permalink raw reply related

* Re: [PATCH] Add more instructions about how to install git.
From: Matthieu Moy @ 2009-09-08 19:41 UTC (permalink / raw)
  To: Thiago Farina; +Cc: Johannes Sixt, Alex Riesen, git
In-Reply-To: <a4c8a6d00909080752p5b663fc8r8bf1c60023ef39b4@mail.gmail.com>

Thiago Farina <tfransosi@gmail.com> writes:

> Running only "make".
> $ make

[...]

> Running configure and then make
> $ make configure
> GIT_VERSION = 1.6.5.rc0.dirty
>     GEN configure
> $ make

[...]

What about trying to do what the INSTALL file suggests, then?

| Alternatively you can use autoconf generated ./configure script to
| set up install paths (via config.mak.autogen), so you can write
| instead
| 
|         $ make configure ;# as yourself
|         $ ./configure --prefix=/usr ;# as yourself
|         $ make all doc ;# as yourself
|         # make install install-doc install-html;# as root

You did the first line, but the three next one give you instructions
that you didn't follow. These instructions start line 16 of the
INSTALL file, that is, in the first screen when viewing the file with
a reasonable window size.

I tend to disagree with the mainainer that the configure should be the
"alternate" and plain make the recommanded way, but the configure
script is there, it works, it's documented early enough in the INSTALL
file, so what's the problem?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply

* Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated
From: Junio C Hamano @ 2009-09-08 20:15 UTC (permalink / raw)
  To: Lars Noschinski; +Cc: git, andyparkins, pape, gitster
In-Reply-To: <1252436418-7660-1-git-send-email-lars@public.noschinski.de>

Lars Noschinski <lars@public.noschinski.de> writes:

> contrib/hooks/post-receive-email used to call the send_mail function
> (and thus, /usr/sbin/sendmail), even if generate_mail generated no
> output.  This is problematic, as the sendmail binary provided by exim4
> generates an error mail if provided with an empty input.

I actually have a bigger question, not about the implementation but about
the cause.

If generate_email results in an empty output in this codepath:

	# Check if we've got anyone to send to
	if [ -z "$recipients" ]; then
		...
		echo >&2 "*** $config_name is not set so no email will be sent"
		echo >&2 "*** for $refname update $oldrev->$newrev"
		exit 0
	fi

shouldn't we rather receive an error e-mail than let the
misconfiguration go undetected?

Before this check, I do not see anywhere generate_email would return nor
exit, and after this check, there is a call to generate_email_header and
that guarantees that the output from the generate_email function is not
empty, so it looks to me that triggering this check is the only case your
patch would change the behaviour of the script.

It looks to me that your exim error mail is actually reporting a
legitimate problem you would want to fix in your configuration.

> Therefore, we now read one line ourselves and use the result to decide
> if we really want to call /usr/sbin/sendmail.
> ---
>  contrib/hooks/post-receive-email |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
>
> Two things changed:
>
>  - we do not read the whole mail in a shell variable
>  - the decision whether to call sendmail is based on the output generated
>    by generate_mail, not its return code
>
> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
> index 2a66063..c855c31 100755
> --- a/contrib/hooks/post-receive-email
> +++ b/contrib/hooks/post-receive-email
> @@ -637,6 +637,16 @@ show_new_revisions()
>  
>  send_mail()
>  {
> +	OIFS=$IFS
> +	IFS='
> +'
> +	read FIRSTLINE || exit 1

Shouldn't this be merely a "return"?  The caller looks like this:

	while read oldrev newrev refname
	do
		generate_email $oldrev $newrev $refname | send_mail
	done

and you would not want to stop after punting to report on the first ref.

> +	(printf $FIRSTLINE'\n'; cat) | call_sendmail
> +	IFS=$OLD_IFS
> +}
> +
> +call_sendmail()
> +{
>  	if [ -n "$envelopesender" ]; then
>  		/usr/sbin/sendmail -t -f "$envelopesender"
>  	else
> @@ -644,6 +654,7 @@ send_mail()
>  	fi
>  }
>  
> +
>  # ---------------------------- main()
>  
>  # --- Constants

Why?

^ permalink raw reply

* Re: [PATCH] Makefile: Add NEEDS_CRYPTO_WITH_SSL
From: Junio C Hamano @ 2009-09-08 20:19 UTC (permalink / raw)
  To: Brian Gernhardt; +Cc: Git List
In-Reply-To: <1252418078-68650-1-git-send-email-brian@gernhardtsoftware.com>

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> The Makefile comment for NEEDS_SSL_WITH_CRYPTO says to define it "if
> you need -lcrypto with -lssl (Darwin)."  However, what it actually
> does is add -lssl when you use -lcrypto and not the other way around.

Correct.  That is worth fixing.

> However, libcrypto contains a majority of the ERR_* functions from
> OpenSSL (at least on OS X) so we need it both ways.

I see.  We need to be able to see ERR_err_string(), whose caller happens
to be only imap-send.c for now, and that function is in -lcrypto together
with other ERR_* functions.

>  Compilation using BLK_SHA1 on OS X 10.5 and 10.6 (at least) is still
>  broken without this patch.
>
>  Alex Riesen <raa.lkml@gmail.com> pointed out that just adding LIB_4_CRYPTO
>  to git-imap-send is simpler, but judging from the fact that nobody else
>  has complained about this issue, I'm guessing that the need for -lcrypto
>  when using -lssl is not widespread.  (Or BLK_SHA1 isn't getting used much
>  or those who do don't compile git-imap-send with SSL.)

BLK_SHA1 is fairly new on 'master', so lack of breakage report is
expected.

The patch makes sense to me, but as the result, depending on platforms
and configuration, we would use three variations when linking imap-send
with no NO_OPENSSL defined:

 * Both -lcrypto -lssl
 * Only -lssl
 * Only -lcrypto

I wonder if we can simplify this in some way (not a 1.6.5 topic).

I am suspecting that the reason we do not say "always both" is because
depending on the vintage of OpenSSL one or the other is missing?

^ permalink raw reply

* Re: [PATCH 1/2] INSTALL: Update description of our SHA-1 code
From: Junio C Hamano @ 2009-09-08 20:21 UTC (permalink / raw)
  To: Brian Gernhardt; +Cc: Git List
In-Reply-To: <1252425313-69793-1-git-send-email-brian@gernhardtsoftware.com>

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> We haven't had Mozilla's code or an ARM optimized algorithm since
> 30ae47b.  Reword the paragraph to give credit but not authorship to
> Mozilla.

Thanks.

> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
>
>  Minor nit I noticed while in the INSTALL file.
>
>  INSTALL |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/INSTALL b/INSTALL
> index ae7f750..4a57e47 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -56,8 +56,8 @@ Issues of note:
>  	  library from here.
>  
>  	  If you don't have openssl, you can use one of the SHA1 libraries
> -	  that come with git (git includes the one from Mozilla, and has
> -	  its own PowerPC and ARM optimized ones too - see the Makefile).
> +	  that come with git (git includes one based on Mozilla's as well
> +	  as a PowerPC optimized one - see the Makefile).

The leading comment in block/sha1.c says "initially based on ... although
none of the original remains."  "one loosely based on" or "one inspired
by" might be a more fair statement, although I do not feel too strongly
about it.

>  
>  	- libcurl library; git-http-fetch and git-fetch use them.  You
>  	  might also want the "curl" executable for debugging purposes.
> -- 
> 1.6.4.2.420.g30ecf

^ permalink raw reply

* Re: [RFC PATCH 2/2] INSTALL: Describe a few knobs from the Makefile
From: Junio C Hamano @ 2009-09-08 20:26 UTC (permalink / raw)
  To: Brian Gernhardt; +Cc: Git List
In-Reply-To: <1252425313-69793-2-git-send-email-brian@gernhardtsoftware.com>

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> We said that some of our dependencies were optional, but didn't say
> how to turn them off.  Add information for that and mention where to
> save the options close to the top of the file.
>
> Also, reorder the list so the absolutely required ones are at the top.
>
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
>
>  I don't know if anyone wants this level of detail in the INSTALL file, or
>  if we'd prefer people actually RTFMakefile.  It didn't take long to write
>  though, so I thought I'd throw it out and see if people liked it.

Thanks.

Sprinkling these Makefile variable names in this document does not add too
much detail.  If anything, they serve as good keyphrases to jump to when
you have Makefile in your pager and editing your own config.mak.

I like your patch especially because it makes it clear what the reader
will be missing if s/he chooses to omit some dependencies.

> +	- "ssh" is used to push and pull over the net
> +

Please add a full-stop at the end (original was missing one, too).

> +	- A POSIX-compliant shell is needed to use most of the bare-bones
> +	  Porcelainish scripts.

Let's stop talking about Porcelain/plumbing in this document.

It is very likely that the reader of this file has not read the main
documentation that talks about the two-tier structure.

The self pejorative reference "bare-bones" dates back to the days when git
Porcelains were supposed to be merely simpler reference implementations,
as opposed to something more end-user friendly like what Cogito aimed to
be.  But that is an old history, and there is nothing "bare-bones" about
them anymore.

So please reword it along this line:

	- A POSIX-compilant shell is needed to use many of the features
          (e.g. "bisect", "pull") in everyday use.

> +	- "openssl".  Unless you specify otherwise (with NO_OPENSSL),
> +	  you'll get the SHA1 library from here.

It is not very clear what will be affected by disabling this.

 - SHA-1 is not used from OpenSSL, as stated;
 - imap-send won't be able to talk over SSL;

Do we still able to walk https:// URLs?  If your cURL library is linked
with gnutls I think we can, but I never tried the combination.

> @@ -62,18 +73,20 @@ Issues of note:
>  	- libcurl library; git-http-fetch and git-fetch use them.  You
>  	  might also want the "curl" executable for debugging purposes.
>  	  If you do not use http transfer, you are probably OK if you
> +	  do not have them (use NO_CURL).

Probably reads more easily if it were:

	If you do not interact with http:// repositories, you do not have
	to have them (say NO_CURL).

> +	- "perl" is used for several scripts that are useful, but not
> +	  required for git (e.g. "git add -i" and "git difftool").  If you
> +	  don't need the *.perl scripts or the library contained in perl/,
> +	  then use NO_PERL.

I do not think moving "Perl" this low in the requirement level is such a
good idea, at least for now.  I'd suggest movign it back immediately after
POSIX-compliant shell, and would say something like this:

	- "Perl" is needed to use some of the features (e.g. preparing a
          partial commit using "git add -i/-p", interacting with svn
          repositories with "git svn").  If you can live without these,
          say NO_PERL.

Maybe they are Ruby, github, and general acceptance by many open source
projects these days, but it used to be that the initial entry points to
git were "git cvsimport" and "git svn" for a surprisingly large number of
people.

^ permalink raw reply

* Re: [PATCHv5 00/14] git notes
From: Junio C Hamano @ 2009-09-08 20:31 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johan Herland, git, trast, tavestbo, git, chriscool, spearce
In-Reply-To: <alpine.DEB.1.00.0909081100020.4330@intel-tinevez-2-302>

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

> Hi,
>
> On Tue, 8 Sep 2009, Johan Herland wrote:
>
>> Algorithm / Notes tree   git log -n10 (x100)   git log --all
>> ------------------------------------------------------------
>> next / no-notes                4.77s              63.84s
>> 
>> before / no-notes              4.78s              63.90s
>> before / no-fanout            56.85s              65.69s
>> 
>> 16tree / no-notes              4.77s              64.18s
>> 16tree / no-fanout            30.35s              65.39s
>> 16tree / 2_38                  5.57s              65.42s
>> 16tree / 2_2_36                5.19s              65.76s
>> 
>> flexible / no-notes            4.78s              63.91s
>> flexible / no-fanout          30.34s              65.57s
>> flexible / 2_38                5.57s              65.46s
>> flexible / 2_2_36              5.18s              65.72s
>> flexible / ym                  5.13s              65.66s
>> flexible / ym_2_38             5.08s              65.63s
>> flexible / ymd                 5.30s              65.45s
>> flexible / ymd_2_38            5.29s              65.90s
>> flexible / y_m                 5.11s              65.72s
>> flexible / y_m_2_38            5.08s              65.67s
>> flexible / y_m_d               5.06s              65.50s
>> flexible / y_m_d_2_38          5.07s              65.79s
>
> It's good to see that the no-notes behaves roughly like baseline.
>
> I can see that some people may think that date-based fan-out is the cat's 
> ass,

Actually, my knee-jerk reaction was that 4.77 (next) vs 5.57 (16tree with
2_38) is already a good enough performance/simplicity tradeoff, and 5.57
vs 5.08 (16tree with ym_2_38) probably does not justify the risk of worst
case behaviour that can come from possible mismatch between the access
pattern and the date-optimized tree layout.

But that only argues against supporting _only_ date-optimized layout.

Support of "flexible layout" is not that flexible as its name suggests;
one single note tree needs to have a uniform fanout strategy.  But it is
not unusably rigid either; you only need to be extra careful when merging
two notes trees.  We can leave the heuristics to choose what the optimum
layout to later rounds.

> - I find the restriction to commits rather limiting.

Yeah, we would not want to be surprised to find many people want to
annotate non-commits with this mechanism.

> - most of the performance difference between the date-based and the SHA-1 
>   based fan-out looks to me as if the issue was the top-level tree.  
>   Basically, this tree has to be read _every_ time _anybody_ wants to read 
>   a note.

A comparison between 'next' and another algorithm that opens the top-level
notes tree object and returns "I did not find any note" without doing
anything else would reveal that cost.  But when you are doing "log -n10"
(or "log --all"), you would read the notes top-level tree once, and it is
likely to be cached in the obj_hash[] (or in delta_base cache) already for
the remaining invocations, even if notes mechanism does not do its own
cache, which I think it does, no?

> - I'd love to see performance numbers for less than 157118 notes.  Don't 
>   get me wrong, it is good to see the worst-case scenario in terms of 
>   notes/commits ratio.  But it will hardly be the common case, and I 
>   very much would like to optimize for the common case.
>
>   So, I'd appreciate if you could do the tests with something like 500 
>   notes, randomly spread over the commits (rationale: my original 
>   understanding was that the notes could amend commit messages, and that 
>   is much more likely to be done with relatively old commits that you 
>   cannot change anymore).

Hmph, is that a typical use case?  How does it relate to CC's object
replacement mechanism?

Also Gitney talked about annotating commits in the code-review thing.
What's the expected notes density and distribution in that application?

^ permalink raw reply

* Re: [PATCH] git.el: Use git-add-file for unmerged files, remove git-resolve-file
From: Junio C Hamano @ 2009-09-08 20:43 UTC (permalink / raw)
  To: Alexandre Julliard; +Cc: Martin Nordholts, git
In-Reply-To: <873a6x9t0l.fsf@wine.dyndns.org>

Alexandre Julliard <julliard@winehq.org> writes:

> Martin Nordholts <enselic@gmail.com> writes:
>
>> Use `git-add-file' to mark unmerged files as resolved in the
>> *git-status* buffer to be consistent with git's CLI instructions. Also
>> remove `git-resolve-file' to make it clear that that "R" is a now a
>> free keybinding.
>>
>> Signed-off-by: Martin Nordholts <martinn@src.gnome.org>
>
> Looks good, thanks.
>
> Acked-by: Alexandre Julliard <julliard@winehq.org>

Thanks, both.

^ permalink raw reply

* Re: git-diff: must --exit-code work with --ignore* options?
From: Thell Fowler @ 2009-09-08 20:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jim Meyering, git list
In-Reply-To: <7vljl1dpud.fsf@alter.siamese.dyndns.org>

On Sun, 30 Aug 2009, Junio C Hamano wrote:

> Jim Meyering <jim@meyering.net> writes:
>
>> Junio C Hamano wrote:
>> ...
>>> Subject: [PATCH] diff --quiet: special case "ignore whitespace" options
>>> ...
>>> Change the semantics of --ignore-whitespace* options to mean more than
>>> "omit showing the difference in text".  When these options are used, the
>>> internal "quick" optimization is turned off, and the status reported with
>>> the --exit-code option will now match if any the textual diff output is
>>> actually produced.
>>>
>>> Also rename the internal option "QUIET" to "QUICK" to better reflect what
>>> its true purpose is.
>>
>> Thanks again.
>> If there's anything I can to do help (add a test?), let me know.
>
> The change has been cooking in 'next' and hopefully be in 1.7.0.  I think
> the updated series adds its own test script, too.
>
> Using it in every day scenario, and reporting any breakage you notice
> before 1.7.0 happens, would be greatly appreciated.
>
> Thanks.

Perhaps I'm expected something different than what I _should_ be 
expecting, but shouldn't --quiet always return the same as --exit-code?

# Cut/Paste example
mkdir test_ws_quiet && cd test_ws_quiet && git init
printf "foo bar  \n\n" >f1.txt
git add .
git commit -m 'f text'
printf "foo  bar\n\n" >f1.txt
git commit -a -m 'f with diff white-space in middle & end'
git diff -w --exit-code HEAD^ >/dev/null
echo $?
# returns '0' which it should
git diff -w --quiet HEAD^
echo $?
# returns '0' which it should
git diff -b --exit-code HEAD^ >/dev/null
echo $?
# returns '0' which it should
git diff -b --quiet HEAD^ >/dev/null
echo $?
# returns '0' which it should
git diff --ignore-space-at-eol --exit-code HEAD^ >/dev/null
echo $?
# returns '1' which it should
git diff --ignore-space-at-eol --quiet HEAD^
echo $?
#returns '0' <=== Unexpected.

#
# Next phase
#
printf "foobar\n\n">f1.txt
git commit -a -m 'f without any spaces'
git diff -w --exit-code HEAD^ >/dev/null
echo $?
# returns '0' which it should
git diff -w --quiet HEAD^
echo $?
# returns '0' which it should
git diff -b --exit-code HEAD^ >/dev/null
echo $?
# returns '1' which it should
git diff -b --quiet HEAD^ >/dev/null
echo $?
# returns '0' <=== Unexpected
git diff --ignore-space-at-eol --exit-code HEAD^ >/dev/null
echo $?
# returns '1' which it should
git diff --ignore-space-at-eol --quiet HEAD^
echo $?
#returns '0' <=== Unexpected.

-- 
Thell

^ permalink raw reply

* Re: [PATCH] post-receive-email: do not call sendmail if no mail was generated
From: Lars Noschinski @ 2009-09-08 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, andyparkins, pape
In-Reply-To: <7v4ord19da.fsf@alter.siamese.dyndns.org>

* Junio C Hamano <gitster@pobox.com> [09-09-08 22:15]:
> Lars Noschinski <lars@public.noschinski.de> writes:
> 
> > contrib/hooks/post-receive-email used to call the send_mail function
> > (and thus, /usr/sbin/sendmail), even if generate_mail generated no
> > output.  This is problematic, as the sendmail binary provided by exim4
> > generates an error mail if provided with an empty input.
> 
> I actually have a bigger question, not about the implementation but about
> the cause.
> 
> If generate_email results in an empty output in this codepath:
> 
> 	# Check if we've got anyone to send to
> 	if [ -z "$recipients" ]; then
> 		...
> 		echo >&2 "*** $config_name is not set so no email will be sent"
> 		echo >&2 "*** for $refname update $oldrev->$newrev"
> 		exit 0
> 	fi
> 
> shouldn't we rather receive an error e-mail than let the
> misconfiguration go undetected?

Probably not. The error message is displayed to the user who did the
push. Normally (if no explicit From: address is configured), this is the
same user, which would receive the error mail.

> Before this check, I do not see anywhere generate_email would return nor
> exit, and after this check, there is a call to generate_email_header and
> that guarantees that the output from the generate_email function is not
> empty, so it looks to me that triggering this check is the only case your
> patch would change the behaviour of the script.

Actually, there are a two cases in the case statement before, where
generate_email would return:

    refs/remotes/*,commit)
        # tracking branch
        refname_type="tracking branch"
        short_refname=${refname##refs/remotes/}
        echo >&2 "*** Push-update of tracking branch, $refname"
        echo >&2 "***  - no email generated."
        exit 0
        ;;
    *)
        # Anything else (is there anything else?)
        echo >&2 "*** Unknown type of update to $refname ($rev_type)"
        echo >&2 "***  - no email generated"
        exit 1
        ;;

i.e. if we are pushing to a branch neither in refs/tags nor refs/heads.

In our setting, the build process pushes to refs/builds, so we can track
code changes between different builds, without displaying a whole lot of
mostly useless branches or tags to the user.

> > diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
> > index 2a66063..c855c31 100755
> > --- a/contrib/hooks/post-receive-email
> > +++ b/contrib/hooks/post-receive-email
> > @@ -637,6 +637,16 @@ show_new_revisions()
> >  
> >  send_mail()
> >  {
> > +	OIFS=$IFS
> > +	IFS='
> > +'
> > +	read FIRSTLINE || exit 1
> 
> Shouldn't this be merely a "return"?  The caller looks like this:

Yes.

I'll fix it in the next patch (when there are further comments); but you
may fold it in (and add the SOB if forgot), if you prefer.

^ 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