Git development
 help / color / mirror / Atom feed
* Re: [RFC] Git Wiki Move
From: Junio C Hamano @ 2010-01-16  0:12 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: J.H., Matthieu Moy, Petr Baudis, git
In-Reply-To: <fabb9a1e1001151608k6911bcf8p854d97c2f2d46264@mail.gmail.com>

Sverre Rabbelier <srabbelier@gmail.com> writes:

> From the original faq:
>
> <<GitLink(git-name, Because Linus is an egotistical git)>>
>
> Is translated to
>
> <<[[GitLink]](git-name, Because Linus is an egotistical git}}
>
> But should instead be turned into a comment (from the source of the faq):
>
> <!-- GitLink[git-name] Because Linus is an egotistical git -->
>
>> There's some
>> extraneous bits on the page that I haven't figured out what they were
>> for originally but most everything on the page seems to work fine for me...
>
> We're probably referring to the same then :).

IIRC, they are read by gitbot listening to #git channel at freenode, so
that people can say "faq git-name" and such to summon a response.

^ permalink raw reply

* Re: [RFC] Git Wiki Move
From: J.H. @ 2010-01-16  0:28 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: Matthieu Moy, Petr Baudis, git
In-Reply-To: <fabb9a1e1001151608k6911bcf8p854d97c2f2d46264@mail.gmail.com>

On 01/15/2010 04:08 PM, Sverre Rabbelier wrote:
> Heya,
> 
> On Sat, Jan 16, 2010 at 01:02, J.H. <warthog19@eaglescrag.net> wrote:
>> If you can be more specific I can go through and fix it...
> 
>>From the original faq:
> 
> <<GitLink(git-name, Because Linus is an egotistical git)>>
> 
> Is translated to
> 
> <<[[GitLink]](git-name, Because Linus is an egotistical git}}
> 
> But should instead be turned into a comment (from the source of the faq):
> 
> <!-- GitLink[git-name] Because Linus is an egotistical git -->
> 
>> There's some
>> extraneous bits on the page that I haven't figured out what they were
>> for originally but most everything on the page seems to work fine for me...
> 
> We're probably referring to the same then :).

Probably, should be fixed now, found it on GitBot too.

- John 'Warthog9' Hawley

^ permalink raw reply

* Re: [msysGit] [PATCH] Installer: Create builtins as symbolic links on Vista
From: Johannes Schindelin @ 2010-01-16  0:41 UTC (permalink / raw)
  To: Michael Lukashov; +Cc: msysgit, git, Sebastian Schuberth
In-Reply-To: <1263593219-6032-1-git-send-email-michael.lukashov@gmail.com>

Hi,

On Fri, 15 Jan 2010, Michael Lukashov wrote:

> When create builtins, first try to use CreateSymbolicLinkW function. If 
> symbolic link creating fails, hard links are created.
> 
> Tested under WinXP (both FAT32 and NTFS), Vista and Win7.
> 
> This patch applies on top of ss/installer-improvements branch.

The commit message is a wonderful place to convince readers that the 
patched code has a tremendous advantage over the existing code.

Maybe you forgot to add this discussion?

Ciao,
Dscho

P.S.: I am quite sure that the Git mailing list is maybe better suited for 
patches to the Git source code rather than for patches to the InnoSetup 
code of Git for Windows.

^ permalink raw reply

* Re: [PATCH 8/9] gitweb: Convert output to using indirect file handle
From: Jakub Narebski @ 2010-01-16  0:43 UTC (permalink / raw)
  To: John 'Warthog9' Hawley; +Cc: git
In-Reply-To: <1263432185-21334-9-git-send-email-warthog9@eaglescrag.net>

"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:

> This converts the output handling of gitweb to using an indirect
> file handle.  This is in preparation to add the caching layer.  This
> is a slight modification to the way I was originally doing it by
> passing the output around.  This should be a nop and this shouldn't
> change the behavior of gitweb.  This does leave error reporting
> functions (die_error specifically) continuing to output directly
> as I want to garauntee those will report their errors regardless of
> what may be going on with respect to the rest of the output.

Signoff?

Compare with my version of this patch:
  http://repo.or.cz/w/git/jnareb-git.git/commitdiff/0dd15cb3f18e2a26fc834fd3b071e6d3ecc00557
in the gitweb/cache-kernel branch:
  http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/gitweb/cache-kernel


My commit message looks like the following:

....
gitweb: Print to explicit filehandle (preparing for caching)

This means replacing

  print <something>;
by
  print {$out} <something>;

and

  binmode STDOUT, <layer>;
by
  binmode $out, <layer>;

where $out is global variable set to \*STDOUT at the beginning of
gitweb, but after reading gitweb config.  This way it would be simple
to e.g. tie output filehandle or use PerlIO layers to simultaneously
write to standard output and to some specified file (like "tee"
utility does), or redirect output to a scalar, or a file.

die_error (re)sets $out to \*STDOUT; we would (probably) want to treat
errors in a special way, and do not cache them.

The only other differences are reindent of continued lines (if needed),
and sometimes word-wrapping lines which this change made too long.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
....

> ---
>  gitweb/gitweb.perl |  880 ++++++++++++++++++++++++++--------------------------
>  1 files changed, 448 insertions(+), 432 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index c4a177d..8bb323c 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -450,6 +450,13 @@ our %feature = (
>  		'default' => [0]},
>  );
>  
> +# Basic file handler for all of gitweb, there are two of them.  The first
> +# is the basic text/html file handler which is used for everything other
> +# then the binary files, that uses a separate file handler though
> +# these are both set to STDOUT for the time being.
> +our $output_handler = *STDOUT;
> +our $output_handler_bin = *STDOUT;
> +

First it is not file handleR, but filehandle.

Second, there is no need for separate filehandle for binary files, if
you do it correctly (i.e. call binmode on filehandle, and not on
STDOUT).  When caching is enabled, and 'print {$output_handle} <sth>'
prints to in-memory file (or even directly to cache file) it would do
conversion, so when reading from cache file we can dump it raw, in
binary mode.

Third, wouldn't it be better to use shorter variable name, e.g. $out
or $oh, instead of $output_handle?  We would be able to align print(f)
statements without making lines much longer.

Fourth, there is slight difference between
  our $out = *STDOUT;
and
  out $out = \*STDOUT;
In the former we have global variable, in latter we have indirect
filehandle.  CGI::Cache uses the latter form, IIRC.

> @@ -3313,7 +3320,7 @@ EOF
>  		if ($use_pathinfo) {
>  			$action .= "/".esc_url($project);
>  		}
> -		print $cgi->startform(-method => "get", -action => $action) .
> +		print {$output_handler} $cgi->startform(-method => "get", -action => $action) .
>  		      "<div class=\"search\">\n" .
>  		      (!$use_pathinfo &&
>  		      $cgi->input({-name=>"p", -value=>$project, -type=>"hidden"}) . "\n") .

Here for example after change gitweb source stops being nicely aligned.
OTOH it makes for bigger patch.  In my version I did realign.

You can always check for true differences with "diff -w".


-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply

* Re: [RFC] Git Wiki Move
From: Sverre Rabbelier @ 2010-01-16  0:44 UTC (permalink / raw)
  To: J.H., Pieter de Bie; +Cc: Matthieu Moy, Petr Baudis, git
In-Reply-To: <4B51082B.5030508@eaglescrag.net>

Heya,

On Sat, Jan 16, 2010 at 01:28, J.H. <warthog19@eaglescrag.net> wrote:
>> <!-- GitLink[git-name] Because Linus is an egotistical git -->
>
> Probably, should be fixed now, found it on GitBot too.

I guess you removed them rather than turning them in to comments
(probably due to MediaWiki's lake of comments)? Works for me, we can
figure out something else for gitbot. A regular text file that it can
slurp or something. We being Pieter perhaps (as he is the original author)?

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* Re: [PATCH v2] Add push --set-upstream
From: Tay Ray Chuan @ 2010-01-16  0:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nanako Shiraishi, Ilari Liusvaara, git
In-Reply-To: <7vk4virjzh.fsf@alter.siamese.dyndns.org>

Hi,

On Sat, Jan 16, 2010 at 8:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> Quoting Junio C Hamano <gitster@pobox.com>
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>>         # Ok let's do it for real.
>>>>         git push    --track there this
>>>
>>> Ugh; s/--track/--set-upstream/, of course.
>>
>> How can I use this to say I want to use 'pull --rebase'?
>
> I dunno; "git push --set-upstream=rebase", perhaps?

how about --setup-merge and --setup-rebase?

After all, there's already the config called branch.autosetupmerge and
branch.autosetuprebase.

-- 
Cheers,
Ray Chuan

^ permalink raw reply

* Re: [PATCH v2] Add push --set-upstream
From: Sverre Rabbelier @ 2010-01-16  0:55 UTC (permalink / raw)
  To: Tay Ray Chuan; +Cc: Junio C Hamano, Nanako Shiraishi, Ilari Liusvaara, git
In-Reply-To: <be6fef0d1001151653o7ba2cf7et8875eaf4333fc15a@mail.gmail.com>

Heya,

On Sat, Jan 16, 2010 at 01:53, Tay Ray Chuan <rctay89@gmail.com> wrote:
> how about --setup-merge and --setup-rebase?

I like it, it also suggests this should be called '--setup-upstream', no?

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* Re: [PATCH v2] Add push --set-upstream
From: Tay Ray Chuan @ 2010-01-16  0:58 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: Junio C Hamano, Nanako Shiraishi, Ilari Liusvaara, git
In-Reply-To: <fabb9a1e1001151655r515374f3ybe2a7d4fb20ea532@mail.gmail.com>

Hi,

On Sat, Jan 16, 2010 at 8:55 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Sat, Jan 16, 2010 at 01:53, Tay Ray Chuan <rctay89@gmail.com> wrote:
>> how about --setup-merge and --setup-rebase?
>
> I like it, it also suggests this should be called '--setup-upstream', no?

if I'm not wrong, --set-upstream (which you want renamed to
--setup-upstream, right?) means the same thing as what I want to call
--setup-merge.

-- 
Cheers,
Ray Chuan

^ permalink raw reply

* Re: [PATCH 8/9] gitweb: Convert output to using indirect file handle
From: Junio C Hamano @ 2010-01-16  0:58 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: John 'Warthog9' Hawley, git
In-Reply-To: <m3ljfydgmt.fsf@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> writes:

> This means replacing
>
>   print <something>;
> by
>   print {$out} <something>;

Just out of curiosity, how is this different from

    print $out <something>;

^ permalink raw reply

* Re: [PATCH v2] Add push --set-upstream
From: Tay Ray Chuan @ 2010-01-16  1:00 UTC (permalink / raw)
  To: Ilari Liusvaara
  Cc: Junio C Hamano, Nanako Shiraishi, Johannes Schindelin,
	Rudolf Polzer, Miles Bader, Martin Langhoff, git
In-Reply-To: <1263595630-18962-1-git-send-email-ilari.liusvaara@elisanet.fi>

Hi,

I'm adding people from the "git push --track" thread here, since this
feature is related to what they want.

(sorry for any line-wrap mangling in the patch.)

-- 
Cheers,
Ray Chuan

On Sat, Jan 16, 2010 at 6:47 AM, Ilari Liusvaara
<ilari.liusvaara@elisanet.fi> wrote:
> Frequent complaint is lack of easy way to set up upstream (tracking)
> references for git pull to work as part of push command. So add switch
> --set-upstream (-u) to do just that.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> ---
> Changes from v1:
> - Handle 'git push -u <remote> HEAD' correctly.
> - Add testsuite (thanks Peff), with some additional tests to test delete.
> - Modify documentation for push -u (thanks Matthieu Moy).
>
>  Documentation/git-push.txt |    9 +++++-
>  builtin-push.c             |    1 +
>  t/t5523-push-upstream.sh   |   64 ++++++++++++++++++++++++++++++++++++++++++++
>  transport.c                |   49 +++++++++++++++++++++++++++++++++
>  transport.h                |    1 +
>  5 files changed, 123 insertions(+), 1 deletions(-)
>  create mode 100755 t/t5523-push-upstream.sh
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index e3eb1e8..2a5394b 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
> -          [--repo=<repository>] [-f | --force] [-v | --verbose]
> +          [--repo=<repository>] [-f | --force] [-v | --verbose] [-u | --set-upstream]
>           [<repository> <refspec>...]
>
>  DESCRIPTION
> @@ -122,6 +122,13 @@ nor in any Push line of the corresponding remotes file---see below).
>        the name "origin" is used. For this latter case, this option
>        can be used to override the name "origin". In other words,
>        the difference between these two commands
> +
> +-u::
> +--set-upstream::
> +       For every branch that is up to date or successfully pushed, add
> +       upstream (tracking) reference, used by argument-less
> +       linkgit:git-pull[1] and other commands. For more information,
> +       see 'branch.<name>.merge' in linkgit:git-config[1].
>  +
>  --------------------------
>  git push public         #1
> diff --git a/builtin-push.c b/builtin-push.c
> index 28a26e7..75ddaf4 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -218,6 +218,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>                OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
>                OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
>                OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
> +               OPT_BIT('u', "set-upstream", &flags, "Set upstream for git pull", TRANSPORT_PUSH_SET_UPSTREAM),
>                OPT_END()
>        };
>
> diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
> new file mode 100755
> index 0000000..e098d37
> --- /dev/null
> +++ b/t/t5523-push-upstream.sh
> @@ -0,0 +1,64 @@
> +#!/bin/sh
> +
> +test_description='push with --set-upstream'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup bare parent' '
> +       git init --bare parent &&
> +       git remote add upstream parent
> +'
> +
> +test_expect_success 'setup local commit' '
> +       echo content >file &&
> +       git add file &&
> +       git commit -m one
> +'
> +
> +check_config() {
> +       (echo $2; echo $3) >expect.$1
> +       (git config branch.$1.remote
> +        git config branch.$1.merge) >actual.$1
> +       test_cmp expect.$1 actual.$1
> +}
> +
> +test_expect_success 'push -u master:master' '
> +       git push -u upstream master:master &&
> +       check_config master upstream refs/heads/master
> +'
> +
> +test_expect_success 'push -u master:other' '
> +       git push -u upstream master:other &&
> +       check_config master upstream refs/heads/other
> +'
> +
> +test_expect_success 'push -u master2:master2' '
> +       git branch master2 &&
> +       git push -u upstream master2:master2 &&
> +       check_config master2 upstream refs/heads/master2
> +'
> +
> +test_expect_success 'push -u master2:other2' '
> +       git push -u upstream master2:other2 &&
> +       check_config master2 upstream refs/heads/other2
> +'
> +
> +test_expect_success 'push -u :master2' '
> +       git push -u upstream :master2 &&
> +       check_config master2 upstream refs/heads/other2
> +'
> +
> +test_expect_success 'push -u --all' '
> +       git branch all1 &&
> +       git branch all2 &&
> +       git push -u --all &&
> +       check_config all1 upstream refs/heads/all1 &&
> +       check_config all2 upstream refs/heads/all2
> +'
> +
> +test_expect_success 'push -u HEAD' '
> +       git checkout -b headbranch &&
> +       git push -u upstream HEAD &&
> +       check_config headbranch upstream refs/heads/headbranch
> +'
> +
> +test_done
> diff --git a/transport.c b/transport.c
> index b5332c0..e5b462b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -8,6 +8,7 @@
>  #include "bundle.h"
>  #include "dir.h"
>  #include "refs.h"
> +#include "branch.h"
>
>  /* rsync support */
>
> @@ -135,6 +136,47 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)
>        }
>  }
>
> +static void set_upstreams(struct transport *trans, struct ref *refs)
> +{
> +       struct ref *i;
> +       for (i = refs; i; i = i->next) {
> +               const char *localname;
> +               const char *tmp;
> +               const char *remotename;
> +               unsigned char sha[20];
> +               int flag = 0;
> +               /*
> +                * Check suitability for tracking. Must be successful /
> +                * alreay up-to-date ref create/modify (not delete).
> +                */
> +               if (i->status != REF_STATUS_OK &&
> +                       i->status != REF_STATUS_UPTODATE)
> +                       continue;
> +               if (!i->peer_ref)
> +                       continue;
> +               if (!i->new_sha1 || is_null_sha1(i->new_sha1))
> +                       continue;
> +
> +               /* Chase symbolic refs (mainly for HEAD). */
> +               localname = i->peer_ref->name;
> +               remotename = i->name;
> +               tmp = resolve_ref(localname, sha, 1, &flag);
> +               if (tmp && flag & REF_ISSYMREF &&
> +                       !prefixcmp(tmp, "refs/heads/"))
> +                       localname = tmp;
> +
> +               /* Both source and destination must be local branches. */
> +               if (!localname || prefixcmp(localname, "refs/heads/"))
> +                       continue;
> +               if (!remotename || prefixcmp(remotename, "refs/heads/"))
> +                       continue;
> +
> +               install_branch_config(BRANCH_CONFIG_VERBOSE,
> +                       localname + 11, trans->remote->name,
> +                       remotename);
> +       }
> +}
> +
>  static const char *rsync_url(const char *url)
>  {
>        return prefixcmp(url, "rsync://") ? skip_prefix(url, "rsync:") : url;
> @@ -974,6 +1016,10 @@ int transport_push(struct transport *transport,
>        verify_remote_names(refspec_nr, refspec);
>
>        if (transport->push) {
> +               /* Maybe FIXME. But no important transport uses this case. */
> +               if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
> +                       die("This transport does not support using --set-upstream");
> +
>                return transport->push(transport, refspec_nr, refspec, flags);
>        } else if (transport->push_refs) {
>                struct ref *remote_refs =
> @@ -1002,6 +1048,9 @@ int transport_push(struct transport *transport,
>                                        verbose | porcelain, porcelain,
>                                        nonfastforward);
>
> +               if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
> +                       set_upstreams(transport, remote_refs);
> +
>                if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
>                        struct ref *ref;
>                        for (ref = remote_refs; ref; ref = ref->next)
> diff --git a/transport.h b/transport.h
> index 97ba251..c4314dd 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -91,6 +91,7 @@ struct transport {
>  #define TRANSPORT_PUSH_VERBOSE 16
>  #define TRANSPORT_PUSH_PORCELAIN 32
>  #define TRANSPORT_PUSH_QUIET 64
> +#define TRANSPORT_PUSH_SET_UPSTREAM 128
>
>  /* Returns a transport suitable for the url */
>  struct transport *transport_get(struct remote *, const char *);
> --
> 1.6.6.102.gd6f8f.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply

* Re: [PATCH v2] Add push --set-upstream
From: Sverre Rabbelier @ 2010-01-16  1:02 UTC (permalink / raw)
  To: Tay Ray Chuan; +Cc: Junio C Hamano, Nanako Shiraishi, Ilari Liusvaara, git
In-Reply-To: <be6fef0d1001151658g78af211duc33c9b3ec71bdb57@mail.gmail.com>

Heya,

On Sat, Jan 16, 2010 at 01:58, Tay Ray Chuan <rctay89@gmail.com> wrote:
> if I'm not wrong, --set-upstream (which you want renamed to
> --setup-upstream, right?) means the same thing as what I want to call
> --setup-merge.

Ah, correct; that should teach me not to send emails when it's almost 2am :).

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
From: Junio C Hamano @ 2010-01-16  1:05 UTC (permalink / raw)
  To: Bill Lear; +Cc: David Aguilar, git
In-Reply-To: <19280.51182.981853.561841@blake.zopyra.com>

Bill Lear <rael@zopyra.com> writes:

> On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:
>>David Aguilar <davvid@gmail.com> writes:
>>
>>> -# Copyright (c) 2009 David Aguilar
>>> +# Copyright (c) 2009-2010 David Aguilar
>>
>>Just a very minor issue.  I'd prefer to see:
>>
>>	Copyright (c) 2008, 2009, 2010
>>
>>over
>>
>>	Copyright (c) 2008-2010
>
> Why?

I learned this from <http://www.gnu.org/licenses/gpl-howto.html>.  The
advice doesn't say _why_, but my understanding of the rationale behind it
is that the international convention that governs this copyright
notice specifically mentions "the year of publication", not "range of
years" (UCC Geneva text, Sept. 06, 1952, Article III 1.).

Berne convention does not require such a copyright notice, and many
countries are signatories of both treaties, so the whole copyright notice
may be a moot point in many countries, but it matters in some.  As long as
the file (and the GNU advice cited above) is being cautious by having the
notice, it would be better to be equally cautious and spell the years of
publication out.

But I am not a lawyer.

^ permalink raw reply

* Re: [PATCH v2] Test t5560: Fix test when run with dash
From: Junio C Hamano @ 2010-01-16  1:05 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Tarmigan Casebolt, Michael Haggerty, git, Shawn O. Pearce
In-Reply-To: <201001152017.00121.j6t@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

>> Yesterday, I saw rebase--interactive has a few codepaths where "output"
>> shell function was used with the single-shot export; perhaps they need to
>> also be fixed.
>
> I knew these spots, and they were discussed when that code was introduced. 
> Before I sent out the mail you were responding to, I tried various ways to 
> show the failure in rebase--interactive, but it didn't fail...

It may be the case that the single-shot-ness of these GIT_AUTHOR_NAME
exports do not matter at all in that program, even though the original
versions may have been written carefully not to leak the value suitable
for the current commit to later rounds.

I think the recent updates from Michael actually depends on the
distinction not to matter.  For example, do_with_author() in 7756ecf
(rebase -i: Extract function do_with_author, 2010-01-14) invokes "$@"
that could be a shell function.

^ permalink raw reply

* Re: [PATCH] grep --no-index: allow use of "git grep" outside a git repository
From: Junio C Hamano @ 2010-01-16  1:05 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Nanako Shiraishi
In-Reply-To: <20100115210854.GA21540@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Fri, Jan 15, 2010 at 12:52:40PM -0800, Junio C Hamano wrote:
>
>> Just like some people wanted diff features that are not found in
>> other people's diff implementations outside of a git repository
>> and added --no-index mode to the command, this adds --no-index mode
>> to the "git grep" command.
>
> Out of curiosity, what are the interesting features in git grep versus
> other greps?

Three examples:

    git grep -e Junio --and -e Dscho --and -e Peff

is different from

    grep "Junio.*Dscho.*Peff"

in that the latter wouldn't find a line that has these names in different
order.  You can of course give permutations explicitly, like

    grep -e "Junio.*Dscho.*Peff" \
         -e "Dscho.*Junio.*Peff" \
         ...
	 -e "Peff.*Dscho.*Junio"

I don't know how you would do these with "grep":

    git grep -e Junio --and -e Dscho --and --not -e Linus

    git grep --all-match -e Junio -e Dscho

^ permalink raw reply

* Re: [PATCH] Remove some junk characters from COPYING
From: Robin Rosenberg @ 2010-01-16  1:09 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: git
In-Reply-To: <f3271551001150022p342dccd3r5e93b5f5354d208c@mail.gmail.com>

fredagen den 15 januari 2010 09.22.30 skrev  Ramkumar Ramachandra:
> I removed a few ^L characters from COPYING. Kindly find patch
> attached. Again, I'm sorry I couldn't include it inline- I'm behind a
> restrictive firewall, and Gmail mangles up patches.

It is a form-feed (force new page when printing) so it should probably be 
called by that name, if it were to be removed.

-- robin

^ permalink raw reply

* Re: [PATCH v2] Add push --set-upstream
From: Junio C Hamano @ 2010-01-16  1:10 UTC (permalink / raw)
  To: Tay Ray Chuan; +Cc: Junio C Hamano, Nanako Shiraishi, Ilari Liusvaara, git
In-Reply-To: <be6fef0d1001151653o7ba2cf7et8875eaf4333fc15a@mail.gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

> Hi,
>
> On Sat, Jan 16, 2010 at 8:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nanako Shiraishi <nanako3@lavabit.com> writes:
>>
>>> Quoting Junio C Hamano <gitster@pobox.com>
>>>
>>>> Junio C Hamano <gitster@pobox.com> writes:
>>>>
>>>>>         # Ok let's do it for real.
>>>>>         git push    --track there this
>>>>
>>>> Ugh; s/--track/--set-upstream/, of course.
>>>
>>> How can I use this to say I want to use 'pull --rebase'?
>>
>> I dunno; "git push --set-upstream=rebase", perhaps?
>
> how about --setup-merge and --setup-rebase?
>
> After all, there's already the config called branch.autosetupmerge and
> branch.autosetuprebase.

Do you mean Ilari's patch already sets up branch.name.rebase for people
with branch.autosetuprebase true?

If so, it might be better to keep "--set-upstream" as is, and have a way
to countermand that "autosetuprebase" default.

^ permalink raw reply

* Re: [PATCH 8/9] gitweb: Convert output to using indirect file handle
From: Jakub Narebski @ 2010-01-16  1:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: John 'Warthog9' Hawley, git
In-Reply-To: <7vmy0eoogx.fsf@alter.siamese.dyndns.org>

On Sat, 16 Jan 2010, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > This means replacing
> >
> >   print <something>;
> > by
> >   print {$out} <something>;
> 
> Just out of curiosity, how is this different from
> 
>     print $out <something>;

Actually there is no difference.  It doesn't matter one way or other in
situations in gitweb.

I have thought however (but I might be mistaken) that "print {$fh} <sth>"
is idiomatic Perl.

'perldoc -f print' says:
    Note that if you're storing FILEHANDLES in an array or other expression,
    you will have to use a block returning its value instead:

           print { $files[$i] } "stuff\n";
           print { $OK ? STDOUT : STDERR } "stuff\n";

Also, there is no "," between FILEHANDLE and LIST in "print FILEHANDLE LIST"
-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [PATCH] grep --no-index: allow use of "git grep" outside a git repository
From: Jeff King @ 2010-01-16  1:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Nanako Shiraishi
In-Reply-To: <7vwrzin9jt.fsf@alter.siamese.dyndns.org>

On Fri, Jan 15, 2010 at 05:05:42PM -0800, Junio C Hamano wrote:

> > Out of curiosity, what are the interesting features in git grep versus
> > other greps?
> 
> Three examples:
> 
>     git grep -e Junio --and -e Dscho --and -e Peff
> 
> is different from
> 
>     grep "Junio.*Dscho.*Peff"

Right. I would do:

  grep Junio | grep Dscho | grep Peff

No, it's not quite as accurate, as you are grepping the filenames too.

And no, it's not as efficient, but given that the first grep eliminates
most of your input anyway, it's generally not a big deal.

So the short answer to my question seems to be "git grep has logical
operators". I don't find that compelling, but I guess some people do.
Thanks for satisfying my curiosity.

> I don't know how you would do these with "grep":
> 
>     git grep -e Junio --and -e Dscho --and --not -e Linus

I would do "grep Junio | grep Dscho | grep -v Linus".

>     git grep --all-match -e Junio -e Dscho

That one is a little harder (though it is not something I do very often,
and I had to actually read the docs to find what --all-match does):

  grep Junio `grep -l Dscho *`

which of course has problems with exotic filenames.

-Peff

^ permalink raw reply

* Re: [RFC] Git Wiki Move
From: J.H. @ 2010-01-16  1:18 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: Pieter de Bie, Matthieu Moy, Petr Baudis, git
In-Reply-To: <fabb9a1e1001151644n65577c99qe41327b66de28114@mail.gmail.com>

On 01/15/2010 04:44 PM, Sverre Rabbelier wrote:
> Heya,
> 
> On Sat, Jan 16, 2010 at 01:28, J.H. <warthog19@eaglescrag.net> wrote:
>>> <!-- GitLink[git-name] Because Linus is an egotistical git -->
>>
>> Probably, should be fixed now, found it on GitBot too.
> 
> I guess you removed them rather than turning them in to comments
> (probably due to MediaWiki's lake of comments)? Works for me, we can
> figure out something else for gitbot. A regular text file that it can
> slurp or something. We being Pieter perhaps (as he is the original author)?
> 

The templates giving me a PITA trying to get it to actually generate a
comment, easiest thing might be to hide it in the span or something, or
add the comments in manually.

- John 'Warthog9' Hawley

^ permalink raw reply

* Re: [PATCH 8/9] gitweb: Convert output to using indirect file handle
From: Junio C Hamano @ 2010-01-16  1:41 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: John 'Warthog9' Hawley, git
In-Reply-To: <201001160214.58167.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

> I have thought however (but I might be mistaken) that "print {$fh} <sth>"
> is idiomatic Perl.
>
> 'perldoc -f print' says:
>     Note that if you're storing FILEHANDLES in an array or other expression,
>     you will have to use a block returning its value instead:

Note that "in an array or other expression".  I've always thought the
intention of this phrase was "you _could_ help the parser by doing this,
if you have expression more complex than a simple scalar variable
reference".

IOW, I know that {} _can_ be used there, but I haven't seen people write
{$a_single_variable}, especially without a space around the "expression"
(technically, a single variable is an expression), when

	print $fh <stuff>

suffices, and I was curious why you chose to use the syntax when it wasn't
necessary.  Besides, {$fh} looks so eh... (hesitates to mention a dirty
word ^W^W^Wthe name of a different language, but bleeps it out)...

^ permalink raw reply

* [PATCH] Documentation: explain how to write aliases that use other aliases
From: Eric James Michael Ritz @ 2010-01-16  1:42 UTC (permalink / raw)
  To: git, gitster

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Users who try to define aliases in terms of other aliases will receive
an error about an unrecognized command.  This is because Git only
applies alias expansion once.  However, users can write aliases with
the exclamation point prefix to achieve this effect.  Teach users how
to do this by building off the example of using aliases to call shell
commands.

Signed-off-by: Eric James Michael Ritz <Eric@cybersprocket.com>
- ---
I ran into this issue try to define some aliases, and then after an
hour of hacking on alias.c a light-bulb went off in my head, and I
realized I could just use the '!' prefix to write aliases that call
other aliases.  Much simplier than the alias code I was changing, lol.
So I thought it would be useful to mention this in the manual, for
users like myself who like to build aliases from other aliases.

 Documentation/config.txt |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8acb613..0874a0d 100644
- --- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -524,7 +524,12 @@ If the alias expansion is prefixed with an exclamation point,
 it will be treated as a shell command.  For example, defining
 "alias.new = !gitk --all --not ORIG_HEAD", the invocation
 "git new" is equivalent to running the shell command
- -"gitk --all --not ORIG_HEAD".  Note that shell commands will be
+"gitk --all --not ORIG_HEAD".  Git will not expand aliases which
+call other aliases,  but you can use the exclamation point prefix to
+achieve this effect.  For example, defining "alias.today =
+!git new --since=yesterday" will cause "git today"
+to expand to "gitk --all --not ORIG_HEAD --since=yesterday".
+Note that shell commands will be
 executed from the top-level directory of a repository, which may
 not necessarily be the current directory.

- --
1.6.6.159.g8802c



- -- 
Eric Ritz
Cyber Sprocket Labs
(843) 225-3830
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJLURmKAAoJEEHUZXw5hMWsCZoIALh+tvIJHNVVQtPr1xOEtuYg
fGh4GUdDOwzsWekbxoUpzFwaE0zteawIlG7QYrLKkuFtR4E3bziM86HpIZ4nIy8b
wnZzFVSEGfhAIkhxICsSWFDZp1R0tUN5+i5NOhIrMC0nnQ0aM36Ruzyt1GbntkH1
uBsO9NYpoP+a4HQRGtafXNg2jBC1PoWy3UWTpETsIUHlV3CTrAr17uVbSDt0QSK+
lfhD717w88mvfewJaNwqnrXskADmE+bYbQBr2dvUAutbcRKx0+HREz3ju+/oTtfN
nGaSOX/kLUfTIR4+aGz6u2bkfaGt8WDBibGnC1d0l/c9jxvlodOBZxWcFscxZ4A=
=Divy
-----END PGP SIGNATURE-----

^ permalink raw reply

* Re: [PATCH 9/9] gitweb: File based caching layer (from git.kernel.org)
From: Jakub Narebski @ 2010-01-16  2:48 UTC (permalink / raw)
  To: John 'Warthog9' Hawley, John 'Warthog9' Hawley; +Cc: git
In-Reply-To: <1263432185-21334-10-git-send-email-warthog9@eaglescrag.net>

"John 'Warthog9' Hawley" <warthog9@eaglescrag.net> writes:

> This is a very large patch

This is true, and that is why I am woeking on splitting this patch
into series of smaller patches, each adding single feature present in
this megapatch (this code drop)... and cleaning up (and improving) it
while at it.  This hopefully would make it easier to review.

>                             that implements the file based
> caching layer that is used on such large sites as kernel.org and
> soon git.fedoraproject.org.  This provides a simple, and straight
> forward caching mechanism that scales dramatically better than
> Gitweb by itself.

Do you have any benchmarks comparing gitweb performace with and
without caching enabled?

> 
> The caching layer basically buffers the output that Gitweb would
> normally return, and saves that output to a cache file on the local
> disk.  When the file is requested it attempts to gain a shared lock
> on the cache file and cat it out to the client.  Should an exclusive
> lock be on a file (it's being updated) the code has a choice to either
> update in the background and go ahead and show the stale page while
> update is being performed, or stall the client(s) until the page
> is generated.

The above paragraph is not very clear to me.


Correct me if I am wrong, but as I understand it the cache
architecture is as following:

* This patch implements output caching, which means that the whole
  gitweb response, including HTTP headers, is stored in cache.  (This
  means that in absence of extra mechanism content-type negotiation
  should be disabled when caching is turned on).

* Caching engine used implements simple file based caching layer,
  where cached data is stored verbatim in cache file (no serialization
  / hibernating / marshalling of data - better performance, and
  possibility of X-Sendfile support).  Cache expiration is global
  value, i.e. is not stored along cache entry in file.  Cache entries
  expire based on mtime of file.

* When there exist cache entry for given request, and it is not
  expired, gitweb output is served directtly from cache file.

* When there exist cache entry for given request, but it is expired,
  one process acquires exclusive (writer) lock on file; the rest of
  clients get served stale data.

* When there does not exist cache entry for given request, one process
  acquires exclusive (writer) lock on cache file; the rest of clients
  wait for cache to be filled.

> 
> There are two forms of stalling involved here, background building
> and non-background building, both of which are discussed in the
> configuration page.

I'd like to have at least design decisions put into commit message,
and perhaps also have caching mechanism described in separate section
in gitweb/README.

> 
> There are still a few known "issues" with respect to this:
> - Code needs to be added to be "browser" aware so
>   that clients like wget that are trying to get a
>   binary blob don't obtain a "Generating..." page

This issue should be clearly addressed: when do we serve
"Generating..." page, and when we do not.  The issue is not only wget
trying to download binary blob or patchset, or snapshot, but also
binary blob which is image referenced from a blob which is HTML, and
there is issue of web feeds (accessed by feed readers).

> - There is an intermittent flushing issue that has yet
>   to be tracked down

Could you tell us more where does this shows (what are the
symptompts)?

BTW if it was split into small separate commits, you could be able to
find bug by bisecting history.  Also troubles with finding this bug
might mean that code is not very clean.

> 
> Caching is disabled by default with the $cache_enable variable,
> setting this to 1 will enable file based caching.  It is expected
> that this will be extended to include additional types of caching
> (like memcached) in the future and should not be exclusively
> considered a binary value.

Not a good idea, IMHO.  In my rewrite of this patch there is _boolean_
$caching_enabled variable which controls if (output) caching is
enabled or not, and $cache variable holding instance of cache engine,
which might be used to select different caching that simple file-based
caching.

Signoff?

> ---
>  gitweb/cache.pm    |  283 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gitweb/gitweb.css  |    6 +
>  gitweb/gitweb.perl |   58 ++++++++++-
>  3 files changed, 344 insertions(+), 3 deletions(-)
>  create mode 100644 gitweb/cache.pm

Very large patch... but no updates to gitweb/README, no updates to
t/gitweb-lib.sh (I guess that gitweb tests are no longer working).
 
> diff --git a/gitweb/cache.pm b/gitweb/cache.pm
> new file mode 100644
> index 0000000..d08bcec
> --- /dev/null
> +++ b/gitweb/cache.pm
> @@ -0,0 +1,283 @@
> +# gitweb - simple web interface to track changes in git repositories
> +#
> +# (C) 2006, John 'Warthog9' Hawley <warthog19@eaglescrag.net>
> +#
> +# This program is licensed under the GPLv2
> +
> +#
> +# Gitweb caching engine
> +#
> +
> +use File::Path qw(make_path remove_tree);

Using make_path (you do not use remove_tree, so there is no need for
importing it) instead of older mkdir interface requires File::Path
version 2.0 (which meant that I had to upgrade File::Path).  This at
least should be mentioned in the comment, perhaps also in
gitweb/INSTALL.

> +use Digest::MD5 qw(md5 md5_hex md5_base64);

You use only md5_hex; no need to import other functions.

> +use Fcntl ':flock';
> +
> +sub cache_fetch {
> +	my ($action) = @_;
> +	my $cacheTime = 0;
> +
> +	# Deal with cache being disabled
> +	if( $cache_enable == 0 ){

Style:

  +	if ($cache_enable == 0) {

or better

  +	if ($cache_enabled) {

> +		$output_handler = *STDOUT;
> +		$output_handler_bin = *STDOUT;

There should be no need for that, as $output_handle is set to *STDOUT
(or \*STDOUT) anyway.

> +		$actions{$action}->();
> +		return;

Anyway I think that the whole block should be _outside_ cache_fetch,
which should be invoked only if caching is enabled.  For example in
gitweb.perl:

  if ($caching_enabled) {
  	do $cache_pm;
  	die $@ if $@;
  
  	# ...
  
	cache_fetch($cache, $action);
  } else {
  	$actions{$action}->();
  }

> +	}elsif( $cache_enable == 1 ){

Style.

> +		#obviously we are using file based caching

See my comment about using $cache_enable as enum selecting cache type
(blergh).  BTW what's with 'obviously'?

> +
> +		if(! -d $cachedir){

Style.

> +			print "*** Warning ***: Caching enabled but cache directory does not exsist.  ($cachedir)\n";

Why this warning?  Is it really necessary?

> +			mkdir ("cache", 0665) || die "Cannot create cache dir - you will need to manually create";
> +			print "Cache directory created successfully\n";
> +		}
> +
> +		our $full_url = "$my_url?". $ENV{'QUERY_STRING'};

Wouldn't work if you client uses path_info URL, e.g.

  http://repo.or.cz/w/git/jnareb-git.git/shortlog/refs/heads/gitweb/cache-kernel

That's why I use href(-replay=>1, -full_url=>1, -path_info=>0) for
cache key for request (you could use freeze(\%input_params) instead,
where freeze is from Storable module).

> +		our $urlhash = md5_hex($full_url);
> +		our $fullhashdir = "$cachedir/". substr( $urlhash, 0, 2) ."/";

Is depth 2 enough for cache?

> +
> +		my $numdirs = make_path( $fullhashdir, { mode => 0777, error => \my $mkdirerr, } );
> +		if( @$mkdirerr ){
> +			my $mkdirerrmsg = "";
> +			for my $diag (@$mkdirerr) {
> +				my ($file, $message) = %$diag;
> +				if($file eq '' ){
> +					$mkdirerrmsg .= "general error: $message\n";
> +				}else{
> +					$mkdirerrmsg .= "problem unlinking $file: $message\n";
> +				}
> +			}
> +			die_error(500, "Could not create cache directory | $mkdirerrmsg");
> +		}
> +		$fullhashpath = "$fullhashdir/". substr( $urlhash, 2 );
> +		$fullhashbinpath = "$fullhashpath.bin";
> +	} # done dealing with cache enabled / disabled

Note also if dealing with caching enabled / disabled was outside
cache_fetch you would have less nested code.

> +
> +	if(! -e "$fullhashpath" ){
> +		if(! defined(my $childPid = fork()) ){

Style.

> +			cacheUpdate($action,0);
> +			cacheDisplay($action);

Why camelCase Java/JavaScript-like convention, quite different from
the C-like naming convention used elsewhere in gitweb?

> +		} elsif ( $childPid == 0 ){
> +			#run the updater
> +			cacheUpdate($action,1);

cacheUpdate($action,0) vs cacheUpdate($action,1) is very cryptic
distinctions.  It would be better to use "named parameter" and/or
separate, differently named, [wrapper] functions.

> +		}else{
> +			cacheWaitForUpdate($action);
> +		}

This whole block should probably be in a separate function.

> +	}else{
> +		#if cache is out dated, update
> +		#else displayCache();
> +		open(cacheFile, '<', "$fullhashpath");
> +		stat(cacheFile);
> +		close(cacheFile);

You don't need to open file to stat it.

> +		$cacheTime = get_loadavg() * 60;
> +		if( $cacheTime > $maxCacheTime ){
> +			$cacheTime = $maxCacheTime;
> +		}
> +		if( $cacheTime < $minCacheTime ){
> +			$cacheTime = $minCacheTime;
> +		}

This should probably be a separate function (effective cache expiraton
time).  Also adaptiveness of caching is not described in commit
message.

> +		if( (stat(_))[9] < (time - $cacheTime) ){
> +			if( ! defined(my $childPid = fork()) ){
> +				cacheUpdate($action,0);
> +				cacheDisplay($action);
> +			} elsif ( $childPid == 0 ){
> +				#run the updater
> +				#print "Running updater\n";

Remains of debugging.

> +				cacheUpdate($action,1);
> +			}else{
> +				#print "Waiting for update\n";
> +				cacheWaitForUpdate($action);
> +			}

Repeated code (I think).

> +		} else {
> +			cacheDisplay($action);
> +		}
> +
> +
> +	}
> +
> +	#
> +	# If all of the caching failes - lets go ahead and press on without it and fall back to 'default'
> +	# non-caching behavior.  This is the softest of the failure conditions.
> +	#
> +	#$actions{$action}->();

Why is this commented out?

> +}
> +
> +sub cacheUpdate {
> +	my ($action,$areForked) = @_;
> +	my $lockingStatus;
> +	my $fileData = "";
> +
> +	if($backgroundCache){
> +		open(cacheFileBG, '>:utf8', "$fullhashpath.bg");
> +		my $lockStatBG = flock(cacheFileBG,LOCK_EX|LOCK_NB);
> +
> +		$lockStatus = $lockStatBG;
> +	}else{
> +		open(cacheFile, '>:utf8', "$fullhashpath");
> +		my $lockStat = flock(cacheFile,LOCK_EX|LOCK_NB);
> +
> +		$lockStatus = $lockStat;
> +	}

Almost identical code.  Use of global handles instead of indirect
filehandles.

> +	#print "lock status: $lockStat\n";
> +
> +
> +	if (! $lockStatus ){
> +		if ( $areForked ){
> +			exit(0);
> +		}else{
> +			return;
> +		}
> +	}

This conditional needs explanation (comment), I think.

> +
> +	if(
> +		$action eq "snapshot"
> +		||
> +		$action eq "blob_plain"

This condition should be put in a separate function/

> +	){
> +		open cacheFileBin, '>', $fullhashbinpath or die_error(500, "Could not open bin dump file");
> +		$output_handler_bin = *cacheFileBin;
> +	}
> +
> +	$output_handler = *cacheFile;
> +
> +	if($backgroundCache){
> +		open(cacheFile, '>:utf8', "$fullhashpath");

Why "$fullhashpath" and not simply $fullhashpath?

> +		$lockStat = flock(cacheFile,LOCK_EX);
> +
> +		if (! $lockStat ){
> +			if ( $areForked ){
> +				exit(0);
> +			}else{
> +				return;
> +			}
> +		}

Repeated code.

> +	}
> +
> +	$actions{$action}->();
> +
> +	if(
> +		$action eq "snapshot"
> +		||
> +		$action eq "blob_plain"
> +	){
> +		close(cacheFileBin);
> +	}
> +
> +	flock(cacheFile,LOCK_UN);
> +	close(cacheFile);
> +
> +	if($backgroundCache){
> +		flock(cacheFileBG,LOCK_UN);
> +		close(cacheFileBG);
> +	}
> +
> +	if ( $areForked ){
> +		exit(0);
> +	} else {
> +		return;
> +	}
> +}
> +
> +
> +sub cacheWaitForUpdate {
> +	my ($action) = @_;
> +	my $x = 0;
> +	my $max = 10;

What is $x, what is $max?

> +	my $lockStat = 0;
> +
> +	if( $backgroundCache ){
> +		if( -e "$fullhashpath" ){
> +			open(cacheFile, '<:utf8', "$fullhashpath");

Why opening with :uft8, and not with :raw?  I don't think we need to
do the eventual conversion once again...

> +			$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
> +			stat(cacheFile);
> +			close(cacheFile);
> +
> +			if( $lockStat && ( (stat(_))[9] > (time - $maxCacheLife) ) ){
> +				cacheDisplay($action);
> +				return;
> +			}

Why do we deal with cache expiration in two places?  If it is not
a bug, it should be explained in a comment.

> +		}
> +	}
> +
> +	if(
> +		$action eq "atom"
> +		||
> +		$action eq "rss"
> +		||
> +		$action eq "opml"
> +	){
> +		do {
> +			sleep 2 if $x > 0;
> +			open(cacheFile, '<:utf8', "$fullhashpath");
> +			$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
> +			close(cacheFile);
> +			$x++;
> +			$combinedLockStat = $lockStat;
> +		} while ((! $combinedLockStat) && ($x < $max));

Why busy wait instead of _blocking_ lock, i.e. waiting on lock for it
to be free?  It doesn't look like we _do_ anything in the loop.

Ah, I see that we wait at most 2*$max seconds (where interval of 2
seconds is hardcoded).  Is it really necessary?

> +
> +		if( $x != $max ){
> +			cacheDisplay($action);
> +		}
> +		return;
> +	}
> +
> +	$| = 1;
> +
> +	print $::cgi->header(-type=>'text/html', -charset => 'utf-8',
> +	                   -status=> 200, -expires => 'never');
> +
> +	print <<EOF;
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www/w3.porg/TR/html4/strict.dtd">
> +<!-- git web w/caching interface version $version, (C) 2006-2010, John 'Warthog9' Hawley <warthog9\@kernel.org> -->
> +<!-- git core binaries version $git_version -->
> +<head>
> +<meta http-equiv="content-type" content="$content_type; charset=utf-8"/>
> +<meta name="generator" content="gitweb/$version git/$git_version"/>
> +<meta name="robots" content="index, nofollow"/>
> +<meta http-equiv="refresh" content="0"/>
> +<title>$title</title>
> +</head>
> +<body>
> +EOF
> +
> +	print "Generating..";
> +	do {
> +		print ".";
> +		sleep 2 if $x > 0;
> +		open(cacheFile, '<:utf8', "$fullhashpath");
> +		$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
> +		close(cacheFile);
> +		$x++;
> +		$combinedLockStat = $lockStat;
> +	} while ((! $combinedLockStat) && ($x < $max));

This trick of having http-equiv 'refresh' meta with the delay of 0
seconds, but not closing the output and therefore not triggering
redirect should be described in comments, and perhaps also in the
commit message.

> +	print <<EOF;
> +</body>
> +</html>
> +EOF
> +	return;
> +}
> +
> +sub cacheDisplay {
> +	my ($action) = @_;
> +	open(cacheFile, '<:utf8', "$fullhashpath");
> +	$lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
> +	if (! $lockStat ){
> +		close(cacheFile);
> +		cacheWaitForUpdate($action);
> +	}
> +
> +	while( <cacheFile> ){
> +		print $_;
> +	}

Why not slurp it (local $/ = undef), but write line after line?

> +	if(
> +		$action eq "snapshot"
> +		||
> +		$action eq "blob_plain"
> +	){
> +		open(cacheFileBin, '<', "$fullhashbinpath");
> +		binmode STDOUT, ':raw';
> +		print <cacheFileBin>;

Why not slurp it (local $/ = undef), but write line after line,
implicitly?

> +		binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
> +		close(cacheFileBin);
> +	}
> +	close(cacheFile);
> +}

> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 8bb323c..ec95bb9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -230,6 +230,50 @@ our $git_versions_must_match = 1;
>  # Leave it undefined (or set to 'undef') to turn off load checking.
>  our $maxload = 300;
>  
> +# This enables/disables the caching layer in gitweb.  This currently only supports the
> +# 'dumb' file based caching layer, primarily used on git.kernel.org.  this is reasonably
> +# effective but it has the downside of requiring a huge amount of disk space if there
> +# are a number of repositories involved.  It is not uncommon for git.kernel.org to have
> +# on the order of 80G - 120G accumulate over the course of a few months.  It is recommended
> +# that the cache directory be periodically completely deleted, and this is safe to perform.
> +# Suggested mechanism
> +# mv $cacheidr $cachedir.flush;mkdir $cachedir;rm -rf $cachedir.flush
> +# Value is binary. 0 = disabled (default), 1 = enabled.
> +#
> +# Values of caching:
> +# 	1 = 'dumb' file based caching used on git.kernel.org
> +our $cache_enable = 0;
> +
> +# Used to set the minimum cache timeout for the dynamic caching algorithm.  Basically
> +# if we calculate the cache to be under this number of seconds we set the cache timeout
> +# to this minimum.
> +# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
> +our $minCacheTime = 20;
> +
> +# Used to set the maximum cache timeout for the dynamic caching algorithm.  Basically
> +# if we calculate the cache to exceed this number of seconds we set the cache timeout
> +# to this maximum.
> +# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
> +our $maxCacheTime = 1200;
> +
> +# If you need to change the location of the caching directory, override this
> +# otherwise this will probably do fine for you
> +our $cachedir = 'cache';

Why not '/tmp/gitweb-cache', or '/var/cache/gitweb'?  Perhaps use
TMPDIR / File::Spec->tmpdir() if it is undefined?

Note that this path is relative to the place where we run gitweb from,
which is important for gitweb tests.

> +
> +# If this is set (to 1) cache will do it's best to always display something instead
> +# of making someone wait for the cache to update.  This will launch the cacheUpdate
> +# into the background and it will lock a <file>.bg file and will only lock the
> +# actual cache file when it needs to write into it.  In theory this will make
> +# gitweb seem more responsive at the price of possibly stale data.
> +our $backgroundCache = 1;

Does it mean that if there exist cache entry for given request, but it
is expired, also the client that created write lock gets stale data
instead of 'Generating...' info, and updates/regenerates cache using
background process?

This comment is not entirely clear for me.

> +
> +# Used to set the maximum cache file life.  If a cache files last modify time exceeds
> +# this value, it will assume that the data is just too old, and HAS to be regenerated
> +# instead of trying to display the existing cache data.
> +# Value is in seconds.  1 = 1 seconds, 60 = 1 minute, 600 = 10 minutes, 3600 = 1 hour
> +# 18000 = 5 hours
> +our $maxCacheLife = 18000;

This should also be mentioned in commit message (modifying what I
wrote).

> +
>  # You define site-wide feature defaults here; override them with
>  # $GITWEB_CONFIG as necessary.
>  our %feature = (
> @@ -593,6 +637,11 @@ if (defined $maxload && get_loadavg() > $maxload) {
>  	die_error(503, "The load average on the server is too high");
>  }
>  
> +#
> +# Includes
> +#
> +do 'cache.pm';

Should be

  +do "$cache_pm";

if you don't use require, where $cache_pm can be overriden in gitweb
config, otherwise gitweb caching tests wouldn't work: they invoke
gitweb from test directory.

> +
>  # version of the core git binary
>  our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
>  $number_of_git_cmds++;
> @@ -994,7 +1043,7 @@ if ($action !~ m/^(?:opml|project_list|project_index)$/ &&
>      !$project) {
>  	die_error(400, "Project needed");
>  }
> -$actions{$action}->();
> +cache_fetch($action);
>  exit;
>  

As I wrote, I think cache_fetch should be invoked only when caching is
enabled.

>  ## ======================================================================
> @@ -3200,7 +3249,9 @@ sub git_header_html {
>  	# support xhtml+xml but choking when it gets what it asked for.
>  	if (defined $cgi->http('HTTP_ACCEPT') &&
>  	    $cgi->http('HTTP_ACCEPT') =~ m/(,|;|\s|^)application\/xhtml\+xml(,|;|\s|$)/ &&
> -	    $cgi->Accept('application/xhtml+xml') != 0) {
> +	    $cgi->Accept('application/xhtml+xml') != 0
> +	    &&
> +	    $cache_enable == 0) {
>  		$content_type = 'application/xhtml+xml';
>  	} else {
>  		$content_type = 'text/html';

O.K.

> @@ -3344,6 +3395,7 @@ sub git_footer_html {
>  	my $feed_class = 'rss_logo';
>  
>  	print {$output_handler} "<div class=\"page_footer\">\n";
> +	print {$output_handler} "<div class=\"cachetime\">Cache Last Updated: ". gmtime( time ) ." GMT</div>\n";

Shouldn't this be conditional on $cache_enabled?

>  	if (defined $project) {
>  		my $descr = git_get_project_description($project);
>  		if (defined $descr) {

BTW. you need, I think, protect timing info and do not show it if
caching is enabled.  It doesn't make much sense to show how much time
it took to generate page... when said page could have been retrieved
from cache.

But it might make sense; I am not sure.

> @@ -3424,7 +3476,7 @@ sub die_error {
>  	my $extra = shift;
>  
>  	# The output handlers for die_error need to be reset to STDOUT
> -	# so that half the message isn't being output to random and 
> +	# so that half the message isn't being output to random and
>  	# half to STDOUT as expected.  This is mainly for the benefit
>  	# of using git_header_html() and git_footer_html() since those
>  	# internaly use the indirect print handler.

It looks like spurious change.

> -- 
> 1.6.5.2
> 

-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply

* Re: [PATCH v2 3/3] commit: show interesting ident information in summary
From: Adam Megacz @ 2010-01-16  2:56 UTC (permalink / raw)
  To: git
In-Reply-To: <5722BD3D-E7C9-47F7-B547-09B14D87DA39@wincent.com>


Wincent Colaiuta <win@wincent.com> writes:
> Fair enough, but I'm sighing here at the thought of people jumping in  
> and using git commands without even having looked at _any_ of the  
> zillions of "your first 10 minutes with Git" tutorials out there,  

I don't think that's what this thread is really about, although helping
those people might be a harmless side-effect.

>> and people who have git configured on another machine but are using
>> _this_ machine for the first time).

For the record, this is what I have been burned by several times now.

I'm now stuck with a bunch of repositories I can no longer fix because
data blindly yanked out of libnss and never shown to me was then SHA-1
signed for all eternity.  It's incredibly frustrating.

Thank you for taking steps to save others from this frustration in the
future.  I appreciate it.

  - a

^ permalink raw reply

* [PATCH] difftool: Update copyright notices to list each year separately
From: David Aguilar @ 2010-01-16  3:10 UTC (permalink / raw)
  To: gitster; +Cc: git

Reference: http://article.gmane.org/gmane.comp.version-control.git/137182
Signed-off-by: David Aguilar <davvid@gmail.com>
---

This applies on top of the difftool series I sent today
which itself applies on top of da/difftool in next.

 git-difftool--helper.sh |    2 +-
 t/t7800-difftool.sh     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 69f6bce..e43b5d6 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -3,7 +3,7 @@
 # This script is typically launched by using the 'git difftool'
 # convenience command.
 #
-# Copyright (c) 2009-2010 David Aguilar
+# Copyright (c) 2009, 2010 David Aguilar
 
 TOOL_MODE=diff
 . git-mergetool--lib
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a183f1d..fad5472 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 #
-# Copyright (c) 2009 David Aguilar
+# Copyright (c) 2009, 2010 David Aguilar
 #
 
 test_description='git-difftool
-- 
1.6.2.5

^ permalink raw reply related

* Re: [PATCH] Remove some junk characters from COPYING
From: Ramkumar Ramachandra @ 2010-01-16  3:52 UTC (permalink / raw)
  To: David Aguilar; +Cc: git
In-Reply-To: <20100115222811.GB18878@gmail.com>

Hi,

> http://git.or.cz/gitwiki/GitTips#Usingmsmtptosendyourpatches

Thank you for the link! Yes, I know about git send-imap, but I my
firewall blocked traffic on all ports except 80 and 443 until
yesterday. I jumped some hoops and got a few more ports opened up for
myself yesterday, so no more attachments in Gmail :)

> I'm sure there are other ways, but that's the one that works for
> me.  I also use mutt to read gmail; also highly recommended :)

In that case, check out Sup. It's *much* better than mutt :)

^ 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