* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: Junio C Hamano @ 2010-12-05 20:23 UTC (permalink / raw)
To: Yann Dirson; +Cc: git
In-Reply-To: <20101205101333.GA7466@home.lan>
Yann Dirson <ydirson@free.fr> writes:
> On Sat, Dec 04, 2010 at 10:30:21PM -0800, Junio C Hamano wrote:
>> * kb/diff-C-M-synonym (2010-11-29) 1 commit
>> - diff: add --detect-copies-harder as a synonym for --find-copies-harder
>>
>> Will merge to 'next' soon.
>
> If we go this way, don't we want to deprecate --find-copies-harder as well ?
Why?
We are being nice to people who did not know --find-copies-harder but
learned the --detect-renames long name before learning it, which by
definition is are people because the long names have been there only for
the last few months; they may expect "detect" to work there. That is the
sole purpose of the additional synonym.
I do not think we would get much benefit from panalizing people who knew
about and have used the --find-copies-harder option for long time by
marking it to be deprecated.
^ permalink raw reply
* Re: Intermittent failures in t9119
From: Junio C Hamano @ 2010-12-05 20:39 UTC (permalink / raw)
To: David D. Kilzer; +Cc: git, Eric Wong
In-Reply-To: <7vmxoml1p7.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Sorry for bringing up an old story, but since you added "git svn info"
> with e6fefa9 (git-svn info: implement info command, 2007-11-21),...
>
> I am seeing breakages from the said test:
>
> not ok - 6 info file
> #
> # (cd svnwc; svn info file) > expected.info-file &&
> # (cd gitwc; git svn info file) > actual.info-file &&
> # test_cmp expected.info-file actual.info-file
>
> : trash directory.t9119-git-svn-info/master; diff *.info-file
> 12c12
> < Text Last Updated: 2010-12-04 00:34:58 +0000 (Sat, 04 Dec 2010)
> ---
> > Text Last Updated: 2010-12-04 00:34:57 +0000 (Sat, 04 Dec 2010)
>
> I do not know if we should expect output from "svn info" to match exactly
> like the above. Is "Text Last Updated" supposed to show the stat
> timestamp of the working tree (I see "(stat $path)[9]" there)?
> ...
>
> I _think_ "svn info file" actually gives the timestamp of the revision
> that touched the file the last (in their linear worldview of the history),
> so perhaps this is demonstrating the real bug, but only when the test runs
> very slowly?
I think we should not expect "git svn info" to match "svn info" at least
at the "Text Last Updated" line. The former seems to want to report the
timestamp of the working tree, but "svn info" does not seem to look at the
file in the working tree.
$ mkdir svntest
$ cd svntest
$ mkdir repo
$ svnadmin create repo
$ svn checkout file://$(pwd)/repo work
Checked out revision 0.
$ cd work
$ echo hello >world; svn add world; svn commit -m initial
A world
Adding world
Transmitting file data .
Committed revision 1.
$ svn info world | grep 'Text Last'
Text Last Updated: 2010-12-05 12:24:52 -0800 (Sun, 05 Dec 2010)
$ touch world
$ svn info world | grep 'Text Last'
Text Last Updated: 2010-12-05 12:24:52 -0800 (Sun, 05 Dec 2010)
$ ls -l world
-rw-rw-r-- 1 junio junio 6 2010-12-05 12:25 world
$ rm world
$ svn info world | grep 'Text Last'
Text Last Updated: 2010-12-05 12:24:52 -0800 (Sun, 05 Dec 2010)
I have these suspicions:
(1) Somebody may have liked if "git svn info" exactly matched "svn info"
in that it would give the timestamp of the commit object that touched
the path the last in the current history. The current implementation
in that sense is buggy;
(2) Nobody uses the value from "Text Last Updated" field in practice, so
that bug has been unnoticed so far;
(3) And it is not worth fixing it ;-)
For now, I would suggest fixing the failing test to ignore the "Text Last
Updated" field while comparing, and if somebody is inclined to, we would
update the code to match what "svn info" does.
Thoughts?
^ permalink raw reply
* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
From: Junio C Hamano @ 2010-12-05 20:40 UTC (permalink / raw)
To: Thiago Farina; +Cc: Junio C Hamano, git
In-Reply-To: <AANLkTikxibh4QkxzokhBYQ+dMS3W6PkDaDLuqm5qN+6v@mail.gmail.com>
Thiago Farina <tfransosi@gmail.com> writes:
>> If a function takes a commit_list, named "reduce_HEADS",
>
> What? reduce_HEADS ? HEADS with CAPSLOCK?
I was just hiliting the relevant parts of the name for you. Read it as if
it was painted in red or something.
^ permalink raw reply
* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
From: Junio C Hamano @ 2010-12-05 21:07 UTC (permalink / raw)
To: Thiago Farina; +Cc: Jonathan Nieder, git
In-Reply-To: <AANLkTinjJpGW2OiXM3edWYaNhS+w4qNLrvg-0aBwsL=x@mail.gmail.com>
Thiago Farina <tfransosi@gmail.com> writes:
> Also I thought that as Junio already picked up the other patch. It's
> was a hint that the other functions that has "struct commit_list *l"
> as its parameters could be renamed as well.
You took a wrong hint and I think that is because you didn't think about
what naming is for.
"insert-by-date" does not say _why_ you want things to be inserted by date
(neither "sort-by-date"). They are pretty generic looking names for any
function that deal with a list of elements that record date. It makes
sense to anticipate there will be many other such functions that deal with
different kinds of lists that hold date-recording things, and naming one
of them "this deals with list of COMMITS" by saying "commit_list_foo"
makes quite a lot of sense, as "insert-by-date" does not give sufficient
information to the reader.
On the other hand, "reduce-heads" is with quite a higher level of
semantics than "insert-by-date" and friends. The caller has a set of
commits and wants to remove the ones that can be reached by other commits
in that set, typically because it wants to come up with a list of commits
to be used as parents of a merge commit across them. It has much stronger
"why" associated with it; unlike "insert-by-date" and friends, there can't
be many other such functions that deal with different datastructures that
hold commits to reduce the heads the same way.
A related tangent. There are two ways to name functions with richer "why"
component. Some people name them after what the caller expects them to do
(e.g. they would name "compute merge parents" the function in question),
and others names them after what they themselves do (e.g. it is about
reducing the set of heads by removing redundant parents, and it does not
question for what purpose the caller wants to do so). In general, it is
preferrable to name them after what they do, not why the caller wants them
to do so, especially when the semantics is clear. It will allow easier
reuse of the function by new callers that do not create a new merge commit
but wants the same head reduction.
^ permalink raw reply
* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
From: Thiago Farina @ 2010-12-05 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git
In-Reply-To: <7vk4joszcj.fsf@alter.siamese.dyndns.org>
On Sun, Dec 5, 2010 at 7:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Thiago Farina <tfransosi@gmail.com> writes:
>
>> Also I thought that as Junio already picked up the other patch. It's
>> was a hint that the other functions that has "struct commit_list *l"
>> as its parameters could be renamed as well.
>
> You took a wrong hint and I think that is because you didn't think about
> what naming is for.
>
> "insert-by-date" does not say _why_ you want things to be inserted by date
> (neither "sort-by-date"). They are pretty generic looking names for any
> function that deal with a list of elements that record date. It makes
> sense to anticipate there will be many other such functions that deal with
> different kinds of lists that hold date-recording things, and naming one
> of them "this deals with list of COMMITS" by saying "commit_list_foo"
> makes quite a lot of sense, as "insert-by-date" does not give sufficient
> information to the reader.
>
That makes sense to me. And clarified why the complain at all. And you
are right.
Would these be a candidates for adding commit_list_ prefix?
free_commit_list -> commit_list_free
contains
pop_most_recent_commit -> I'm not sure about this because of the
length of it, as Jonathan pointed in this thread.
pop_commit
^ permalink raw reply
* [PATCH] parse-remote: handle detached HEAD
From: Santi Béjar @ 2010-12-05 23:47 UTC (permalink / raw)
To: git; +Cc: Sverre Rabbelier
In get_remote_merge_branch 'git for-each-ref' is used to know the
upstream branch of the current branch ($curr_branch). But $curr_branch
can be empty when in detached HEAD, so the call to for-each-ref is
made without a pattern.
Quote the $curr_branch variable in the git for-each-ref call to always
provide a pattern (the current branch or an empty string) Otherwise it
would mean all refs.
This fixes a bug reported by Sverre Rabbelier. The overall results
were correct but not the output text.
Signed-off-by: Santi Béjar <santi@agolina.net>
---
Hi *,
Sorry Sverre, but the patch you tested fixed your case but broke all
the others :(
Hope you can also test it. Now it passes the test suite.
Thanks,
Santi
git-parse-remote.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 5f47b18..07060c3 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -68,7 +68,7 @@ get_remote_merge_branch () {
test -z "$origin" && origin=$default
curr_branch=$(git symbolic-ref -q HEAD)
[ "$origin" = "$default" ] &&
- echo $(git for-each-ref --format='%(upstream)' $curr_branch)
+ echo $(git for-each-ref --format='%(upstream)' "$curr_branch")
;;
*)
repo=$1
--
1.7.3.3.399.gea47f
^ permalink raw reply related
* Re: [PATCH] parse-remote: handle detached HEAD
From: Sverre Rabbelier @ 2010-12-05 23:49 UTC (permalink / raw)
To: Santi Béjar; +Cc: git
In-Reply-To: <1291592835-29949-1-git-send-email-santi@agolina.net>
Heya,
On Mon, Dec 6, 2010 at 00:47, Santi Béjar <santi@agolina.net> wrote:
> This fixes a bug reported by Sverre Rabbelier. The overall results
> were correct but not the output text.
I think we usually use:
Reported-by: Sverre Rabbelier <srabbelier@gmail.com>
But since I already verified the fix, perhaps just:
Tested-by: Sverre Rabbelier <srabbelier@gmail.com>
is enough? Both are fine with me.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* [PATCHv2] parse-remote: handle detached HEAD
From: Santi Béjar @ 2010-12-05 23:58 UTC (permalink / raw)
To: git; +Cc: Sverre Rabbelier, Junio C Hamano
In-Reply-To: <AANLkTi=yfuiFuatshYuS2Q0EV0Ytj-QFKpuXAWeGerQB@mail.gmail.com>
In get_remote_merge_branch 'git for-each-ref' is used to know the
upstream branch of the current branch ($curr_branch). But $curr_branch
can be empty when in detached HEAD, so the call to for-each-ref is
made without a pattern.
Quote the $curr_branch variable in the git for-each-ref call to always
provide a pattern (the current branch or an empty string) Otherwise it
would mean all refs.
Reported-by: Sverre Rabbelier <srabbelier@gmail.com>
Signed-off-by: Santi Béjar <santi@agolina.net>
Tested-by: Sverre Rabbelier <srabbelier@gmail.com>
---
Changes since v1:
Tags for Reported-by and Tested-by.
git-parse-remote.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 5f47b18..07060c3 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -68,7 +68,7 @@ get_remote_merge_branch () {
test -z "$origin" && origin=$default
curr_branch=$(git symbolic-ref -q HEAD)
[ "$origin" = "$default" ] &&
- echo $(git for-each-ref --format='%(upstream)' $curr_branch)
+ echo $(git for-each-ref --format='%(upstream)' "$curr_branch")
;;
*)
repo=$1
--
1.7.3.3.399.gea47f
^ permalink raw reply related
* Re: path/to/some/file: needs update
From: Patrick Doyle @ 2010-12-06 0:44 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqk4jq3cpc.fsf@bauges.imag.fr>
On Fri, Dec 3, 2010 at 1:58 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Patrick Doyle <wpdster@gmail.com> writes:
>
>> path/to/some/file: needs update.
>>
>> Then things got worse... one of the messages said:
>>
>> error: Entry 'path/to/some/binaryfile' not uptodate. Cannot merge
>
> Which commands triggered these messages? Which version of Git?
>
> AFAICT, these messages never appear anymore in normal (porcelain) use
> in recent Gits.
Ahhh... that's something.
I just checked. The box they were using is an RHEL 5 box that has
1.5.5.6 installed on it. Perhaps it's time to upgrade their git.
I'll go see what repo I need to add to RHEL to get a more recent git.
Thanks.
--wpd
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: aleksi.aalto @ 2010-12-06 0:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7v8w04vvvr.fsf@alter.siamese.dyndns.org>
On Sun, 5 Dec 2010, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Sat, Dec 04, 2010 at 10:30:21PM -0800, Junio C Hamano wrote:
>>
>>> * aa/status-hilite-branch (2010-11-18) 1 commit
>>> - status: show branchname with a configurable color
>>>
>>> I am indifferent/uninterested; I don't see anything wrong with it, but I
>>> do not find coloring the field particularly useful myself.
>>
>> I am not particularly interested, either, but FWIW, the gitcommit syntax
>> highlighting that ships with vim does highlight this, so there are at
>> least other people who think this is a good idea.
>
> As you already know, when I say "'Meh' personally", I am not saying "I
> want to forbid others to want it".
>
> How does vim highlight the other parts of that particular line? Does it
> keep them intact, or paint them in some other color?
The default colorscheme results in the branchname being colored with a
different color than the rest of the header. Also the texts "Changes to be
committed:" and "Untracked files" are colored with the same color. However
with some other colorschemes, these texts have a different color from the
branchname.
> I had this suspicion that the class of people who choose a non default
> status.header color and the class of people who choose plain there (or
> have been happy with the default) expect different things. The former
> prefer louder output, different pieces of information painted in different
> colors to help them chromatically distinguish them. The latter (including
> myself) favor subdued output, without too many colors distacting them
> while reading the output.
>
> This suspicion further led me to think that the former would want this new
> feature to paint the branch name in a color different from status.header
> color, while the latter would want it in plain. So the default of "plain"
> would be a win for both audiences.
This reasoning sounds good to me.
:Aga
^ permalink raw reply
* Re: [PATCHv2] parse-remote: handle detached HEAD
From: Junio C Hamano @ 2010-12-06 3:33 UTC (permalink / raw)
To: Santi Béjar; +Cc: git, Sverre Rabbelier
In-Reply-To: <1291593517-4406-1-git-send-email-santi@agolina.net>
Santi Béjar <santi@agolina.net> writes:
> In get_remote_merge_branch 'git for-each-ref' is used to know the
> upstream branch of the current branch ($curr_branch). But $curr_branch
> can be empty when in detached HEAD, so the call to for-each-ref is
> made without a pattern.
>
> Quote the $curr_branch variable in the git for-each-ref call to always
> provide a pattern (the current branch or an empty string) Otherwise it
> would mean all refs.
What output do you want to see in this case? "Nothing needs to be
reported because on detached head you are not tracking anything?"
If that is the case, shouldn't we be not calling "echo" at all to begin
with? IOW, shouldn't the code read more like this?
curr_branch=$(git symbolic-ref -q HEAD) &&
test "$origin" = "$default" &&
echo ...
> Reported-by: Sverre Rabbelier <srabbelier@gmail.com>
> Signed-off-by: Santi Béjar <santi@agolina.net>
> Tested-by: Sverre Rabbelier <srabbelier@gmail.com>
> ---
> Changes since v1:
> Tags for Reported-by and Tested-by.
>
> git-parse-remote.sh | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-parse-remote.sh b/git-parse-remote.sh
> index 5f47b18..07060c3 100644
> --- a/git-parse-remote.sh
> +++ b/git-parse-remote.sh
> @@ -68,7 +68,7 @@ get_remote_merge_branch () {
> test -z "$origin" && origin=$default
> curr_branch=$(git symbolic-ref -q HEAD)
> [ "$origin" = "$default" ] &&
> - echo $(git for-each-ref --format='%(upstream)' $curr_branch)
> + echo $(git for-each-ref --format='%(upstream)' "$curr_branch")
> ;;
> *)
> repo=$1
^ permalink raw reply
* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
From: Junio C Hamano @ 2010-12-06 3:58 UTC (permalink / raw)
To: Thiago Farina; +Cc: Jonathan Nieder, git
In-Reply-To: <AANLkTi=QK=N+_iGR9-47JKFs_SDKujJ8c4mtnnM0yo94@mail.gmail.com>
Thiago Farina <tfransosi@gmail.com> writes:
> Would these be a candidates for adding commit_list_ prefix?
>
> free_commit_list -> commit_list_free
I think free_commit_list is a reasonable name for a function to free a
commit-list already.
> contains
Historically I think we had two functions with this name, and both were
named perfectly fine in the context they were introduced in.
One is "does this haystack contain the needle we are looking for?", which
is a private helper in diffcore-pickaxe.c and considering what that module
does, it is crystal clear that it is about "needle in haystack" without
anything else tucked to its name.
The other is in 'pu' that came from Peff's "How about this" patch to
compute something similar to is-descendant-of more efficiently, while
sacrificing the ability to be usable as a general helper function.
As I already said in the review of that stalled series, the particular
implementation of that function is good enough within the scope of the
command it is used for (namely "tag --contains"); its implementation needs
to be cleaned up, moved from commit.c to builtin/tag.c and made static to
the file, but as long as that happens, it is named appropriately.
> pop_most_recent_commit -> I'm not sure about this because of the
> length of it, as Jonathan pointed in this thread.
> pop_commit
These two do not sound wrong---pop/push implies queue/list-ness and is
quite clear that we are removing the topmost element from it.
^ permalink raw reply
* Re: [PATCH] commit: Add commit_list prefix to reduce_heads function.
From: Junio C Hamano @ 2010-12-06 4:01 UTC (permalink / raw)
To: Thiago Farina; +Cc: Junio C Hamano, Jonathan Nieder, git
In-Reply-To: <AANLkTi=QK=N+_iGR9-47JKFs_SDKujJ8c4mtnnM0yo94@mail.gmail.com>
Thiago Farina <tfransosi@gmail.com> writes:
>> "insert-by-date" does not say _why_ you want things to be inserted by date
>> (neither "sort-by-date"). They are pretty generic looking names for any
>> function that deal with a list of elements that record date. It makes
>> sense to anticipate there will be many other such functions that deal with
>> different kinds of lists that hold date-recording things, and naming one
>> of them "this deals with list of COMMITS" by saying "commit_list_foo"
>> makes quite a lot of sense, as "insert-by-date" does not give sufficient
>> information to the reader.
>>
> That makes sense to me. And clarified why the complain at all. And you
> are right.
Actually I think s/insert_by_date/commit_list_insert_by_date/ is a
mistake. Something like insert-commit-by-date would be more appropriate.
Similarly for s/sort_by_date/commit_list_sort_by_date/
^ permalink raw reply
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
From: Anders Kaseorg @ 2010-12-06 5:28 UTC (permalink / raw)
To: Diego Elio Pettenò, Junio C Hamano; +Cc: git
In-Reply-To: <1290629033-20566-1-git-send-email-flameeyes@gmail.com>
On Wed, 2010-11-24 at 21:03 +0100, Diego Elio Pettenò wrote:
> When using stricter linkers, such as GNU gold or Darwin ld, transitive
> dependencies are not counted towards symbol resolution. If we don't
> link imap-send to libcrypto, we'll have undefined references to the
> HMAC_*, EVP_* and ERR_* functions families.
> […]
> git-imap-send$X: imap-send.o $(GITLIBS)
> $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> - $(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
> + $(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
This broke the build with NO_OPENSSL=1, so Debian will need to revert
it:
CC imap-send.o
LINK git-imap-send
/usr/bin/ld: cannot find -lcrypto
collect2: ld returned 1 exit status
make[1]: *** [git-imap-send] Error 1
Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s
automatically set on Darwin, Windows, and MinGW. We shouldn’t have two
mechanisms for addressing the same problem; maybe we just need to enable
the existing flag on more (or all) platforms?
Anders
^ permalink raw reply
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
From: Diego Elio Pettenò @ 2010-12-06 5:39 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, git
In-Reply-To: <1291613304.3339.12.camel@fixed-disk>
Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:
> This broke the build with NO_OPENSSL=1, so Debian will need to revert
> it:
I'll try a NO_OPENSSL build later on today and see to get it fixed.
> Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s
> automatically set on Darwin, Windows, and MinGW. We shouldn’t have two
> mechanisms for addressing the same problem; maybe we just need to enable
> the existing flag on more (or all) platforms?
No, these should be different issues; you may have a libssl (which uses
libcrypto) requiring libcrypto to be linked in, even if you only use
interfaces from libssl (and that's what NEEDS_CRYPTO_WITH_SSL seem to be
designed to deal with), but in this case what you have is rather
imap-send using the libcrypto interfaces _as well as_ the libssl
interfaces.
I have blogged a detailed analysis of the problem, if you wish to see
the details:
http://blog.flameeyes.eu/2010/11/26/it-s-not-all-gold-that-shines-why-underlinking-is-a-bad-thing
--
Diego Elio Pettenò — “Flameeyes”
http://blog.flameeyes.eu/
If you found a .asc file in this mail and know not what it is,
it's a GnuPG digital signature: http://www.gnupg.org/
^ permalink raw reply
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
From: Jonathan Nieder @ 2010-12-06 6:06 UTC (permalink / raw)
To: Diego Elio Pettenò
Cc: Anders Kaseorg, Junio C Hamano, git, Brian Gernhardt
In-Reply-To: <1291613978.4756.157.camel@yamato.local>
Diego Elio Pettenò wrote:
> Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:
>> Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s
>> automatically set on Darwin, Windows, and MinGW. We shouldn’t have two
>> mechanisms for addressing the same problem; maybe we just need to enable
>> the existing flag on more (or all) platforms?
>
> No, these should be different issues
Are you sure? The change description for v1.6.5-rc1~23 (Makefile: Add
NEEDS_CRYPTO_WITH_SSL, 2009-10-08) says:
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.
However, libcrypto contains a majority of the ERR_* functions from
OpenSSL (at least on OS X) so we need it both ways.
and grepping that version reveals the same hit I think your patch
deals with:
$ git grep -F -e ERR_ v1.6.5-rc1~23
v1.6.5-rc1~23:imap-send.c: fprintf(stderr, "%s: %s\n", func, ERR_error_string(ERR_get_error(), NULL));
I am guessing the reality is closer to:
Previously we relied on -lssl pulling in libcrypto on most
platforms. That has at least three problems:
(1) it is not resiliant against future changes in libssl.
The DT_NEEDED entries in a library are generally
considered an implementation detail that is allowed to
change.
(2) it does not work on all platforms; for example, ld on
Mac OS X and ld --no-copy-dt-needed-entries on Linux
do not permit that trick.
(3) it is tricky. A simple rule is "explicitly link to
all libraries whos symbols you use directly".
To fix this, eliminate the NEEDS_CRYPTO_WITH_SSL knob and
always treat it as true. NEEDS_SSL_WITH_CRYPTO can stay
because there really are differences between platforms
for that one.
Warning: I could be totally wrong. :) I have not verified any
of these claims.
^ permalink raw reply
* Re: [PATCH] imap-send: link against libcrypto for HMAC and others
From: Junio C Hamano @ 2010-12-06 6:11 UTC (permalink / raw)
To: Diego Elio Pettenò; +Cc: Anders Kaseorg, git
In-Reply-To: <1291613978.4756.157.camel@yamato.local>
Diego Elio Pettenò <flameeyes@gmail.com> writes:
> Il giorno lun, 06/12/2010 alle 00.28 -0500, Anders Kaseorg ha scritto:
>> This broke the build with NO_OPENSSL=1, so Debian will need to revert
>> it:
>
> I'll try a NO_OPENSSL build later on today and see to get it fixed.
>
>> Also, the Makefile already has a NEEDS_CRYPTO_WITH_SSL flag that’s
>> automatically set on Darwin, Windows, and MinGW. We shouldn’t have two
>> mechanisms for addressing the same problem; maybe we just need to enable
>> the existing flag on more (or all) platforms?
>
> No, these should be different issues; you may have a libssl (which uses
> libcrypto) requiring libcrypto to be linked in, even if you only use
> interfaces from libssl (and that's what NEEDS_CRYPTO_WITH_SSL seem to be
> designed to deal with), but in this case what you have is rather
> imap-send using the libcrypto interfaces _as well as_ the libssl
> interfaces.
You are both correct; the point of NO_OPENSSL is not to link with anything
from openssl suite, so we need a separate mechanism to address this.
Anders, wouldn't this be a better fix for NO_OPENSSL build, than reverting
a fix for an incorrect ld invocation?
Makefile | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 7a5fb69..b20ae1a 100644
--- a/Makefile
+++ b/Makefile
@@ -1296,11 +1296,15 @@ else
BLK_SHA1 = 1
OPENSSL_LIBSSL =
endif
+ifdef NO_OPENSSL
+ LIB_4_CRYPTO =
+else
ifdef NEEDS_SSL_WITH_CRYPTO
LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto -lssl
else
LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
endif
+endif
ifdef NEEDS_LIBICONV
ifdef ICONVDIR
BASIC_CFLAGS += -I$(ICONVDIR)/include
^ permalink raw reply related
* Re: [PATCH] describe: Don’t look up commits with --exact-match
From: Anders Kaseorg @ 2010-12-06 7:19 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Junio C Hamano, git, SZEDER Gábor, Kirill Smelkov,
Thomas Rast
In-Reply-To: <20101203084348.GD18202@burratino>
On Fri, 3 Dec 2010, Jonathan Nieder wrote:
> - static?
> - the convention in git is to leave off the zero-initializers
> for bss-allocated vars (static and globals).
Will fix.
> The usual convention is to use !hashcmp(...) to match the !strcmp(...)
> idiom.
Will fix (I hate that idiom, but yeah, match existing style).
> > + for (e = names; e; e = e->next) {
> > + if (!hashcmp(e->peeled, cmit->object.sha1) &&
> > + replace_name(n, e->prio, e->sha1, &e->tag))
> > + n = e;
> > + }
>
> Instead of looking up the commit to be matched exactly in the commits
> hash table, this makes a linear search.
>
> No change to the assymptotic running time, but would this make things
> much slower in the case of many tags? How many before it's a problem
> (if ever)?
I don’t think it’s ever a problem: in my repository with 1800 tags on a
warm cache, that loop accounts for about 0.1% of even the fastest
non-exact-match query (a commit right after a tag).
> (If it's a problem in ordinary cases, I think the optimization could be
> limited to --exact-match pretty easily.)
Then you’d lose the speedup in the case where --exact-match wasn’t
specified but a tag happens to match exactly (which isn’t critical, but
seemed nice).
> > - for_each_ref(get_name, NULL);
> > + for_each_rawref(get_name, NULL);
>
> Orthogonal change snuck in?
This does fall under the category of “Don’t lookup commits,” and is
necessary to get the speedup (otherwise for_each_ref has already looked up
the commits that the rest of the patch is trying to avoid looking up).
But I could split it out if you want.
Anders
^ permalink raw reply
* [PATCH] describe: Don’t look up commits with --exact-match
From: Anders Kaseorg @ 2010-12-06 7:22 UTC (permalink / raw)
To: Jonathan Nieder, Junio C Hamano
Cc: git, SZEDER Gábor, Kirill Smelkov, Thomas Rast
In-Reply-To: <alpine.DEB.2.02.1012060149550.23348@dr-wily.mit.edu>
This makes ‘git describe --exact-match HEAD’ about 15 times faster on
a cold cache (2.3s instead of 35s) in a linux-2.6 repository with many
packed tags. That’s a huge win for the interactivity of the __git_ps1
shell prompt helper.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
builtin/describe.c | 64 ++++++++++++++++++++++++++-------------------------
1 files changed, 33 insertions(+), 31 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 43caff2..0cddef1 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -22,7 +22,7 @@ static int tags; /* Allow lightweight tags */
static int longformat;
static int abbrev = DEFAULT_ABBREV;
static int max_candidates = 10;
-static int found_names;
+static struct commit_name *names;
static const char *pattern;
static int always;
static const char *dirty;
@@ -34,6 +34,8 @@ static const char *diff_index_args[] = {
struct commit_name {
+ struct commit_name *next;
+ unsigned char peeled[20];
struct tag *tag;
unsigned prio:2; /* annotated tag = 2, tag = 1, head = 0 */
unsigned name_checked:1;
@@ -78,31 +80,26 @@ static int replace_name(struct commit_name *e,
}
static void add_to_known_names(const char *path,
- struct commit *commit,
+ const unsigned char *peeled,
int prio,
const unsigned char *sha1)
{
- struct commit_name *e = commit->util;
struct tag *tag = NULL;
- if (replace_name(e, prio, sha1, &tag)) {
- size_t len = strlen(path)+1;
- free(e);
- e = xmalloc(sizeof(struct commit_name) + len);
- e->tag = tag;
- e->prio = prio;
- e->name_checked = 0;
- hashcpy(e->sha1, sha1);
- memcpy(e->path, path, len);
- commit->util = e;
- }
- found_names = 1;
+ size_t len = strlen(path)+1;
+ struct commit_name *e = xmalloc(sizeof(struct commit_name) + len);
+ hashcpy(e->peeled, peeled);
+ e->tag = tag;
+ e->prio = prio;
+ e->name_checked = 0;
+ hashcpy(e->sha1, sha1);
+ memcpy(e->path, path, len);
+ e->next = names;
+ names = e;
}
static int get_name(const char *path, const unsigned char *sha1, int flag, void *cb_data)
{
int might_be_tag = !prefixcmp(path, "refs/tags/");
- struct commit *commit;
- struct object *object;
unsigned char peeled[20];
int is_tag, prio;
@@ -110,16 +107,10 @@ static int get_name(const char *path, const unsigned char *sha1, int flag, void
return 0;
if (!peel_ref(path, peeled) && !is_null_sha1(peeled)) {
- commit = lookup_commit_reference_gently(peeled, 1);
- if (!commit)
- return 0;
- is_tag = !!hashcmp(sha1, commit->object.sha1);
+ is_tag = !!hashcmp(sha1, peeled);
} else {
- commit = lookup_commit_reference_gently(sha1, 1);
- object = parse_object(sha1);
- if (!commit || !object)
- return 0;
- is_tag = object->type == OBJ_TAG;
+ hashcpy(peeled, sha1);
+ is_tag = 0;
}
/* If --all, then any refs are used.
@@ -142,7 +133,7 @@ static int get_name(const char *path, const unsigned char *sha1, int flag, void
if (!prio)
return 0;
}
- add_to_known_names(all ? path + 5 : path + 10, commit, prio, sha1);
+ add_to_known_names(all ? path + 5 : path + 10, peeled, prio, sha1);
return 0;
}
@@ -228,7 +219,7 @@ static void describe(const char *arg, int last_one)
unsigned char sha1[20];
struct commit *cmit, *gave_up_on = NULL;
struct commit_list *list;
- struct commit_name *n;
+ struct commit_name *n, *e;
struct possible_tag all_matches[MAX_TAGS];
unsigned int match_cnt = 0, annotated_cnt = 0, cur_match;
unsigned long seen_commits = 0;
@@ -240,7 +231,12 @@ static void describe(const char *arg, int last_one)
if (!cmit)
die("%s is not a valid '%s' object", arg, commit_type);
- n = cmit->util;
+ n = NULL;
+ for (e = names; e; e = e->next) {
+ if (!hashcmp(e->peeled, cmit->object.sha1) &&
+ replace_name(n, e->prio, e->sha1, &e->tag))
+ n = e;
+ }
if (n && (tags || all || n->prio == 2)) {
/*
* Exact match to an existing ref.
@@ -259,6 +255,12 @@ static void describe(const char *arg, int last_one)
if (debug)
fprintf(stderr, "searching to describe %s\n", arg);
+ for (e = names; e; e = e->next) {
+ struct commit *c = lookup_commit_reference_gently(e->peeled, 1);
+ if (c && replace_name(c->util, e->prio, e->sha1, &e->tag))
+ c->util = e;
+ }
+
list = NULL;
cmit->object.flags = SEEN;
commit_list_insert(cmit, &list);
@@ -418,8 +420,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
return cmd_name_rev(i + argc, args, prefix);
}
- for_each_ref(get_name, NULL);
- if (!found_names && !always)
+ for_each_rawref(get_name, NULL);
+ if (!names && !always)
die("No names found, cannot describe anything.");
if (argc == 0) {
--
1.7.3.2
^ permalink raw reply related
* Re: [PATCH] describe: Don’t look up commits with --exact-match
From: Jonathan Nieder @ 2010-12-06 7:32 UTC (permalink / raw)
To: Anders Kaseorg
Cc: Junio C Hamano, git, SZEDER Gábor, Kirill Smelkov,
Thomas Rast
In-Reply-To: <alpine.DEB.2.02.1012060149550.23348@dr-wily.mit.edu>
Anders Kaseorg wrote:
> On Fri, 3 Dec 2010, Jonathan Nieder wrote:
>> Instead of looking up the commit to be matched exactly in the commits
>> hash table, this makes a linear search.
[...]
> I don’t think it’s ever a problem: in my repository with 1800 tags on a
> warm cache, that loop accounts for about 0.1% of even the fastest
> non-exact-match query (a commit right after a tag).
Thanks for checking. Makes sense.
>>> - for_each_ref(get_name, NULL);
>>> + for_each_rawref(get_name, NULL);
>>
>> Orthogonal change snuck in?
>
> This does fall under the category of “Don’t lookup commits,” and is
> necessary to get the speedup (otherwise for_each_ref has already looked up
> the commits that the rest of the patch is trying to avoid looking up).
Depends on what "Don't lookup commits" means, I suppose. I
think the difference between _ref and _rawref is
if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
if (entry->flag & REF_BROKEN)
return 0; /* ignore dangling symref */
if (!has_sha1_file(entry->sha1)) {
error("%s does not point to a valid object!", entry->name);
return 0;
}
}
so if I understand correctly, for_each_ref would still allow one to
get away without unpacking the objects. Is that correct?
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: Yann Dirson @ 2010-12-06 7:29 UTC (permalink / raw)
To: gitster; +Cc: git list
In-Reply-To: <7v62v8ufyl.fsf@alter.siamese.dyndns.org>
>> On Sat, Dec 04, 2010 at 10:30:21PM -0800, Junio C Hamano wrote:
>>> * kb/diff-C-M-synonym (2010-11-29) 1 commit
>>> - diff: add --detect-copies-harder as a synonym for
>>> --find-copies-harder
>>>
>>> Will merge to 'next' soon.
>>
>> If we go this way, don't we want to deprecate --find-copies-harder as
>> well ?
>
>Why?
>
>We are being nice to people who did not know --find-copies-harder but
>learned the --detect-renames long name before learning it, which by
>definition is are people because the long names have been there only for
>the last few months; they may expect "detect" to work there. That is
>the sole purpose of the additional synonym.
But then, why not simply use --find-renames (since --detect-renames has
luckily not been released ontl the masses yet), and avoid making similar-usage
opts dissimilar and then adding a synonym just to make them similar the other
way ?
IOW, we already have tons of options everywhere, let's not just add clutter.
We'll end up here with those people used to using --detect-copies-harder willing
shell completion; that will just add one more item to the list we get after
"--<TAB>", and it will eat precious screen space for pretty much nothing.
Just my 0.02€...
--
Yann Dirson - Bertin Technologies
^ permalink raw reply
* Re: [PATCH 01/10] parse-options: Don't call parse_options_check() so much
From: Stephen Boyd @ 2010-12-06 7:57 UTC (permalink / raw)
To: René Scharfe
Cc: Jonathan Nieder, git, Nguyễn Thái Ngọc Duy,
Pierre Habouzit
In-Reply-To: <4CFBD693.20305@lsrfire.ath.cx>
On 12/05/10 10:14, René Scharfe wrote:
> Am 02.12.2010 00:28, schrieb Jonathan Nieder:
>> From: Stephen Boyd <bebarino@gmail.com>
>> void parse_options_start(struct parse_opt_ctx_t *ctx,
>> int argc, const char **argv, const char *prefix,
>> - int flags)
>> + int flags, const struct option *options)
>
> It might be better to put options before flags, i.e. to use the same
> order as in parse_options().
>
Jonathan mentioned that too. Sounds good to me. Here's an updated patch.
--->8----8<----
Subject: [PATCH] parse-options: Don't call parse_options_check() so much
parse_options_check() is being called for each invocation of
parse_options_step which can be quite a bit for some commands. The
commit introducing this function cb9d398 (parse-options: add
parse_options_check to validate option specs., 2009-06-09) had the
correct motivation and explicitly states that parse_options_check()
should be called from parse_options_start(). However, the implementation
differs from the motivation. Fix it.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
builtin/blame.c | 4 ++--
builtin/shortlog.c | 4 ++--
parse-options.c | 7 +++----
parse-options.h | 2 +-
4 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index cb25ec9..aa30ec5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2325,8 +2325,8 @@ int cmd_blame(int argc, const char **argv, const
char *prefix)
save_commit_buffer = 0;
dashdash_pos = 0;
- parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ parse_options_start(&ctx, argc, argv, prefix, options,
+ PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
for (;;) {
switch (parse_options_step(&ctx, options, blame_opt_usage)) {
case PARSE_OPT_HELP:
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 2135b0d..1a21e4b 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -268,8 +268,8 @@ int cmd_shortlog(int argc, const char **argv, const
char *prefix)
git_config(git_default_config, NULL);
shortlog_init(&log);
init_revisions(&rev, prefix);
- parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ parse_options_start(&ctx, argc, argv, prefix, options,
+ PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
for (;;) {
switch (parse_options_step(&ctx, options, shortlog_usage)) {
diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..9bbbc6a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -338,7 +338,7 @@ static void parse_options_check(const struct option
*opts)
void parse_options_start(struct parse_opt_ctx_t *ctx,
int argc, const char **argv, const char *prefix,
- int flags)
+ const struct option *options, int flags)
{
memset(ctx, 0, sizeof(*ctx));
ctx->argc = argc - 1;
@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
(flags & PARSE_OPT_STOP_AT_NON_OPTION))
die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+ parse_options_check(options);
}
static int usage_with_options_internal(struct parse_opt_ctx_t *,
@@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
{
int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
- parse_options_check(options);
-
/* we must reset ->opt, unknown short option leave it dangling */
ctx->opt = NULL;
@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const
char *prefix,
{
struct parse_opt_ctx_t ctx;
- parse_options_start(&ctx, argc, argv, prefix, flags);
+ parse_options_start(&ctx, argc, argv, prefix, options, flags);
switch (parse_options_step(&ctx, options, usagestr)) {
case PARSE_OPT_HELP:
exit(129);
diff --git a/parse-options.h b/parse-options.h
index ae8647d..7b53414 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {
extern void parse_options_start(struct parse_opt_ctx_t *ctx,
int argc, const char **argv, const char *prefix,
- int flags);
+ const struct option *options, int flags);
extern int parse_options_step(struct parse_opt_ctx_t *ctx,
const struct option *options,
--
^ permalink raw reply related
* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: Miles Bader @ 2010-12-06 8:13 UTC (permalink / raw)
To: Yann Dirson; +Cc: gitster, git list
In-Reply-To: <20101206082948.1403cc5a@chalon.bertin.fr>
Yann Dirson <dirson@bertin.fr> writes:
> But then, why not simply use --find-renames (since --detect-renames has
> luckily not been released ontl the masses yet), and avoid making similar-usage
> opts dissimilar and then adding a synonym just to make them similar the other
> way ?
"Find" and "detect" have different nuances.
"Detect" sounds somewhat passive/minor, so "detect renames" makes it
more clear that renames are detected _in addition_ to normal processing.
"Find," by contrast, is active/major, so "find renames" makes it sounds
like an action to be performed _instead_ of some normal processing.
-Miles
--
97% of everything is grunge
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: Yann Dirson @ 2010-12-06 8:21 UTC (permalink / raw)
To: Miles Bader; +Cc: gitster, git list
In-Reply-To: <buopqtfmi85.fsf@dhlpc061.dev.necel.com>
On Mon, 06 Dec 2010 17:13:46 +0900
Miles Bader <miles@gnu.org> wrote:
> Yann Dirson <dirson@bertin.fr> writes:
> > But then, why not simply use --find-renames (since --detect-renames
> > has luckily not been released ontl the masses yet), and avoid
> > making similar-usage opts dissimilar and then adding a synonym just
> > to make them similar the other way ?
>
> "Find" and "detect" have different nuances.
>
> "Detect" sounds somewhat passive/minor, so "detect renames" makes it
> more clear that renames are detected _in addition_ to normal
> processing.
Seen that argument before.
1. does anyone care ? (I personally don't)
2. whether we care or not we have IMHO to face the implications, see my
other mails about implications of both paths
--
Yann Dirson - Bertin Technologies
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2010, #01; Sat, 4)
From: Miles Bader @ 2010-12-06 8:39 UTC (permalink / raw)
To: Yann Dirson; +Cc: gitster, git list
In-Reply-To: <20101206092122.21c19011@chalon.bertin.fr>
On Mon, Dec 6, 2010 at 5:21 PM, Yann Dirson <dirson@bertin.fr> wrote:
> Seen that argument before.
> 1. does anyone care ? (I personally don't)
If nobody cared, you wouldn't get an argument.
-Miles
--
Cat is power. Cat is peace.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox