Git development
 help / color / mirror / Atom feed
* Re: Candidates for sequencer?
From: Jeff King @ 2012-11-12 22:16 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0n0tTZ3EgzeesOr2B1LjeEUjTnWQh5dDfW28jA1ia0-gA@mail.gmail.com>

On Tue, Nov 13, 2012 at 03:16:51AM +0530, Ramkumar Ramachandra wrote:

> I'd like to get more commands to call into sequencer.c, so that we can
> (finally) implement a sensible `git continue` and `git abort`.
> Currently, am and rebase come to mind, but they are shell scripts and
> I don't see how we can make them call into sequencer.c.  Is there any
> way to make sequencer.c more useful?  Should we implement a `git
> continue` and `git reset` anyway, lifting code from 83c750ac to check
> the tree state?

I'd love it if ther was a mechanism for interruptible commands to notify
the rest of git that they are in progress. This would not only make
cross-language commands like "rebase" and "am" work easily, but it would
mean that we can automatically support third-party commands that we
don't even know about.

I am already doing something like this for some personal scripts which
do sequences of merges and rebases, and my "git continue" looks like:

  if test -f "$GIT_DIR/continue"; then
    eval "$(cat "$GIT_DIR/continue")"
  elif test -d "$GIT_DIR/rebase-merge"; then
    git rebase --continue
  elif test -d "$GIT_DIR/rebase-apply"; then
    if test -f "$GIT_DIR/rebase-apply/applying"; then
      git am --continue
    else
      git rebase --continue
    fi
  fi
  [... etc ...]

It works pretty well in practice, but I'd have a few suggestions on top:

  1. Even if "am" and "rebase" learn to use $GIT_DIR/continue, we should
     probably continue to support the fallback state-examination in case
     people use mixed versions of git. And clearly the code should be
     shared with what's in wt-status.c.

  2. The "continue" file should probably be called "in-progress", and
     should have more structured data, like the name of the operation.

  3. The list of operations in progress needs to be a stack. If I am
     rebasing and start a cherry pick, what would "git continue" do? It
     probably makes sense to continue the most recently started
     operation first.

  4. Rebase already detects an in-progress rebase. It should probably
     also detect an "am" or "cherry-pick" in progress. But it's not
     commutative; you should be able to start a "cherry-pick" in the
     middle of a "rebase". This is really independent of the storage
     format (we could do it right now if the state logic from wt-status
     is made available in a machine-readable format).

     We _might_ want to put information in the in-progress file that
     helps programs reach that decision. I'm not sure of a sane way of
     doing it, though. You could say "don't rebase while I'm in
     progress", but that depends on the writer knowing the universe of
     all programs that might be run. It might be possible to come up
     with some taxonomy of operations like "a history rewriting
     operation is in progress" which would cause other history rewriting
     operations to choose not to start. But coming up with the right set
     of attributes might be impossible.

-Peff

^ permalink raw reply

* Re: [PATCH 2/2] send-email: add series-cc-cmd option
From: Ramkumar Ramachandra @ 2012-11-12 21:51 UTC (permalink / raw)
  To: Felipe Contreras
  Cc: git, Junio C Hamano, Joe Perches, Jonathan Nieder, Pascal Obry
In-Reply-To: <1352653463-1923-3-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras wrote:
> cc-cmd is only per-file, and many times receipients get lost without
> seing the full patch series.

s/seing/seeing

> [...]

Looks good otherwise.

Ram

^ permalink raw reply

* Candidates for sequencer?
From: Ramkumar Ramachandra @ 2012-11-12 21:46 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

Hi,

I'd like to get more commands to call into sequencer.c, so that we can
(finally) implement a sensible `git continue` and `git abort`.
Currently, am and rebase come to mind, but they are shell scripts and
I don't see how we can make them call into sequencer.c.  Is there any
way to make sequencer.c more useful?  Should we implement a `git
continue` and `git reset` anyway, lifting code from 83c750ac to check
the tree state?

Ram

^ permalink raw reply

* Re: [PATCH] Update cygwin.c for new mingw-64 win32 api headers
From: Jeff King @ 2012-11-12 21:43 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: git
In-Reply-To: <CAK2bgnXLnKmnVwzE5U_1qZueUdj-QaT23f-iFmhJyRto3tWnhA@mail.gmail.com>

On Mon, Nov 12, 2012 at 04:39:39PM -0500, Mark Levedahl wrote:

> >> diff --git a/Makefile b/Makefile
> >> index f69979e..1cc5d96 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -1082,6 +1082,7 @@ ifeq ($(uname_O),Cygwin)
> >>               NO_SYMLINK_HEAD = YesPlease
> >>               NO_IPV6 = YesPlease
> >>               OLD_ICONV = UnfortunatelyYes
> >> +             V15_MINGW_HEADERS = YesPlease
> >>       endif
> >
> > The "if" part of the conditional that did not make it into the context
> > above is an expr match for "1.6.*" From the name, I would think that we
> > would want to use these headers on cygwin 1.5.* , too. Is v1.5 too old
> > to care about now?
> 
> The opening if expression is:
>     ifeq ($(shell expr "$(uname_R)" : '1\.[1-6]\.'),4)
> which I believe matches any version below 1.7. 1.5 is what is released
> opensource, 1.6 is (or was) available as a supported product from
> redhat. So, I think the stanza is ok as is.

Oh, right, I'm an idiot. I read the patch, thought "gee, I wonder what
that conditional just outside the context is", looked it up, then
promptly misremembered exactly what it said when writing the rest of my
email.

Yes, what you have is absolutely right. Sorry for the noise.

-Peff

^ permalink raw reply

* Re: RFD: fast-import is picky with author names (and maybe it should - but how much so?)
From: Jeff King @ 2012-11-12 21:41 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: A Large Angry SCM, Michael J Gruber, Git Mailing List
In-Reply-To: <CAMP44s1m8sAD9D0F-6b=+dm_AvLb_4_f7h=3A_VMYMDUEcTW7g@mail.gmail.com>

On Sun, Nov 11, 2012 at 07:48:14PM +0100, Felipe Contreras wrote:

> >   3. Exporters should not use it if they have any broken-down
> >      representation at all. Even knowing that the first half is a human
> >      name and the second half is something else would give it a better
> >      shot at cleaning than fast-import would get.
> 
> I'm not sure what you mean by this. If they have name and email, then
> sure, it's easy.

But not as easy as just printing it. What if you have this:

  name="Peff <angle brackets> King"
  email="<peff@peff.net>"

Concatenating them does not produce a valid git author name. Sending the
concatenation through fast-import's cleanup function would lose
information (namely, the location of the boundary between name and
email).

Similarly, one might have other structured data (e.g., CVS username)
where the structure is a useful hint, but some conversion to name+email
is still necessary.

-Peff

^ permalink raw reply

* Re: [PATCH] Update cygwin.c for new mingw-64 win32 api headers
From: Mark Levedahl @ 2012-11-12 21:39 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20121112205832.GI4623@sigill.intra.peff.net>

On Mon, Nov 12, 2012 at 3:58 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Nov 11, 2012 at 07:17:20PM -0500, Mark Levedahl wrote:
>
>> The cygwin project recently switched to a new implementation of the
>> windows api, now using header files from the mingw-64 project. These
>> new header files are incompatible with the way cygwin.c included the
>> old headers: cygwin.c can be compiled using the new or the older (mingw)
>> headers, but different files must be included in different order for each
>> to work. The new headers are in use only for the current release series
>> (based upon the v1.7.x dll version). The previous release series using
>> the v1.5 dll is kept available but unmaintained for use on older versions
>> of Windows. So, patch cygwin.c to use the new include ordering only if
>> the dll version is 1.7 or higher.
>
> I have very little knowledge of cygwin compatibility, so thanks for a
> nice explanation.  I'll queue it in 'pu' for now, and hopefully we can
> get some test reports from other cygwin folks (on new and old cygwin).
>
>> diff --git a/Makefile b/Makefile
>> index f69979e..1cc5d96 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1082,6 +1082,7 @@ ifeq ($(uname_O),Cygwin)
>>               NO_SYMLINK_HEAD = YesPlease
>>               NO_IPV6 = YesPlease
>>               OLD_ICONV = UnfortunatelyYes
>> +             V15_MINGW_HEADERS = YesPlease
>>       endif
>
> The "if" part of the conditional that did not make it into the context
> above is an expr match for "1.6.*" From the name, I would think that we
> would want to use these headers on cygwin 1.5.* , too. Is v1.5 too old
> to care about now?
>
> -Peff

The opening if expression is:
    ifeq ($(shell expr "$(uname_R)" : '1\.[1-6]\.'),4)
which I believe matches any version below 1.7. 1.5 is what is released
opensource, 1.6 is (or was) available as a supported product from
redhat. So, I think the stanza is ok as is.

Mark

^ permalink raw reply

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Jeff King @ 2012-11-12 21:34 UTC (permalink / raw)
  To: Jakub Narębski; +Cc: Junio C Hamano, Drew Northup, glpk xypron, git
In-Reply-To: <CANQwDwdRTeaVS5cMic5gv9SP1A8Z1vruOsZBFfMDQDTZHBAtvQ@mail.gmail.com>

On Mon, Nov 12, 2012 at 10:13:27PM +0100, Jakub Narębski wrote:

> > Yeah, that looks correct, given the way how the other variables
> > emitted with the same "print" like $descr and $owner are formed.
> 
> It looks like good solution to me too.
> 
> Nb. the problems with feed are mainly because it is generated
> by hand even more than HTML (which uses CGI.pm).

Yeah, I noticed that. Here it is in patch form with a test. It would be
nice if people interested in gitweb would add more entries to the XSS
test below (I put in the one that fails, along with an obvious variation
that is actually OK).

I didn't look carefully through the rest of gitweb for more XSS
instances. From a glance, it looks like we mostly use the safe CGI
methods, but probably it could use a full audit (which again, I would be
happy if people who care more about gitweb would do).

-- >8 --
Subject: [PATCH] gitweb: escape html in rss title

The title of an RSS feed is generated from many components,
including the filename provided as a query parameter, but we
failed to quote it.  Besides showing the wrong output, this
is a vector for XSS attacks.

Signed-off-by: Jeff King <peff@peff.net>
---
 gitweb/gitweb.perl                        |  1 +
 t/t9502-gitweb-standalone-parse-output.sh | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10ed9e5..a51a8ba 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -8055,6 +8055,7 @@ sub git_feed {
 		$feed_type = 'history';
 	}
 	$title .= " $feed_type";
+	$title = esc_html($title);
 	my $descr = git_get_project_description($project);
 	if (defined $descr) {
 		$descr = esc_html($descr);
diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
index 731e64c..3a8e7d3 100755
--- a/t/t9502-gitweb-standalone-parse-output.sh
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -185,5 +185,20 @@ test_expect_success 'forks: project_index lists all projects (incl. forks)' '
 	test_cmp expected actual
 '
 
+xss() {
+	echo >&2 "Checking $1..." &&
+	gitweb_run "$1" &&
+	if grep "$TAG" gitweb.body; then
+		echo >&2 "xss: $TAG should have been quoted in output"
+		return 1
+	fi
+	return 0
+}
+
+test_expect_success 'xss checks' '
+	TAG="<magic-xss-tag>" &&
+	xss "a=rss&p=$TAG" &&
+	xss "a=rss&p=foo.git&f=$TAG"
+'
 
 test_done
-- 
1.8.0.207.gdf2154c

^ permalink raw reply related

* Re: [RFC PATCH 1/1] status: Allow for short-form via config option
From: Jeff King @ 2012-11-12 21:17 UTC (permalink / raw)
  To: Thomas Adam; +Cc: git
In-Reply-To: <1352674383-23654-2-git-send-email-thomas@xteddy.org>

On Sun, Nov 11, 2012 at 10:53:03PM +0000, Thomas Adam wrote:

> It is currently not possible to use the short-form output of git status
> without declaring an alias to do so.
> 
> This isn't always desirable therfore, define a git config option which can
> be set to display the short-form:  status.shortwithbranch

We usually try to avoid booleans for selection among options, even if
there is currently only one useful option. That way, it makes it easier
to extend later when another option presents itself.  So having
"status.format" that you could set to "short", "long", or "porcelain"
would make more sense (although "short" is likely to be the only useful
one currently).

And then we can have a separate boolean like "status.branch" to show the
branch by default when showing short format. That would would naturally
extend to more booleans as other options are added (Phil Hord's recent
"show tree state" patches come to mind).

We also need to consider the impact of options on scripts. I think
status.format should be OK, since any script calling "git status" would
have to explicitly pass "--porcelain" already, which would override this
option. But we would want to make sure that "status.branch" would not
affect the porcelain.

> ---
>  builtin/commit.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

It would need documentation, and tests would be good both to make sure
the feature works, but also to demonstrate that it does not break
--porcelain.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index a17a5df..552a9f1 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1142,6 +1142,18 @@ static int git_status_config(const char *k, const char *v, void *cb)
>  			return error(_("Invalid untracked files mode '%s'"), v);
>  		return 0;
>  	}
> +
> +	if (!strcmp(k, "status.shortwithbranch")) {
> +		if (git_config_bool(k, v)) {
> +			if (!v)
> +				return config_error_nonbool(k);
> +			else if(!strcmp(v, "true")) {
> +				status_format = STATUS_FORMAT_SHORT;
> +				s->show_branch = 1;
> +			}
> +			return 0;
> +		}
> +	}

I'm not sure what is going on with the extra nonbool and "true" check.
Shouldn't it just be:

  if (git_config_bool(k, v)) {
          status_format = STATUS_FORMAT_SHORT;
          s->show_branch = 1;
  }
  else {
          /* what do we do when it is false? */
  }
  return 0;

If we follow my suggestions above, then it would be more like:

  if (!strcmp(k, "status.format")) {
          if (!v)
                  return config_error_nonbool(k);
          return parse_status_format(v, &status_format);
  }
  if (!strcmp(k, "status.branch")) {
          s->show_branch = git_config_bool(k, v);
          return 0;
  }

but that would still have to resolve the setting of show_branch when
--porcelain is in use. I think you would need to store the
config-derived value separately, and then only fill it into
s->show_branch if no value was given on the command-line _and_ we are
not showing the porcelain format.

-Peff

^ permalink raw reply

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Jakub Narębski @ 2012-11-12 21:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Drew Northup, glpk xypron, git
In-Reply-To: <7vmwymh83r.fsf@alter.siamese.dyndns.org>

On Mon, Nov 12, 2012 at 9:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> On Mon, Nov 12, 2012 at 03:24:13PM -0500, Jeff King wrote:
>>
>>> I think the right answer is going to be a well-placed call to esc_html.
>>
>> I'm guessing the right answer is this:
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 10ed9e5..a51a8ba 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -8055,6 +8055,7 @@ sub git_feed {
>>               $feed_type = 'history';
>>       }
>>       $title .= " $feed_type";
>> +     $title = esc_html($title);
>>       my $descr = git_get_project_description($project);
>>       if (defined $descr) {
>>               $descr = esc_html($descr);
>>
>> but I did not test it (and I am not that familiar with gitweb, so it is
>> a slight guess from spending 5 minutes grepping and reading).
>
> Yeah, that looks correct, given the way how the other variables
> emitted with the same "print" like $descr and $owner are formed.

It looks like good solution to me too.

Nb. the problems with feed are mainly because it is generated
by hand even more than HTML (which uses CGI.pm).

-- 
Jakub Narębski

-- 
Jakub Narebski

^ permalink raw reply

* Re: [PATCH] Update cygwin.c for new mingw-64 win32 api headers
From: Jeff King @ 2012-11-12 20:58 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: git
In-Reply-To: <1352679440-4098-1-git-send-email-mlevedahl@gmail.com>

On Sun, Nov 11, 2012 at 07:17:20PM -0500, Mark Levedahl wrote:

> The cygwin project recently switched to a new implementation of the
> windows api, now using header files from the mingw-64 project. These
> new header files are incompatible with the way cygwin.c included the
> old headers: cygwin.c can be compiled using the new or the older (mingw)
> headers, but different files must be included in different order for each
> to work. The new headers are in use only for the current release series
> (based upon the v1.7.x dll version). The previous release series using
> the v1.5 dll is kept available but unmaintained for use on older versions
> of Windows. So, patch cygwin.c to use the new include ordering only if
> the dll version is 1.7 or higher.

I have very little knowledge of cygwin compatibility, so thanks for a
nice explanation.  I'll queue it in 'pu' for now, and hopefully we can
get some test reports from other cygwin folks (on new and old cygwin).

> diff --git a/Makefile b/Makefile
> index f69979e..1cc5d96 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1082,6 +1082,7 @@ ifeq ($(uname_O),Cygwin)
>  		NO_SYMLINK_HEAD = YesPlease
>  		NO_IPV6 = YesPlease
>  		OLD_ICONV = UnfortunatelyYes
> +		V15_MINGW_HEADERS = YesPlease
>  	endif

The "if" part of the conditional that did not make it into the context
above is an expr match for "1.6.*" From the name, I would think that we
would want to use these headers on cygwin 1.5.* , too. Is v1.5 too old
to care about now?

-Peff

^ permalink raw reply

* Re: RFD: fast-import is picky with author names (and maybe it should - but how much so?)
From: Felipe Contreras @ 2012-11-12 20:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: gitzilla, Michael J Gruber, Git Mailing List, Jeff King
In-Reply-To: <7vwqxqiul3.fsf@alter.siamese.dyndns.org>

On Mon, Nov 12, 2012 at 6:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> A Large Angry SCM <gitzilla@gmail.com> writes:
>
>> On 11/11/2012 07:41 AM, Felipe Contreras wrote:
>>> On Sat, Nov 10, 2012 at 8:25 PM, A Large Angry SCM<gitzilla@gmail.com>  wrote:
>>>> On 11/10/2012 01:43 PM, Felipe Contreras wrote:
>>>
>>>>> So, the options are:
>>>>>
>>>>> a) Leave the name conversion to the export tools, and when they miss
>>>>> some weird corner case, like 'Author<email', let the user face the
>>>>> consequences, perhaps after an hour of the process.
>>>>>
>>>>> We know there are sources of data that don't have git-formatted author
>>>>> names, so we know every tool out there must do this checking.
>>>>>
>>>>> In addition to that, let the export tool decide what to do when one of
>>>>> these bad names appear, which in many cases probably means do nothing,
>>>>> so the user would not even see that such a bad name was there, which
>>>>> might not be what they want.
>>>>>
>>>>> b) Do the name conversion in fast-import itself, perhaps optionally,
>>>>> so if a tool missed some weird corner case, the user does not have to
>>>>> face the consequences.
>>>>>
>>>>> The tool writers don't have to worry about this, so we would not have
>>>>> tools out there doing a half-assed job of this.
>>>>>
>>>>> And what happens when such bad names end up being consistent: warning,
>>>>> a scaffold mapping of bad names, etc.
>>>>>
>>>>>
>>>>> One is bad for the users, and the tools writers, only disadvantages,
>>>>> the other is good for the users and the tools writers, only
>>>>> advantages.
>>>>>
>>>>
>>>> c) Do the name conversion, and whatever other cleanup and manipulations
>>>> you're interesting in, in a filter between the exporter and git-fast-import.
>>>
>>> Such a filter would probably be quite complicated, and would decrease
>>> performance.
>>>
>>
>> Really?
>>
>> The fast import stream protocol is pretty simple. All the filter
>> really needs to do is pass through everything that isn't a 'commit'
>> command. And for the 'commit' command, it only needs to do something
>> with the 'author' and 'committer' lines; passing through everything
>> else.
>>
>> I agree that an additional filter _may_ decrease performance somewhat
>> if you are already CPU constrained. But I suspect that the effect
>> would be negligible compared to the all of the SHA-1 calculations.
>
> More importantly, which do users prefer: quickly produce an
> incorrect result, or spend some more time to get it right?

Why not both?

If I do 'git clone hg::http://selenic.com/hg' I expect it to work, no
matter what. Then, if I care about getting it right, like for example
if the project is moving to git, then check
.git/hg/origin/bad-authors, and fill them with the right ones.

Of course, the current remote helper framework doesn't have the option
to map authors, but it could be added. That would be better than
letting every remote helper tool to have a custom way of mapping
authors, and also custom configuration for them.

> Because the exporting tool has a lot more intimate knowledge about
> how the names are represented in the history of the original SCM,
> canonicalization of the names, if done at that point, would likely
> to give us more useful results, than a canonicalization done at the
> beginning of the importer, which lacks SCM specific details.  So in
> that sense, (a) is more preferrable than (b).

But it doesn't have more intimate knowledge. It has exactly the same
information as fast-import; nothing.

What intimate knowledge is a tool expected to get from this?

% hg commit -u 'Foo Bar<foo.bar@example.com> <none@none>' -m one
% hg --debug log
changeset:   0:5ef37a2c773f02d0e01f1ecdcc59149832d294e8
tag:         tip
phase:       draft
parent:      -1:0000000000000000000000000000000000000000
parent:      -1:0000000000000000000000000000000000000000
manifest:    0:c6d4cd25b9fc2f83b0dd51f4acbea9486fce54d7
user:        Foo Bar<foo.bar@example.com> <none@none>
date:        Sun Nov 11 18:33:00 2012 +0100
files+:      file
extra:       branch=default
description:
one

Some tools might, but if they did, then bad authors wouldn't be a problem.

> On the other hand, we would want consistency across the converted
> results no matter what SCM the history was originally in.  E.g. a
> name without email that came from CVS or SVN would consistently want
> to become "name <noname@noname>" or "name <name>" or whatever, and
> letting exporting tools responsible for the canonicalization will
> lead them to create their own garbage.  In that sense, (b) can be
> better than (a).

Or 'Unknown <unknown>' or '<none@none>' or '<>', or any of the forms
conversion tools have been doing for ages.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply

* Re: [PATCHv3] replace: parse revision argument for -d
From: Jeff King @ 2012-11-12 20:42 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <a35a8f44b908bded0b475b02e7917011fb3bf90b.1352728712.git.git@drmicha.warpmail.net>

On Mon, Nov 12, 2012 at 03:18:02PM +0100, Michael J Gruber wrote:

> 'git replace' parses the revision arguments when it creates replacements
> (so that a sha1 can be abbreviated, e.g.) but not when deleting
> replacements.
> 
> Make it parse the argument to 'replace -d' in the same way.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> 
> Notes:
>     v3 safeguards the hex buffer against reuse

Thanks, I don't see any other functional problems.

> diff --git a/builtin/replace.c b/builtin/replace.c
> index e3aaf70..33e6ec3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
>  
>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
>  {
> -	const char **p;
> +	const char **p, *q;

I find this readable today, but I wonder if in six months we will wonder
what in the world "q" means. Maybe "short_refname" or something would be
appropriate?

-Peff

^ permalink raw reply

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Junio C Hamano @ 2012-11-12 20:36 UTC (permalink / raw)
  To: Jeff King; +Cc: Drew Northup, glpk xypron, git, jnareb
In-Reply-To: <20121112202701.GE4623@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Mon, Nov 12, 2012 at 03:24:13PM -0500, Jeff King wrote:
>
>> I think the right answer is going to be a well-placed call to esc_html.
>
> I'm guessing the right answer is this:
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 10ed9e5..a51a8ba 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -8055,6 +8055,7 @@ sub git_feed {
>  		$feed_type = 'history';
>  	}
>  	$title .= " $feed_type";
> +	$title = esc_html($title);
>  	my $descr = git_get_project_description($project);
>  	if (defined $descr) {
>  		$descr = esc_html($descr);
>
> but I did not test it (and I am not that familiar with gitweb, so it is
> a slight guess from spending 5 minutes grepping and reading).

Yeah, that looks correct, given the way how the other variables
emitted with the same "print" like $descr and $owner are formed.

^ permalink raw reply

* Re: [PATCH 1/2] git-sh-setup: refactor ident-parsing functions
From: Junio C Hamano @ 2012-11-12 20:32 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Sixt, Ilya Basin, git
In-Reply-To: <20121112201235.GA7210@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Mon, Nov 12, 2012 at 12:08:37PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Changing the interface for get_author_ident_from_commit would be a pain,
>> > but if we just wanted to help filter-branch, we could do something like
>> > this:
>> 
>> Yes, that is the direction I was alluding to.
>> 
>> Callers of get_author_ident_from_commit can also do the same and
>> avoid rebuilding the same $pick_author_script over and over again,
>> or get_author_ident_from_commit can do so for its callers.
>
> I don't think get_author_ident_from_commit can do so on demand due to
> the subshell issue I mentioned.

OK, that is what I obviously missed.

> I wonder if we should simply generate these at build time and store them
> in the script.

Yeah, my knee-jerk reaction was that it was wasteful to run script
at runtime to build a constant string, but the engineering cost may
not be worth it.

^ permalink raw reply

* Re: [PATCH 1/4] remote-hg: add missing config for basic tests
From: Jeff King @ 2012-11-12 20:32 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, Ramkumar Ramachandra
In-Reply-To: <1352742068-15346-2-git-send-email-felipe.contreras@gmail.com>

On Mon, Nov 12, 2012 at 06:41:05PM +0100, Felipe Contreras wrote:

> From: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> 'hg commit' fails otherwise in some versiosn of mercurial because of

s/versiosn/versions/

> +setup () {
> +	(
> +	echo "[ui]"
> +	echo "username = A U Thor <author@example.com>"
> +	) >> "$HOME"/.hgrc
> +}

This makes sense, but I wonder if we should use something different from
the git author ident set up by the test scripts, just to double check
that we do not have any bugs in confusing the two during the import.

Something like "H G Wells <wells@example.com>" would work, and satisfies
my deep-seated desire for bad puns.

-Peff

^ permalink raw reply

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Jeff King @ 2012-11-12 20:27 UTC (permalink / raw)
  To: Drew Northup; +Cc: glpk xypron, git, jnareb, Junio C Hamano
In-Reply-To: <20121112202413.GD4623@sigill.intra.peff.net>

On Mon, Nov 12, 2012 at 03:24:13PM -0500, Jeff King wrote:

> I think the right answer is going to be a well-placed call to esc_html.

I'm guessing the right answer is this:

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 10ed9e5..a51a8ba 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -8055,6 +8055,7 @@ sub git_feed {
 		$feed_type = 'history';
 	}
 	$title .= " $feed_type";
+	$title = esc_html($title);
 	my $descr = git_get_project_description($project);
 	if (defined $descr) {
 		$descr = esc_html($descr);

but I did not test it (and I am not that familiar with gitweb, so it is
a slight guess from spending 5 minutes grepping and reading).

-Peff

^ permalink raw reply related

* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Jeff King @ 2012-11-12 20:24 UTC (permalink / raw)
  To: Drew Northup; +Cc: glpk xypron, git, jnareb, Junio C Hamano
In-Reply-To: <CAM9Z-n=6xsC7yiKJ+NU-CxNPxEXWmJzvXLUocgZgWPQnuK6G4Q@mail.gmail.com>

On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:

> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de> wrote:
> > Gitweb can be used to generate an RSS feed.
> >
> > Arbitrary tags can be inserted into the XML document describing
> > the RSS feed by careful construction of the URL.
> [...]
> Something like this may be useful to defuse the "file" parameter, but
> I presume a more definitive fix is in order...
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 10ed9e5..af93e65 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1447,6 +1447,10 @@ sub validate_pathname {
>         if ($input =~ m!\0!) {
>                 return undef;
>         }
> +       # No XSS <script></script> inclusions
> +       if ($input =~ m!(<script>)(.*)(</script>)!){
> +               return undef;
> +       }
>         return $input;
>  }

This is the wrong fix for a few reasons:

  1. It is on the input-validation side, whereas the real problem is on
     the output-quoting side. Your patch means I could not access a file
     called "<script>foo</script>". What we really want is to have the
     unquoted name internally, but then make sure we quote it when
     outputting as part of an HTML (or XML) file.

  2. Script tags are only part of the problem. They are what make it
     obviously a security vulnerability, but it is equally incorrect for
     us to show the filename "<b>foo</b>" as bold. I would also not be
     surprised if there are other cross-site attacks one can do without
     using <script>.

  3. Your filter is too simplistic. At the very least, it would not
     filter out "<SCRIPT>". I am not up to date on all of the
     sneaking-around-HTML-filters attacks that are available these days,
     but I wonder if one could also get around it using XML entities or
     similar.

I think the right answer is going to be a well-placed call to esc_html.
This already happens automatically when we go through the CGI
element-building functions, but obviously we failed to make the call
when building the output manually.  This is a great reason why template
languages which default to safe expansion should always be used.
Unfortunately, gitweb is living in 1995 in terms of web frameworks.

-Peff

^ permalink raw reply

* Re: [Patch 1/1] Wire html for all files in ./technical and ./howto in Makefile
From: Junio C Hamano @ 2012-11-12 20:13 UTC (permalink / raw)
  To: Jeff King; +Cc: Thomas Ackermann, git
In-Reply-To: <20121112191953.GA4623@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Mon, Nov 12, 2012 at 09:45:31AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Mon, Oct 29, 2012 at 07:33:47PM +0100, Thomas Ackermann wrote:
>> >
>> >> This patch addresses Junios comment in WC:
>> >> "Misapplication of a patch fixed; the ones near the tip needs to
>> >>  update the links to point at the html files, though."
>> >> 
>> >> See older mail in this thread:
>> >> [...]
>> >> That means that for the patch [6/8], which adds content-type to the
>> >> text files, to be complete, it needs to update Makefile to produce
>> >> html files from them.
>> >> [...]
>> >> So IMHO no open issues with this patch.
>> >
>> > OK, that explains the situation. Thanks, I'll merge it to master in the
>> > next iteration.
>> 
>> What happened to this topic?
>
> I mistyped in the above response; it should have been "next" not
> "master".

Ah, ok, I was just wondering why I didn't see it in your 'master'.
Thanks.

> But then I forgot to merge it to next in the last iteration; it is
> ta/doc-cleanup in pu, and is marked in "What's Cooking" to be merged to
> next.
>
> -Peff

^ permalink raw reply

* Re: [PATCH 1/2] git-sh-setup: refactor ident-parsing functions
From: Jeff King @ 2012-11-12 20:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Ilya Basin, git
In-Reply-To: <7vzk2mh9dm.fsf@alter.siamese.dyndns.org>

On Mon, Nov 12, 2012 at 12:08:37PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Changing the interface for get_author_ident_from_commit would be a pain,
> > but if we just wanted to help filter-branch, we could do something like
> > this:
> 
> Yes, that is the direction I was alluding to.
> 
> Callers of get_author_ident_from_commit can also do the same and
> avoid rebuilding the same $pick_author_script over and over again,
> or get_author_ident_from_commit can do so for its callers.

I don't think get_author_ident_from_commit can do so on demand due to
the subshell issue I mentioned. So you'd have to generate the pick
script on inclusion of git-sh-setup, whether the includer wants to call
the function or not.

I wonder if we should simply generate these at build time and store them
in the script.

I'm also not sure that saving one process invocation is worth spending a
lot of thought cycles on. Maybe somebody on Windows could run a
filter-branch with the patch I just sent and measure whether it even
makes a difference.

-Peff

^ permalink raw reply

* Re: [PATCH 1/2] git-sh-setup: refactor ident-parsing functions
From: Junio C Hamano @ 2012-11-12 20:08 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Sixt, Ilya Basin, git
In-Reply-To: <20121112194434.GB4623@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Changing the interface for get_author_ident_from_commit would be a pain,
> but if we just wanted to help filter-branch, we could do something like
> this:

Yes, that is the direction I was alluding to.

Callers of get_author_ident_from_commit can also do the same and
avoid rebuilding the same $pick_author_script over and over again,
or get_author_ident_from_commit can do so for its callers.

> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 5314249..7a693ba 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -74,7 +74,7 @@ finish_ident() {
>  }
>  
>  set_ident () {
> -	parse_ident_from_commit author AUTHOR committer COMMITTER
> +	parse_ident_from_commit_via_script "$ident_script"
>  	finish_ident AUTHOR
>  	finish_ident COMMITTER
>  }
> @@ -93,6 +93,7 @@ if [ "$(is_bare_repository)" = false ]; then
>  	require_clean_work_tree 'rewrite branches'
>  fi
>  
> +ident_script=$(pick_ident_script author AUTHOR committer COMMITTER)
>  tempdir=.git-rewrite
>  filter_env=
>  filter_tree=
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 22f0aed..1e20e17 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -225,10 +225,17 @@ pick_ident_script () {
>  	echo '/^$/q'
>  }
>  
> +# Feed a pick_ident_script return value to sed. Use this instead of
> +# parse_ident_from_commit below if you are going to be parsing commits in a
> +# tight loop and want to save a process.
> +parse_ident_from_commit_via_script() {
> +	LANG=C LC_ALL=C sed -ne "$1"
> +}
> +
>  # Create a pick-script as above and feed it to sed. Stdout is suitable for
>  # feeding to eval.
>  parse_ident_from_commit () {
> -	LANG=C LC_ALL=C sed -ne "$(pick_ident_script "$@")"
> +	parse_ident_from_commit_via_script "$(pick_ident_script "$@")"
>  }
>  
>  # Parse the author from a commit given as an argument. Stdout is suitable for

^ permalink raw reply

* Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash
From: Marc Khouzam @ 2012-11-12 20:07 UTC (permalink / raw)
  To: git
In-Reply-To: <CAFj1UpE6OtJEojaED1_DZJD0kU=nVsFE_w8xa0oJE-6auCU2rw@mail.gmail.com>

Hi,

this patch allows tcsh-users to get the benefits of the awesome
git-completion.bash script.  It could also help other shells do the same.

==

The current tcsh-completion support for Git, as can be found on the
internet, takes the approach of defining the possible completions
explicitly.  This has the obvious draw-back to require constant
updating as the Git code base evolves.

The approach taken by this commit is to to re-use the advanced bash
completion script and use its result for tcsh completion.  This is
achieved by executing (versus sourcing) the bash script and
outputting the completion result for tcsh consumption.

Three solutions were looked at to implement this approach with (A)
being retained:

  A) Modifications:
          git-completion.bash and new git-completion.tcsh

     Modify the existing git-completion.bash script to support
     being sourced using bash (as now), but also executed using bash.
     When being executed, the script will output the result of the
     computed completion to be re-used elsewhere (e.g., in tcsh).
     Pros:
       1- allows the git-completion.bash script to easily be re-used
       2- tcsh support is mostly isolated in git-completion.tcsh
     Cons (for tcsh users only):
       1- requires the user to copy both git-completion.tcsh and
          git-completion.bash to ${HOME}
       2- requires bash script to have a fixed name and location:
          ${HOME}/.git-completion.bash

  B) Modifications:
          git-completion.bash

     Modify the existing git-completion.bash script to support
     being sourced using bash (as now), but also executed using bash,
     and sourced using tcsh.
     Pros:
       1- only requires the user to deal with a single file
       2- maintenance more obvious for tcsh since it is entirely part
          of the same git-completion.bash script.
     Cons:
       1- tcsh support could affect bash support as they share the
          same script
       2- small tcsh section must use syntax suitable for both tcsh
          and bash and must be at the beginning of the script
       3- requires the user to explicitly make the script executable
          when using tcsh (for tcsh users only)
       4- requires script to have a fixed name and location:
          ${HOME}/.git-completion.sh (for tcsh users only)

  C) Modifications:
          New git-completion.tcsh

     Provide a short tcsh script that converts git-completion.bash
     into an executable script suitable to be used by tcsh.
     Pros:
       1- tcsh support is entirely isolated in git-completion.tcsh
       2- new tcsh script can be as complex as needed
     Cons (for tcsh users only):
       1- requires the user to copy both git-completion.tcsh and
          git-completion.bash to ${HOME}
       2- requires bash script to have a fixed name and location:
          ${HOME}/.git-completion.bash
       3- sourcing the new script will generate a third script

Approach (A) was selected to keep the tcsh completion support well
isolated without introducing excessive complexity.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

==

With the changes applied, tcsh users should:

#    1) Copy both this file and the bash completion script to your
${HOME} directory
#       using the names ${HOME}/.git-completion.tcsh and
${HOME}/.git-completion.bash.
#    2) Add the following line to your .tcshrc/.cshrc:
#        source ${HOME}/.git-completion.tcsh

The code can be found on GitHub.
Option (A):
https://github.com/marckhouzam/git/commit/86d3a8e740ae85b4b4462c997a0fd969b1b2d24c

Option (B):
https://github.com/marckhouzam/git/commit/e64606541682eaf66c0a56aceff279ca6e1d06cd

Option (C):
https://github.com/marckhouzam/git/commit/59792455f1e6a98d3ffeb828f4cff1ded0e4ed37

Thanks

Marc

---
 contrib/completion/git-completion.bash |   53 +++++++++++++++++++++++++++++++-
 contrib/completion/git-completion.tcsh |   34 ++++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletions(-)
 create mode 100755 contrib/completion/git-completion.tcsh

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index be800e0..6d4b57a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1,4 +1,6 @@
-#!bash
+#!/bin/bash
+# The above line is important as this script can be executed when used
+# with another shell such as tcsh
 #
 # bash/zsh completion support for core Git.
 #
@@ -2481,3 +2483,52 @@ __git_complete gitk __gitk_main
 if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
 __git_complete git.exe __git_main
 fi
+
+# Method that will output the result of the completion done by
+# the bash completion script, so that it can be re-used in another
+# context than the bash complete command.
+# It accepts 1 to 2 arguments:
+# 1: The command-line to complete
+# 2: The index of the word within argument #1 in which the cursor is
+#    located (optional). If parameter 2 is not provided, it will be
+#    determined as best possible using parameter 1.
+_git_complete_with_output ()
+{
+       # Set COMP_WORDS to the command-line as bash would.
+       COMP_WORDS=($1)
+
+       # Set COMP_CWORD to the cursor location as bash would.
+       if [ -n "$2" ]; then
+               COMP_CWORD=$2
+       else
+               # Assume the cursor is at the end of parameter #1.
+               # We must check for a space as the last character which will
+               # tell us that the previous word is complete and the cursor
+               # is on the next word.
+               if [ "${1: -1}" == " " ]; then
+                       # The last character is a space, so our
location is at the end
+                       # of the command-line array
+                       COMP_CWORD=${#COMP_WORDS[@]}
+               else
+                       # The last character is not a space, so our
location is on the
+                       # last word of the command-line array, so we
must decrement the
+                       # count by 1
+                       COMP_CWORD=$((${#COMP_WORDS[@]}-1))
+               fi
+       fi
+
+       # Call _git() or _gitk() of the bash script, based on the first
+       # element of the command-line
+       _${COMP_WORDS[0]}
+
+       # Print the result that is stored in the bash variable ${COMPREPLY}
+       for i in ${COMPREPLY[@]}; do
+               echo "$i"
+       done
+}
+
+if [ -n "$1" ] ; then
+  # If there is an argument, we know the script is being executed
+  # so go ahead and run the _git_complete_with_output function
+  _git_complete_with_output "$1" "$2"
+fi
diff --git a/contrib/completion/git-completion.tcsh
b/contrib/completion/git-completion.tcsh
new file mode 100755
index 0000000..7b7baea
--- /dev/null
+++ b/contrib/completion/git-completion.tcsh
@@ -0,0 +1,34 @@
+#!tcsh
+#
+# tcsh completion support for core Git.
+#
+# Copyright (C) 2012 Marc Khouzam <marc.khouzam@gmail.com>
+# Distributed under the GNU General Public License, version 2.0.
+#
+# This script makes use of the git-completion.bash script to
+# determine the proper completion for git commands under tcsh.
+#
+# To use this completion script:
+#
+#    1) Copy both this file and the bash completion script to your
${HOME} directory
+#       using the names ${HOME}/.git-completion.tcsh and
${HOME}/.git-completion.bash.
+#    2) Add the following line to your .tcshrc/.cshrc:
+#        source ${HOME}/.git-completion.tcsh
+
+# One can change the below line to use a different location
+set __git_tcsh_completion_script = ${HOME}/.git-completion.bash
+
+# Check that the user put the script in the right place
+if ( ! -e ${__git_tcsh_completion_script} ) then
+       echo "ERROR in git-completion.tcsh script.  Cannot find:
${__git_tcsh_completion_script}.  Git completion will not work."
+       exit
+endif
+
+# Make the script executable if it is not
+if ( ! -x ${__git_tcsh_completion_script} ) then
+       chmod u+x ${__git_tcsh_completion_script}
+endif
+
+complete git  'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
| sort | uniq`/'
+complete gitk 'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
| sort | uniq`/'
+
--
1.7.0.4

^ permalink raw reply related

* Re: [PATCH 2/5] launch_editor: ignore SIGINT while the editor has control
From: Jeff King @ 2012-11-12 19:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Paul Fox, Kalle Olavi Niemitalo, git
In-Reply-To: <7v4nkuk966.fsf@alter.siamese.dyndns.org>

On Mon, Nov 12, 2012 at 09:44:49AM -0800, Junio C Hamano wrote:

> How did this message happen?
> 
>     Subject: [PATCH 2/5] launch_editor: ignore SIGINT while the editor has control
>     To: Kalle Olavi Niemitalo <kon@iki.fi>
>     Cc: Paul Fox <pgf@foxharp.boston.ma.us>, git@vger.kernel.org
>     Date: Sun, 11 Nov 2012 11:55:11 -0500
>     Message-ID: <20121111165510.GB19850@sigill.intra.peff.net>
>     References: <20121111163100.GB13188@sigill.intra.peff.net>
> 
>     The user's editor likely catches SIGINT (ctrl-C).  but if
>     the user spawns a command from the editor and uses ctrl-C to
>     kill that command, the SIGINT will likely also kill git
>     itself (depending on the editor, this can leave the terminal
>     in an unusable state).
> 
>     Signed-off-by: Paul Fox <pgf@foxharp.boston.ma.us>
>     Signed-off-by: Jeff King <peff@peff.net>
>     ---
> 
> Judging from S-o-b, message-id and EHLO, I think this was sent by
> Peff, but came without Sender: or anything.
> 
> Just being curious.

I screwed up when sending out the series and did not properly move
Paul's "From" address from the email header down to the body. It is not
a git or send-email screw-up; I load format-patch output directly into
mutt as a template. Since I do not often send out other people's
patches, I never bothered to write a script to migrate the "from" into
the body automatically.

-Peff

^ permalink raw reply

* Re: [PATCH 1/2] git-sh-setup: refactor ident-parsing functions
From: Jeff King @ 2012-11-12 19:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Ilya Basin, git
In-Reply-To: <7vpq3ik97i.fsf@alter.siamese.dyndns.org>

On Mon, Nov 12, 2012 at 09:44:01AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The only ident-parsing function we currently provide is
> > get_author_ident_from_commit. This is not very
> > flexible for two reasons:
> >
> >   1. It takes a commit as an argument, and can't read from
> >      commit headers saved on disk.
> >
> >   2. It will only parse authors, not committers.
> >
> > This patch provides a more flexible interface which will
> > parse multiple idents from a commit provide on stdin. We can
> > easily use it as a building block for the current function
> > to retain compatibility.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > Since we are counting processes in this series, I should note that this
> > actually adds a subshell invocation for each call, since it went from:
> >
> >   script='...'
> >   sed $script
> >
> > to:
> >
> >   sed "$(make_script)"
> >
> > For filter-branch, which is really the only high-performance caller we
> > have, this is negated by the fact that it will do author and committer
> > at the same time, saving us an extra subshell (in addition to an extra
> > sed invocation).
> 
> Given that pick-ident-script is a const function, a caller that
> repeatedly call is could call it once and use it in a variable, no?

The problem is that it is a helper called from parse_ident_from_commit.
And that function just passes along its arguments, so it does not know
that it is being called repeatedly with the same arguments. So you'd
have to either change the interface or memoize internally.

I don't think memoization is a good option for two reasons:

  1. Storing the arguments to compare to later is complex. You don't
     want to just store "$*" from the last run and see if we got the
     same arguments. You'd have to quote your delimiter (e.g., you would
     not want to confuse ("foo", "bar") with ("foo bar"). Though in this
     instance, we know that our args do not have spaces, so we could get
     away with that.

  2. If you are in a subshell or even a while loop, your memoized
     variable will not be retained.

So unless somebody has some clever scheme for memoizing shell functions
without any process overhead, it is probably not worth it.

Changing the interface for get_author_ident_from_commit would be a pain,
but if we just wanted to help filter-branch, we could do something like
this:

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..7a693ba 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -74,7 +74,7 @@ finish_ident() {
 }
 
 set_ident () {
-	parse_ident_from_commit author AUTHOR committer COMMITTER
+	parse_ident_from_commit_via_script "$ident_script"
 	finish_ident AUTHOR
 	finish_ident COMMITTER
 }
@@ -93,6 +93,7 @@ if [ "$(is_bare_repository)" = false ]; then
 	require_clean_work_tree 'rewrite branches'
 fi
 
+ident_script=$(pick_ident_script author AUTHOR committer COMMITTER)
 tempdir=.git-rewrite
 filter_env=
 filter_tree=
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 22f0aed..1e20e17 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -225,10 +225,17 @@ pick_ident_script () {
 	echo '/^$/q'
 }
 
+# Feed a pick_ident_script return value to sed. Use this instead of
+# parse_ident_from_commit below if you are going to be parsing commits in a
+# tight loop and want to save a process.
+parse_ident_from_commit_via_script() {
+	LANG=C LC_ALL=C sed -ne "$1"
+}
+
 # Create a pick-script as above and feed it to sed. Stdout is suitable for
 # feeding to eval.
 parse_ident_from_commit () {
-	LANG=C LC_ALL=C sed -ne "$(pick_ident_script "$@")"
+	parse_ident_from_commit_via_script "$(pick_ident_script "$@")"
 }
 
 # Parse the author from a commit given as an argument. Stdout is suitable for

^ permalink raw reply related

* Re: [regression] Newer gits cannot clone any remote repos
From: Douglas Mencken @ 2012-11-12 19:35 UTC (permalink / raw)
  To: Kevin; +Cc: git
In-Reply-To: <CAO54GHC9ibTwxqLnk1vGSo9R011HE05JOL1mgpTGzeWX7cCcwQ@mail.gmail.com>

> On Mon, Nov 12, 2012 at 12:12 PM, Kevin <ikke@ikke.info> wrote:
> > Maybe handy to say that you're on a Powerpc platform.

Oh, and yes, I'm on 2 x 2-core ("4-core") machine.

^ permalink raw reply

* Re: [Patch 1/1] Wire html for all files in ./technical and ./howto in Makefile
From: Jeff King @ 2012-11-12 19:19 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Thomas Ackermann, git
In-Reply-To: <7vpq3iiukk.fsf@alter.siamese.dyndns.org>

On Mon, Nov 12, 2012 at 09:45:31AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Oct 29, 2012 at 07:33:47PM +0100, Thomas Ackermann wrote:
> >
> >> This patch addresses Junios comment in WC:
> >> "Misapplication of a patch fixed; the ones near the tip needs to
> >>  update the links to point at the html files, though."
> >> 
> >> See older mail in this thread:
> >> [...]
> >> That means that for the patch [6/8], which adds content-type to the
> >> text files, to be complete, it needs to update Makefile to produce
> >> html files from them.
> >> [...]
> >> So IMHO no open issues with this patch.
> >
> > OK, that explains the situation. Thanks, I'll merge it to master in the
> > next iteration.
> 
> What happened to this topic?

I mistyped in the above response; it should have been "next" not
"master".

But then I forgot to merge it to next in the last iteration; it is
ta/doc-cleanup in pu, and is marked in "What's Cooking" to be merged to
next.

-Peff

^ 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