Git development
 help / color / mirror / Atom feed
* Re: [RFC/PATCH] git-web--browse: don't add start as candidate on Ubuntu
From: Junio C Hamano @ 2009-01-04  7:53 UTC (permalink / raw)
  To: Christian Couder; +Cc: Petr Baudis, Ramsay Jones, GIT Mailing-list
In-Reply-To: <200901040833.25849.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

> Le jeudi 1 janvier 2009, Ramsay Jones a écrit :
> ...
>> Does anybody else see this issue and can someone test the patch?
>
> Petr, as you added support for using /bin/start on MinGW, could you test?
>
> Thanks,
> Christian.

> diff --git a/git-web--browse.sh b/git-web--browse.sh
> index 78d236b..7ed0fad 100755
> --- a/git-web--browse.sh
> +++ b/git-web--browse.sh
> @@ -115,7 +115,7 @@ if test -z "$browser" ; then
>  	browser_candidates="open $browser_candidates"
>      fi
>      # /bin/start indicates MinGW
> -    if test -n /bin/start; then
> +    if test -x /bin/start; then
>  	browser_candidates="start $browser_candidates"
>      fi

In any case, the original test is simply bogus.  'test -n "$foo"' is to
see if $foo is an empty string, and giving a constant /bin/start always
yields true.

As an old timer, I tend to prefer "test -f" in this context, as anybody
sane can expect that the directory /bin will contain executables and
nothing else.  Another reason is purely stylistic.  Historically "-f" has
been much more portable than "-x" (which came from SVID), even though it
wouldn't make much difference in practice in the real world these days.

^ permalink raw reply

* Re: git-branch --print-current
From: Arnaud Lacombe @ 2009-01-04  8:21 UTC (permalink / raw)
  To: Karl Chen; +Cc: Git mailing list
In-Reply-To: <quack.20090101T1928.lthzliaqtdf@roar.cs.berkeley.edu>

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

Hi,

On Thu, Jan 1, 2009 at 10:28 PM, Karl Chen <quarl@cs.berkeley.edu> wrote:
>
> How about an option to git-branch that just prints the name of the
> current branch for scripts' sake?  To replace:
>
>    git branch --no-color 2>/dev/null | perl -ne '/^[*] (.*)/ && print $1'
FWIW, I had this in a stalled modification in a tree, it just add the
'-c' (as "current") option to git branch. Patch is mostly for the
record :/

The main trouble I have with pipe stuff is that it forks a process for
something that can be done natively. Previously, I was using awk(1) to
extract the current branch:

$ git branch | awk '/^\*/ {print $2}'

 - Arnaud

[-- Attachment #2: show-current-branch.diff --]
[-- Type: application/octet-stream, Size: 5373 bytes --]

diff --git a/builtin-branch.c b/builtin-branch.c
index 494cbac..2846768 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -17,7 +17,7 @@
 #include "revision.h"
 
 static const char * const builtin_branch_usage[] = {
-	"git branch [options] [-r | -a] [--merged | --no-merged]",
+	"git branch [options] [-r | -a | -c] [--merged | --no-merged]",
 	"git branch [options] [-l] [-f] <branchname> [<start-point>]",
 	"git branch [options] [-r] (-d | -D) <branchname>",
 	"git branch [options] (-m | -M) [<oldbranch>] <newbranch>",
@@ -312,9 +312,6 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	int color;
 	struct commit *commit = item->commit;
 
-	if (!matches_merge_filter(commit))
-		return;
-
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
 		color = COLOR_BRANCH_LOCAL;
@@ -373,18 +370,58 @@ static int calc_maxwidth(struct ref_list *refs)
 	return w;
 }
 
-static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit)
+static inline int is_current(struct ref_item *item, int detached)
+{
+
+	if (detached ||
+	    item->kind != REF_LOCAL_BRANCH ||
+	    strcmp(item->name, head) != 0)
+		return 0;
+
+	return 1;
+}
+
+static void print_ref_list(struct ref_list *ref_list, int kinds, int detached,
+    int verbose, int abbrev, struct commit_list *with_commit)
 {
+	struct commit *head_commit;
 	int i;
+
+	head_commit = lookup_commit_reference_gently(head_sha1, 1);
+
+	detached = (detached && (kinds & REF_LOCAL_BRANCH));
+	if (detached && head_commit && has_commit(head_commit, with_commit)) {
+		struct ref_item item;
+		item.name = xstrdup("(no branch)");
+		item.kind = REF_LOCAL_BRANCH;
+		item.commit = head_commit;
+		if (strlen(item.name) > ref_list->maxwidth)
+			ref_list->maxwidth = strlen(item.name);
+		print_ref_item(&item, ref_list->maxwidth, verbose, abbrev, 1);
+		free(item.name);
+	}
+	for (i = 0; i < ref_list->index; i++) {
+		int current = is_current(&ref_list->list[i], detached);
+		print_ref_item(&ref_list->list[i], ref_list->maxwidth, verbose,
+			       abbrev, current);
+	}
+
+}
+
+static void print_branch(int kinds, int detached, int verbose, int abbrev,
+			 int only_current, struct commit_list *with_commit)
+{
 	struct ref_list ref_list;
-	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
+	int i;
 
 	memset(&ref_list, 0, sizeof(ref_list));
 	ref_list.kinds = kinds;
 	ref_list.with_commit = with_commit;
 	if (merge_filter != NO_FILTER)
 		init_revisions(&ref_list.revs, NULL);
+
 	for_each_ref(append_ref, &ref_list);
+
 	if (merge_filter != NO_FILTER) {
 		struct commit *filter;
 		filter = lookup_commit_reference_gently(merge_filter_ref, 0);
@@ -399,29 +436,24 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev, str
 
 	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
-	detached = (detached && (kinds & REF_LOCAL_BRANCH));
-	if (detached && head_commit && has_commit(head_commit, with_commit)) {
-		struct ref_item item;
-		item.name = xstrdup("(no branch)");
-		item.kind = REF_LOCAL_BRANCH;
-		item.commit = head_commit;
-		if (strlen(item.name) > ref_list.maxwidth)
-			ref_list.maxwidth = strlen(item.name);
-		print_ref_item(&item, ref_list.maxwidth, verbose, abbrev, 1);
-		free(item.name);
-	}
+	if (only_current) {
+		for (i = 0; i < ref_list.index; i++) {
+			if (!is_current(&ref_list.list[i], detached))
+				continue;
+			if (!matches_merge_filter(ref_list.list[i].commit))
+				continue;
 
-	for (i = 0; i < ref_list.index; i++) {
-		int current = !detached &&
-			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
-			!strcmp(ref_list.list[i].name, head);
-		print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
-			       abbrev, current);
+			printf("%s\n", ref_list.list[i].name);
+		}
+		goto free_it;
 	}
 
+	print_ref_list(&ref_list, kinds, detached, verbose,
+	    abbrev, with_commit);
+
+free_it:
 	free_ref_list(&ref_list);
 }
-
 static void rename_branch(const char *oldname, const char *newname, int force)
 {
 	struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
@@ -499,7 +531,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, force_create = 0;
 	int verbose = 0, abbrev = DEFAULT_ABBREV, detached = 0;
-	int reflog = 0;
+	int only_current = 0, reflog = 0;
 	enum branch_track track;
 	int kinds = REF_LOCAL_BRANCH;
 	struct commit_list *with_commit = NULL;
@@ -524,6 +556,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_HIDDEN | PARSE_OPT_LASTARG_DEFAULT,
 			opt_parse_with_commit, (intptr_t) "HEAD",
 		},
+		OPT_SET_INT('c', NULL, &only_current, "show only current branch", 1),
 		OPT__ABBREV(&abbrev),
 
 		OPT_GROUP("Specific git-branch actions:"),
@@ -576,9 +609,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	if (delete)
 		return delete_branches(argc, argv, delete > 1, kinds);
-	else if (argc == 0)
-		print_ref_list(kinds, detached, verbose, abbrev, with_commit);
-	else if (rename && (argc == 1))
+	else if (argc == 0) {
+		print_branch(kinds, detached, verbose, abbrev, only_current,
+		    with_commit);
+	}else if (rename && (argc == 1))
 		rename_branch(head, argv[0], rename > 1);
 	else if (rename && (argc == 2))
 		rename_branch(argv[0], argv[1], rename > 1);

^ permalink raw reply related

* GNOME DVCS Survey results - Interesting figures
From: Mike Hommey @ 2009-01-04  8:12 UTC (permalink / raw)
  To: git

Hi,

There has been a survey running in the GNOME community to have figures
whether they want to switch to a DVCS and which one would be preferred.

AFAIK, this is the biggest survey I've seen so far that is more or less
unbiased.
http://blogs.gnome.org/newren/2009/01/03/gnome-dvcs-survey-results/

Mike

^ permalink raw reply

* Re: [PATCH 2/3] unpack-trees: fix path search bug in verify_absent
From: Junio C Hamano @ 2009-01-04 10:01 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Clemens Buchacher, git
In-Reply-To: <alpine.DEB.1.00.0901031353090.30769@pacific.mpi-cbg.de>

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

> On Sat, 3 Jan 2009, Clemens Buchacher wrote:
>
>> On Fri, Jan 02, 2009 at 10:59:47PM +0100, Johannes Schindelin wrote:
>> > This explanation makes sense.  However, this:
>> > 
>> > > @@ -289,7 +289,8 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas
>> > >  	return 0;
>> > >  }
>> > >  
>> > > -static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
>> > > +static int unpack_callback(int n, unsigned long mask, unsigned long dirmask,
>> > > +		struct name_entry *names, struct traverse_info *info)
>> > >  {
>> > >  	struct cache_entry *src[5] = { NULL, };
>> > >  	struct unpack_trees_options *o = info->data;
>> > 
>> > ... is distracting during review, and this:
>> > 
>> > > @@ -517,22 +518,22 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
>> > >  	namelen = strlen(ce->name);
>> > >  	pos = index_name_pos(o->src_index, ce->name, namelen);
>> > >  	if (0 <= pos)
>> > > -		return cnt; /* we have it as nondirectory */
>> > > +		return 0; /* we have it as nondirectory */
>> > >  	pos = -pos - 1;
>> > >  	for (i = pos; i < o->src_index->cache_nr; i++) {
>> > 
>> > ... is not accounted for in the commit message.  Intended or not, that is 
>> > the question.
>> 
>> Those are trivial readability improvements in the context of the patch.
>
> They are not trivial enough for me not to be puzzled.  Reason enough to 
> explain in the commit message?

I'd say the first hunk quoted is probably on the borderline.  It is an
unnecessary churn that won't even be commented on if it were sent alone,
but as a "while we are at it" hunk in a patch that is not too big, this is
a kind of thing that often is tolerated, because it is obvious enough not
to hurt anything from the correctness standpoint [*1*].

The second one is moderately worse for two reasons.

 * I actually had to scratch my head because you need to view the change
   in a lot wider context that covers the initializing definition of "int
   cnt" near the beginning of the function down to the area affected by
   the hunk, in order to see that the new "return 0" is the same as the
   old "return cnt" and does not break things.  A comment to say that "at
   this point in the codeflow, cnt which is returned by the old code is
   always zero", perhaps below the three-dash marker, would have saved me
   a minute.

 * The function's purpose and logic is to see if the subdirectory is
   clean, and return how many cache entries need to be skipped if it is
   (otherwise a negative number as an error indicator).  For that purpose,
   the return value cnt is initialized to 0 (i.e. "we haven't counted any
   entry that needs to be skipped yet"), the loop below the patched part
   counts it up while performing the verification, and then the resulting
   count is returned from the function.  The logic flow, at least to me,
   is easier to follow if it returned the value in cnt, not a hardcoded 0,
   from the place the patch tries to touch.

The latter point is with "at least to me", because I think an alternate
position is entirely valid if the author wants to justify the change by
saying something like:

    The function's purpose is ....  Before entering the loop to count the
    number of entries to skip, this check to detect if we do not even have
    to count appears.  When this check triggers, we know we do not want to
    skip anything, and returning constant 0 is much clearer than returning
    a variable cnt that was initialized to 0 near the beginning of the
    function; we haven't even started using it to count yet.

But the point is, if that is the reason the author thinks it is an
improvement, that probably needs to be stated.

[Footnote]

*1* I am not sure if it is obviously clear that the change improves any
readability.  Some people argue that splitting the function definition
header hurts greppability for one thing.  I personally do not find it easy
to read when the subsequent header lines are indented without aligning
(compare the way it is indented in the postimage of the patch with the way
the headers verify_absent() and show_stage_entry() are indented), either.

^ permalink raw reply

* Re: [PATCH 3/3] pretty: support multiline subjects with format:
From: Junio C Hamano @ 2009-01-04 10:01 UTC (permalink / raw)
  To: René Scharfe; +Cc: markus.heidelberg, git
In-Reply-To: <49594C16.2010406@lsrfire.ath.cx>

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

>> It's unchanged since it has it's own commit message parser.
>
> ... which displays the first line of the commit message, unlike
> --pretty=oneline.  Here's a quick fix.  I probably won't have time
> to come up with something prettier this year.

I think the code is pretty enough ;-)  I just need to come up with a patch
description and forge your Sign-off.

Thanks.

^ permalink raw reply

* Re: [PATCH] cvsserver: add option to configure commit message
From: Junio C Hamano @ 2009-01-04 10:01 UTC (permalink / raw)
  To: Fabian Emmes; +Cc: git, gitster, Lars Noschinski
In-Reply-To: <1230910814-32307-1-git-send-email-fabian.emmes@rwth-aachen.de>

Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:

> cvsserver annotates each commit message by "via git-CVS emulator". This is
> made configurable via gitcvs.commitmsgannotation.
>
> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>

I do not see the development history behind this and am somewhat puzzled
by these two S-o-b lines.  Is it "Fabian developed it, showed it to Lars
who cleaned it up and/or enhanced it and here is the result"?  Or is it
"Lars developed it, circulated it in his closer circle, Fabian found it
useful and worthy for inclusion and sending it to the mailing list"?

Whichever it is, I just will take it as "This is co-developed and between
the authors Fabian is the primary author" and apply.

Thanks.

^ permalink raw reply

* Re: What about allowing multiple hooks?
From: Junio C Hamano @ 2009-01-04 10:01 UTC (permalink / raw)
  To: Alexander Potashev; +Cc: Marc Weber, git, Rogan Dawes, martin f krafft
In-Reply-To: <20090103233252.GA12095@myhost>

Alexander Potashev <aspotashev@gmail.com> writes:

>> Thoughts?

I deliberately omitted support for multiple scripts in core git Porcelains
to avoid this exact issue.  It is a huge can of worms and it is dubious if
you can have a coherent and generic enough semantics.

In the meantime, you can have a single .git/hooks/pre-commit script that
defines your own convention.  Maybe it uses .git/hooks/pre-commit.d/
directory, full of scripts, and implements the semantics you want,
including:

 (1) the execution order and the naming convention of the scripts (e.g.
     they all live in pre-commit.d/ directory, and executed in ASCII byte
     value order of their names);

 (2) how their exit status combine together.  For example, maybe a failure
     from one of the scripts prevents none of the later scripts to even
     run and make the whole hook return a failure; maybe a failure will be
     remembered, but the other scripts may still want to be run to learn
     about the fact that the commit was attempted, and the whole hook
     returns a failure if any of them fail.

     In a hook that is run primarily for its side effects and not for
     validation, it may even be desireble if the whole hook returns a
     failure only when all of them fail, iow, for such a hook the status
     is not ANDed but ORed together.

Once you have such a framework and get help from others to widely try it
in the field, it may prove generic enough to include it as the sample hook
script to be installed everywhere.

^ permalink raw reply

* Re: git-branch --print-current
From: Alexandre Dulaunoy @ 2009-01-04 10:07 UTC (permalink / raw)
  To: Git mailing list
In-Reply-To: <quack.20090101T1928.lthzliaqtdf@roar.cs.berkeley.edu>

On Fri, Jan 2, 2009 at 4:28 AM, Karl Chen <quarl@cs.berkeley.edu> wrote:
>
> How about an option to git-branch that just prints the name of the
> current branch for scripts' sake?  To replace:
>
>    git branch --no-color 2>/dev/null | perl -ne '/^[*] (.*)/ && print $1'

I tend to support your request especially that extracting the current
branch is something that is done regularly. Looking in my own scripts/aliases
and some of my colleagues, there are plenty of variation using Perl,
sed, awk, tr
and Python to extract the current branch.

Using git-symbolic-ref is not obvious, especially that the summary/name
 of the man page is :

"git-symbolic-ref - Read and modify symbolic refs"

But the description is pretty clear :

"Given one argument, reads which branch head the given symbolic ref refers to
and outputs its path, relative to the .git/ directory. Typically you
would give HEAD
as the <name> argument to see on which branch your working tree is on."

But naturally, as a lazy user, you will pick git-branch especially
that's the tools is listed
with the most commonly used git commands with a very attractive description :

"branch     List, create, or delete branches"

On an user perspective, having the option in git-branch seems more natural.

Just a comment,

-- 
--                   Alexandre Dulaunoy (adulau) -- http://www.foo.be/
--                             http://www.foo.be/cgi-bin/wiki.pl/Diary
--         "Knowledge can create problems, it is not through ignorance
--                                that we can solve them" Isaac Asimov

^ permalink raw reply

* Re: [PATCH] cvsserver: change generation of CVS author names
From: Lars Noschinski @ 2009-01-04 11:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Fabian Emmes, git, Frank Lichtenheld, Martin Langhoff
In-Reply-To: <7vwsdc3ulg.fsf@gitster.siamese.dyndns.org>

* Junio C Hamano <gitster@pobox.com> [09-01-03 23:36]:
>Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:
>
>> CVS username is generated from local part email address.
>> We take the whole local part but restrict the character set to the
>> Portable Filename Character Set, which is used for Unix login names
>> according to Single Unix Specification v3.
>>
>> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
>> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
>
>Stating "we should have done this from day one" is one thing (even though
>"because some standard says so" is not particularly a good justification
>without "and matches the way people use CVS in the real world in practice"
>appended to it).

Documentation about valid cvs/rcs usernames is a bit scarce. When we
wrote the patch, we did not find much more information than "the cvs
username is supposed to be the login name". In my limited CVS
experience, I never saw CVS user names which were not (unix) login
names.

After this mail, I looked to the RCS source code (see checkidentifier()
in rcslex.c) which tells us that anything (encoded in ISO-8859-1)
consisting of IDCHAR, LETTER, Letter, DIGIT and PERIOD, containing at
least one IDCHAR, LETTER or Letter is a valid username (for the
character classes, see
http://avalon.hoffentlich.net/~cebewee/rcs-charmap.txt) The most
important character _not_ allowed in an user name is the @ sign, so we
cannot use the full mail address.

So our patch generates a valid username for any "sane" local part. In a
few corner cases like "!#$%&'*+-/=?^_`.{|}~@example.com" our patch
generates a result worse than the original - an empty username. This
is probably something we should fix.

Obviously, the short names generated are not necessarily unique, which
can be irritating, but is not a problem from a technical point of view.
Improving this would probably require to store a map of mail addresses
to cvs user names.

>"We should suddenly change the behaviour" is quite a different thing and
>it depends on what follows that sentence if the change is justifiable.  We
>do not want to hear "...; screw the existing repositories if they have
>nonconforming names.".  It is Ok if it is "...; existing repositories will
>be affected, but the damage is limited to very minor set of operations,
>namely X, Y and Z".
>
>In other words, is there any backward compatibility issue when a
>repository that has served existing CVS users and checkouts with older
>version switches to the patched one?  If there is one, is that grave
>enough that we should care?

Obviously the reported user names change. To the best of my knowledge
(but I'm just a barely experienced CVS user) those names are not stored
anywhere on the client and are regenerated by git-cvsserver for every
request, so even old repositories get the new names for all commits.

   - Lars.

^ permalink raw reply

* Re: What's cooking in git.git (Dec 2008, #04; Mon, 29)
From: Christian Couder @ 2009-01-04 11:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhc4gadg9.fsf@gitster.siamese.dyndns.org>

Le samedi 3 janvier 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Le mardi 30 décembre 2008, Junio C Hamano a écrit :
> >> * cc/bisect-replace (Mon Nov 24 22:20:30 2008 +0100) 9 commits
> >>  - bisect: add "--no-replace" option to bisect without using replace
> >>    refs
> >>  - rev-list: make it possible to disable replacing using "--no-
> >>    bisect-replace"
> >>  - bisect: use "--bisect-replace" options when checking merge bases
> >>  - merge-base: add "--bisect-replace" option to use fixed up revs
> >>  - commit: add "bisect_replace_all" prototype to "commit.h"
> >>  - rev-list: add "--bisect-replace" to list revisions with fixed up
> >>    history
> >>  - Documentation: add "git bisect replace" documentation
> >>  - bisect: add test cases for "git bisect replace"
> >>  - bisect: add "git bisect replace" subcommand
> >>
> >> I think a mechanism like this should be added to replace grafts,
> >
> > The problem with replacing grafts is that a graft can specify many
> > parents for one commit while a ref associates only one object to a
> > name.
>
> Sorry, maybe I misunderstood your implementation.  What I thought we
> discussed during GitTogether was to write out the object name of the
> replacement object in refs/replace/<sha1>.
>
> When the caller asks read_sha1_file() for an object whose object name is
> <sha1>, you see if there is refs/replace/<sha1> in the repository, and
> read the ref to learn the object name of the object that replaces it. 
> And you return that as if it is the original object.

Ok. When I first implemented "bisect replace" I saw that I could reuse the 
graft fix-up mechanism. And as you talked about replacing grafts, I thought 
that you wanted the implementation to use that mechanism instead of adding 
a different one.

But I agree that it may be more powerfull and generic to replace objects the 
way you describe it. So I will work on that.

Thanks,
Christian.

^ permalink raw reply

* Re: [PATCH] cvsserver: add option to configure commit message
From: Lars Noschinski @ 2009-01-04 11:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Fabian Emmes, git
In-Reply-To: <7viqovz8y0.fsf@gitster.siamese.dyndns.org>

* Junio C Hamano <gitster@pobox.com> [09-01-04 12:13]:
>Fabian Emmes <fabian.emmes@rwth-aachen.de> writes:
>
>> cvsserver annotates each commit message by "via git-CVS emulator". This is
>> made configurable via gitcvs.commitmsgannotation.
>>
>> Signed-off-by: Fabian Emmes <fabian.emmes@rwth-aachen.de>
>> Signed-off-by: Lars Noschinski <lars@public.noschinski.de>
>
>I do not see the development history behind this and am somewhat puzzled
>by these two S-o-b lines.  Is it "Fabian developed it, showed it to Lars
>who cleaned it up and/or enhanced it and here is the result"?  Or is it
>"Lars developed it, circulated it in his closer circle, Fabian found it
>useful and worthy for inclusion and sending it to the mailing list"?

It is "Fabian and Lars developed it and Fabian is the one who mailed it
for inclusion". We could just leave off the second S-o-b line, if this
is less irritating?

>Whichever it is, I just will take it as "This is co-developed and between
>the authors Fabian is the primary author" and apply.

Fine with me.

  - Lars.

^ permalink raw reply

* Re: [PATCH] gitweb: merge boolean feature subroutines
From: demerphq @ 2009-01-04 11:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matt Kraai, git
In-Reply-To: <7vvdsv3af6.fsf@gitster.siamese.dyndns.org>

2009/1/4 Junio C Hamano <gitster@pobox.com>:
> demerphq <demerphq@gmail.com> writes:
>
>> 2009/1/3 Matt Kraai <kraai@ftbfs.org>:
>> [...]
>>> -sub feature_blame {
>>> -       my ($val) = git_get_project_config('blame', '--bool');
>>> +sub feature_bool {
>>> +       my $key = shift;
>>> +       my ($val) = git_get_project_config($key, '--bool');
>>>
>>>        if ($val eq 'true') {
>>>                return 1;
>>
>> Maybe that should be:
>>
>>            return ($val eq 'true');
>>
>> as It is not a good idea to use 0 as a replacement for perls false, as
>> the two have different behaviour.
>
> I'd rather want to keep our scripts free of deep Perl magic.  Even if
> there are SVs that are interpreted as false other than 0, that does not
> necessarily mean you have to fear that 0 can sometimes evaluate to true.

No, thats not the point. The point is that why should the code do more
work to return a value that a perl programmer might find unexpected.
Especially when the function has the word "bool" in it. Its like
writing a function whose name says "int" that returns a double. If the
routine was not called "bool" then it wouldnt matter at all.

> As long as you refrain from doing something crazy like "0 but true",
> people who are not (and/or are not inclined to become) familiar with the
> gory innards of Perl can rely on 0 being false and 1 being true when
> calling feature_something subs, no?

Why execute more opcodes, and return a slightly surprising false, when
you dont have to?

Is it really deep perl magic to do:

  return $val eq 'true';

instead of

  return $val eq 'true' ? 1 : 0;

or the actual use:

   if ($val eq 'true') {
      return 1
   } else {
      return 0
   }

Isn't the former superior just on pure minimalism metrics? Theres less
code to understand, less code to go wrong, and as a bonus it returns a
true boolean. Isn't that just a win-win-win? I mean most perl
programmers I know would instantly convert the latter two to the first
just on the grounds that the first version is the clearest expression
of the desired intent.

Anyway, leave it or not, its a minor nit.

cheers,
Yves




-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: [PATCH] strbuf_readlink semantics update.
From: Pierre Habouzit @ 2009-01-04 12:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: René Scharfe, Linus Torvalds, git
In-Reply-To: <7viqp8afap.fsf@gitster.siamese.dyndns.org>

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

On Thu, Dec 25, 2008 at 07:23:58AM +0000, Junio C Hamano wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> > Pierre Habouzit schrieb:
> >> On Tue, Dec 23, 2008 at 06:16:01PM +0000, Linus Torvalds wrote:
> >>>
> >>> On Tue, 23 Dec 2008, Pierre Habouzit wrote:
> >>>> when readlink fails, the strbuf shall not be destroyed. It's not how
> >>>> read_file_or_gitlink works for example.
> >>> I disagree.
> >>>
> >>> This patch just makes things worse. Just leave the "strbuf_release()" in 
> >>> _one_ place.
> > ...
> > The "append or do nothing" rule is broken by strbuf_getline(), but I agree
> > to your reasoning.  How about refining this rule a bit to "do your thing
> > and roll back changes if an error occurs"?  I think it's not worth to undo
> > allocation extensions, but making reverting first time allocations seems
> > like a good idea.  Something like this?
> 
> I think this is much better than Pierre's.

I agree it's a fine semantics.

> Pierre's "if it is called strbuf_*, it should always append" is a good
> uniformity to have in an API, but making the caller suffer for
> clean-up is going backwards.  The reason we use strbuf when we can is
> so that the callers do not have to worry about memory allocation
> issues too much.

Ack.

Sorry for the delay I was on vacation.

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

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

^ permalink raw reply

* Re: git-branch --print-current
From: demerphq @ 2009-01-04 12:31 UTC (permalink / raw)
  To: Alexandre Dulaunoy; +Cc: Git mailing list
In-Reply-To: <1baa801f0901040207r64195594m64359dbc60a5f662@mail.gmail.com>

2009/1/4 Alexandre Dulaunoy <adulau@gmail.com>:
> On Fri, Jan 2, 2009 at 4:28 AM, Karl Chen <quarl@cs.berkeley.edu> wrote:
>>
>> How about an option to git-branch that just prints the name of the
>> current branch for scripts' sake?  To replace:
>>
>>    git branch --no-color 2>/dev/null | perl -ne '/^[*] (.*)/ && print $1'
>
> I tend to support your request especially that extracting the current
> branch is something that is done regularly. Looking in my own scripts/aliases
> and some of my colleagues, there are plenty of variation using Perl,
> sed, awk, tr
> and Python to extract the current branch.
>
> Using git-symbolic-ref is not obvious, especially that the summary/name
>  of the man page is :
>
> "git-symbolic-ref - Read and modify symbolic refs"
>
> But the description is pretty clear :
>
> "Given one argument, reads which branch head the given symbolic ref refers to
> and outputs its path, relative to the .git/ directory. Typically you
> would give HEAD
> as the <name> argument to see on which branch your working tree is on."
>
> But naturally, as a lazy user, you will pick git-branch especially
> that's the tools is listed
> with the most commonly used git commands with a very attractive description :

I dont think it has to do with lazyness. It has to do with the fact
that parsing git branch gives you a branch name that you can use an an
argument to many other git commands. Whereas git-symbolic-ref doesnt.
It requires additional post-processing that unless you are very git
aware is not at all clear. Like for instance in this thread the
recommendation is to use sed like this:

  git symbolic-ref HEAD|sed s,refs/heads/,,

however, that makes me think "how do i do that on a windows box? does
the presence of git on a  windows box mean that they will necessarily
have sed available? Can i rely on that? Can i rely on the sed rule
being sufficient? And what happens with this command if im not on a
branch at all? Well it turns out that git symbolic-ref HEAD *dies*
with a fatal error on this command.  SO it probably should be:

  git symbolic-ref HEAD 2>/dev/null|sed s,refs/heads/,,

but now its even less portable. Even if sed is available on windows
/dev/null isnt.

Id very much like a proper way to find the usable form of the branch
name as it would make a lot of thing easier. In particular requiring
people use pipes means that there is a portability issue with scripts.
How does one make this happen on a windows box for instance?

Id also very much like a way to find the "upstream" for a branch. IOW,
id very much like to know where I will push to if i issue a "git push"
command, or what i will merge if i do a git pull. There doesnt seem to
be an easy way to find this out currently. And its very useful
information.

Im coming from the point of view of someone trying to make the perl
build process a bit more "git aware". Unfortunately Perl has to build
out of the box on so many platforms that unix-centric tricks like huge
command pipes arent very helpful. They immediately fall down when you
start dealing with oddball platforms like Windows and VMS.

Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* git-rev-parse --symbolic-abbrev-name [was Re: git-branch --print-current]
From: Karl Chen @ 2009-01-04 12:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Miklos Vajna, David Aguilar, Git mailing list
In-Reply-To: <7vzli73b1g.fsf@gitster.siamese.dyndns.org>

>>>>> On 2009-01-03 21:17 PST, Junio C Hamano writes:

    Junio> That is a good point about user education, and is a
    Junio> demonstration why a new option to cover a very
    Junio> narrow-special case to symbolic-ref will not help the
    Junio> situation.  People will add their own embellishments
    Junio> around the name of the branch anyway, and the most
    Junio> generic symbolic-ref output is just as useful as a
    Junio> special case option to show without refs/heads/.

That's arguable :) you really think "branchfoo" instead of
"refs/heads/branchfoo" is a narrow special case?  Seems like a
common case for everyone except plumbing tools.

Here's a more general idea you might like better:

    git symbolic-ref --abbrev BLAH
or even
    git rev-parse --symbolic-abbrev-name BLAH

This would be like git-rev-parse --symbolic-full-name, but strips
the "refs/x/" iff the result is unambiguous.  Since it's much more
work for a script to check whether the stripped version is
ambiguous, this functionality is appropriate as a builtin option.

(Hmm, I guess to be able to specify a ref it has to already be
unambiguous, so the main use that --symbolic doesn't already cover
is for symbolic refs such as HEAD.)

    Junio> What you quoted are all inferior implementations of
    Junio> showing the name of the current branch in the bash
    Junio> prompt.

Yup, that was the point - it's so ugly seeing all these things
floating around, but that's where things stand right now.

    Junio> ... __git_ps1 shell function is defined to be used for
    Junio> this exact purpose and is documented in the completion
    Junio> script.

Thanks for the detailed explanation.  I actually use zsh rather
than of bash and I did already find git-completion.bash.  But
obviously all those people posting on blogs don't know about it :)

^ permalink raw reply

* Re: git-rev-parse --symbolic-abbrev-name [was Re: git-branch --print-current]
From: demerphq @ 2009-01-04 12:40 UTC (permalink / raw)
  To: Karl Chen; +Cc: Junio C Hamano, Miklos Vajna, David Aguilar, Git mailing list
In-Reply-To: <quack.20090104T0434.lthfxjz1c8x_-_@roar.cs.berkeley.edu>

2009/1/4 Karl Chen <quarl@cs.berkeley.edu>:
>>>>>> On 2009-01-03 21:17 PST, Junio C Hamano writes:
>
>    Junio> That is a good point about user education, and is a
>    Junio> demonstration why a new option to cover a very
>    Junio> narrow-special case to symbolic-ref will not help the
>    Junio> situation.  People will add their own embellishments
>    Junio> around the name of the branch anyway, and the most
>    Junio> generic symbolic-ref output is just as useful as a
>    Junio> special case option to show without refs/heads/.
>
> That's arguable :) you really think "branchfoo" instead of
> "refs/heads/branchfoo" is a narrow special case?  Seems like a
> common case for everyone except plumbing tools.

I agree. All the scripting I've done involves using the non qualified form.

> Here's a more general idea you might like better:
>
>    git symbolic-ref --abbrev BLAH
> or even
>    git rev-parse --symbolic-abbrev-name BLAH
>
> This would be like git-rev-parse --symbolic-full-name, but strips
> the "refs/x/" iff the result is unambiguous.  Since it's much more
> work for a script to check whether the stripped version is
> ambiguous, this functionality is appropriate as a builtin option.

I vote for this, I could and would use it many scripts. Also please
dont make it die if BLAH is not a symbolic ref if this option is used.
Just return nothing.

cheers,
Yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: git-branch --print-current
From: Karl Chen @ 2009-01-04 12:40 UTC (permalink / raw)
  To: Arnaud Lacombe; +Cc: Git mailing list
In-Reply-To: <1a69a9d80901040021i1dae2c6j7337cf57eed6476a@mail.gmail.com>

>>>>> On 2009-01-04 00:21 PST, Arnaud Lacombe writes:

    Arnaud> FWIW, I had this in a stalled modification in a tree,
    Arnaud> it just add the '-c' (as "current") option to git
    Arnaud> branch. Patch is mostly for the record :/

Thanks, glad someone else wanted this too.  If we modified
git-symbolic-ref it would probably be less code since it doesn't
have to loop over all branches, though from a UI perspective I
still prefer git-branch.  Anyway doesn't look like people like the
idea so how about that git-rev-parse --symbolic-abbrev-name idea
:)

    Arnaud> The main trouble I have with pipe stuff is that it
    Arnaud> forks a process for something that can be done
    Arnaud> natively. Previously, I was using awk(1) to extract
    Arnaud> the current branch:

    Arnaud> $ git branch | awk '/^\*/ {print $2}'

Yet another addition to the list of ways to pipeline it, this one
probably the shortest :)

[BTW, your patch mime type was application/octet-stream :(]

^ permalink raw reply

* Re: git-branch --print-current
From: demerphq @ 2009-01-04 12:49 UTC (permalink / raw)
  To: Karl Chen; +Cc: Arnaud Lacombe, Git mailing list
In-Reply-To: <quack.20090104T0440.lthbpun1bxo@roar.cs.berkeley.edu>

2009/1/4 Karl Chen <quarl@cs.berkeley.edu>:
>>>>>> On 2009-01-04 00:21 PST, Arnaud Lacombe writes:
>
>    Arnaud> FWIW, I had this in a stalled modification in a tree,
>    Arnaud> it just add the '-c' (as "current") option to git
>    Arnaud> branch. Patch is mostly for the record :/
>
> Thanks, glad someone else wanted this too.  If we modified
> git-symbolic-ref it would probably be less code since it doesn't
> have to loop over all branches, though from a UI perspective I
> still prefer git-branch.  Anyway doesn't look like people like the
> idea so how about that git-rev-parse --symbolic-abbrev-name idea
> :)

FWIW: I like the idea. Ive always thought that a --current flag to git
branch was missing. IOW i should be able to do:

   branch=`git branch --current`

and get back a usable branch name. I dont think one should need to
rely on awk or sed or scripts to find this out, if only for
portability reasons.

>
>    Arnaud> The main trouble I have with pipe stuff is that it
>    Arnaud> forks a process for something that can be done
>    Arnaud> natively. Previously, I was using awk(1) to extract
>    Arnaud> the current branch:
>
>    Arnaud> $ git branch | awk '/^\*/ {print $2}'
>
> Yet another addition to the list of ways to pipeline it, this one
> probably the shortest :)

Unfortunately it doesnt work well when you arent on a branch:

  $ git branch | awk '/^\*/ {print $2}'
  (no

So far two apparently expert git people have given solutions to this
problem that don't elegantly handle the edge cases.

That seems to me to be a powerful argument that it is actually more
difficult to do than is being represented here on the list, and
deserves to be native level git functionality.

Cheers,
yves



-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: a few Topgit patches
From: Uwe Kleine-König @ 2009-01-04 13:05 UTC (permalink / raw)
  To: martin f krafft; +Cc: git, Petr Baudis
In-Reply-To: <20081226170334.GA18722@pengutronix.de>

Hi martin,

On Fri, Dec 26, 2008 at 06:03:34PM +0100, Uwe Kleine-König wrote:
> On Thu, Dec 25, 2008 at 03:58:34PM +0100, martin f krafft wrote:
> > also sprach Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [2008.12.23.1530 +0100]:
> > > I hacked using topgit for some time now, and found the following changes
> > > to topgit useful:
> > > 
> > > Uwe Kleine-König (3):
> > >       tg export: implement skipping empty patches for collapse driver
> > >       tg export: Implement flattening patch paths for quilt mode
> > >       tg export (quilt): Implement numbering the patches
> > 
> > They all look good. I am a bit concerned about the use of
> > single-letter options at this stage. tg-export is bound to grow, and
> > using them all up now might mean breaking compatibility later, when
> > a more common option needs e.g. -f, which has already been taken.
> I updated my tree to make '-f' '--flatten'.  IMHO -n is nice for getting
> numbered patches, but if you prefer, I will make this --numbered.
> 
> Oh, and I just noticed that documentation is missing.  I will fix this
> when I sent this mail.
This is done.  Now I even changed '-n' to '--numbered' and changed the
commit log accordingly.  Are there still concerns about my patches?
Should I resend the current version?

The patches are still available in my topgit repo at

	git://git.pengutronix.de/git/ukl/topgit.git master

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |
Peiner Strasse 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686              | Fax:   +49-5121-206917-5555 |

^ permalink raw reply

* Re: [RFC/PATCH] git-web--browse: don't add start as candidate on Ubuntu
From: Petr Baudis @ 2009-01-04 13:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Christian Couder, Ramsay Jones, GIT Mailing-list
In-Reply-To: <7vljtr33sb.fsf@gitster.siamese.dyndns.org>

On Sat, Jan 03, 2009 at 11:53:56PM -0800, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> 
> > Le jeudi 1 janvier 2009, Ramsay Jones a écrit :
> > ...
> >> Does anybody else see this issue and can someone test the patch?
> >
> > Petr, as you added support for using /bin/start on MinGW, could you test?

Sorry, I don't have access to MinGW system anymore.

> > diff --git a/git-web--browse.sh b/git-web--browse.sh
> > index 78d236b..7ed0fad 100755
> > --- a/git-web--browse.sh
> > +++ b/git-web--browse.sh
> > @@ -115,7 +115,7 @@ if test -z "$browser" ; then
> >  	browser_candidates="open $browser_candidates"
> >      fi
> >      # /bin/start indicates MinGW
> > -    if test -n /bin/start; then
> > +    if test -x /bin/start; then
> >  	browser_candidates="start $browser_candidates"
> >      fi
> 
> In any case, the original test is simply bogus.  'test -n "$foo"' is to
> see if $foo is an empty string, and giving a constant /bin/start always
> yields true.

Oops. :-) That should've obviously been -f.

-- 
				Petr "Pasky" Baudis
The average, healthy, well-adjusted adult gets up at seven-thirty
in the morning feeling just terrible. -- Jean Kerr

^ permalink raw reply

* Re: git-branch --print-current
From: demerphq @ 2009-01-04 13:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Karl Chen, Miklos Vajna, David Aguilar, Git mailing list
In-Reply-To: <7vzli73b1g.fsf@gitster.siamese.dyndns.org>

2009/1/4 Junio C Hamano <gitster@pobox.com>:
> Karl Chen <quarl@cs.berkeley.edu> writes:
>
>> For example: Google for how to add the name of the git branch to
>> the bash prompt and you'll find countless examples of people using
>> git-branch.  And they're all different, so people aren't just
>> blindly copying one guy; here is a small sample:
>> ...
>> There were a few using git-symbolic-ref but most used git-branch.
>
> That is a good point about user education, and is a demonstration why a
> new option to cover a very narrow-special case to symbolic-ref will not
> help the situation.  People will add their own embellishments around the
> name of the branch anyway, and the most generic symbolic-ref output is
> just as useful as a special case option to show without refs/heads/.
>
> What you quoted are all inferior implementations of showing the name of
> the current branch in the bash prompt.  The most correct way (in the sense
> that it won't be broken in future git) is always found in the bash
> completion script in contrib/completion/git-completion.bash and it reads:
>
>    PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ '
>
> You can of course change this to suit your taste.  For example, here is a
> variant I personally use:
>
>    PS1=': \h \W$(__git_ps1 "/%s"); '
>
> The point is that __git_ps1 shell function is defined to be used for this
> exact purpose and is documented in the completion script.
>
> Besides showing the current branch, it knows how to interpret the various
> state clues git operations leave in the repository and the work tree, and
> reminds them what you are in the middle of (e.g. applying patch series
> using "git am", rebasing interactively, resolving conflicts after a merge
> did not autoresolve, etc.), and also knows how to show the detached HEAD.

The version im using, from git version 1.6.0.4.724.ga0d3a produces the
following error:

cut: ./HEAD: No such file or directory

when in the .git/refs directory.

Cheers,
yves


-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

^ permalink raw reply

* Re: [PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X
From: Wincent Colaiuta @ 2009-01-04 13:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Marcel M. Cary, git, jnareb, ae, j.sixt
In-Reply-To: <7v8wps59ss.fsf@gitster.siamese.dyndns.org>

El 3/1/2009, a las 23:01, Junio C Hamano escribió:

> "Marcel M. Cary" <marcel@oak.homeunix.org> writes:
>
>> I sent the first rev of this patch to just Brian.  It didn't have
>> either of the unit test changes.  He said it fixed all but t2300.3,
>> where cd_to_toplevel doesn't actually "cd", so I made the same change
>> to the unit test itself.  Can someone with OS X try running the test
>> suite with v2 of this patch?  I don't have OS X readily available.
>
> I think I saw a success report on the list.  Care to resend it with
> Sign-off (by you) and
>
> 	Tested-by: tester <test@er.xz> (on PLATFORM)
>
> lines as you see necessary for application?
>
> Thanks.

I also tested it and can confirm that it fixes the failures on Mac OS  
X 10.5.5. So feel free to add:

Tested-by: Wincent Colaiuta <win@wincent.com> (on Mac OS X 10.5.5)

Cheers,
Wincent

^ permalink raw reply

* announce: TortoiseGit 0.2 preview released
From: Frank Li @ 2009-01-04 15:15 UTC (permalink / raw)
  To: git, tortoisegit-announce

After heavy developing, TortoiseGit 0.2 preview release published.
TortoiseGit 0.2 can finish regular work, such as commit, show log,
diff version, create tag, create branch, create patchs.

Download:
http://code.google.com/p/tortoisegit/downloads/list

Version 0.2.0.0 preview version:
Features:
 1. Add TortoisgeMerge as default compare tools
 2. Pull, Fetch, Push
 3. Create Branch\Tag
 4. Switch branch \Chechout
 5. Compare with previous version
 6. Clone(only support local repository, see known issue for detail)
 7. Log Dialog support filter
 8. Check for modifications
 9. Revert local change
 10.Create Patch Serial
 11.Apply Patch Serial
 12.Add file to repository(see know issue)
 13.Export to zip file

Bug Fix:
 1. A2W cause stack overwrite bug when git output is long.


Known Issue:
 1. ProcessDlg will wait for ever when clone remote repository(ssh, http,git).
 2. push fetch and pull don't support password mode. Just support
public key problem.
 3. Just fetch first 100 log item.
 4. If install TortoiseGit before MsysGit, you need modify register
     HKEY_LOCAL_MACHINE\Software\TortoiseGit\\MsysGit\
    Let it point to correct msysgit install path.
 5. Add File, please commit and show unversion file, the choose add
file, then right clict, Choose Add file
 6. To new initial repository, You will not see add file again in
commit dialog box if give up commit when choose add

^ permalink raw reply

* [Q] post-commit hook to backup repo by emailing git bundle
From: Alexander Gladysh @ 2009-01-04 15:27 UTC (permalink / raw)
  To: git

Hi, list!

I have a server, where I store my light-weight private data (a wiki
contents) inside a git repo. I need a backup solution for this data.

Setting up a mirror repository and fetching/pushing over the network
is highly uncomfortable for a number of reasons for this specific
case.

I'm thinking of setting up a post-commit hook on my wiki repo, which
would prepare git bundle for the commit and send it to me via e-mail.

Perhaps someone already has something like this, so I would not have
to reimplement the wheel?

Alexander.

^ permalink raw reply

* Re: [PATCH] gitweb: merge boolean feature subroutines
From: Matt Kraai @ 2009-01-04 15:58 UTC (permalink / raw)
  To: demerphq; +Cc: Junio C Hamano, git
In-Reply-To: <9b18b3110901040341n5ff5fa09s878228131d11d2a6@mail.gmail.com>

On Sun, Jan 04, 2009 at 12:41:14PM +0100, demerphq wrote:
> Why execute more opcodes, and return a slightly surprising false, when
> you dont have to?
> 
> Is it really deep perl magic to do:
> 
>   return $val eq 'true';
> 
> instead of
> 
>   return $val eq 'true' ? 1 : 0;
> 
> or the actual use:
> 
>    if ($val eq 'true') {
>       return 1
>    } else {
>       return 0
>    }
>
> Isn't the former superior just on pure minimalism metrics? Theres less
> code to understand, less code to go wrong, and as a bonus it returns a
> true boolean. Isn't that just a win-win-win? I mean most perl
> programmers I know would instantly convert the latter two to the first
> just on the grounds that the first version is the clearest expression
> of the desired intent.

I agree that what you suggest is better than the alternatives you
present.  Unfortunately, none of them match the current behavior.
Here's the current code:

	if ($val eq 'true') {
		return 1;
	} elsif ($val eq 'false') {
		return 0;
	}

	return $_[0];

Is there a way to use the form you suggest while falling back to the
default if $val isn't set to 'true' or 'false'?

-- 
Matt                                                 http://ftbfs.org/

^ 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