How to setup gitweb with categories

2012-11-13 Thread frank . jakop
Hello,

I'm trying to setup a gitweb frontend with categorized projects. I already 

have a plain project list running an changed my gitweb.conf so that it 
contains

our $projects_list_group_categories = "1";
our $project_list_default_category = "foo";

I'd now expect all projects to be shown under the category "foo", but upon 

reload nothing happens on the web page.

Can anyone tell me what's wrong?

Best regards

Frank
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 0/5] push: update remote tags only with force

2012-11-13 Thread Chris Rorvick
resending to list ...

On Tue, Nov 13, 2012 at 3:20 PM, Junio C Hamano  wrote:
> Chris Rorvick  writes:
>
>> Minor changes since from v2 set.  Reposting primarily because I mucked
>> up the Cc: list (again) and hoping to route feedback to the appropriate
>> audience.
>>
>> This patch set can be divided into two sets:
>>
>>   1. Provide useful advice for rejected tag references.
>>
>>  push: return reject reasons via a mask
>>  push: add advice for rejected tag reference
>>
>>  Recommending a merge to resolve a rejected tag update seems
>>  nonsensical since the tag does not come along for the ride.  These
>>  patches change the advice for rejected tags to suggest using
>>  "push -f".
>
> Below, I take that you mean by "tag reference" everything under
> refs/tags/ (not limited to "annotated tag objects", but also
> lightweight tags).

Yes.

> Given that the second point below is to strongly discourage updating
> of existing any tag, it might be even better to advise *not* to push
> tags in the first place, instead of destructive "push -f", no?

That does seem like a better idea.  Read the full manual page to
figure out how to force the update if that's what you want to do--the
advice should not suggest something exceptional.

>>   2. Require force when updating tag references, even on a fast-forward.
>>
>>  push: flag updates
>>  push: flag updates that require force
>>  push: update remote tags only with force
>>
>>  An email thread initiated by Angelo Borsotti did not come to a
>>  consensus on how push should behave with regard to tag references.
>
> I think the original motivation of allowing fast-forward updates to
> tags was for people who wanted to have "today's recommended version"
> tag that can float from day to day. I tend to think that was a
> misguided notion and it is better implemented with a tip of a
> branch (iow, I personally am OK with the change to forbid tag
> updates altogether, without --force).
>
>>  I think a key point is that you currently cannot be sure your push
>>  will not clobber a tag (lightweight or not) in the remote.
>
> "Do not update, only add new" may be a good feature, but at the same
> time I have this suspicion that its usefulness may not necessarily
> be limited to refs/tags/* hierarchy.
>
> I dunno.

Are you suggesting allowing forwards for just refs/heads/*?  I
initially went this route based on some feedback in the original
thread, but being that specific broke a couple tests in t5516 (i.e.,
pushing to refs/remotes/origin/master and another into refs/tmp/*.)
My initial thought was that I'd broken something and I need to modify
the patch, but now I think I should just modify those tests.  Branches
are restricted to refs/heads/* (if I understand correctly), so
allowing fast-forwards when pushing should be limited to this
hierarchy, too.

Thanks,

Chris
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] config: don't segfault when given --path with a missing value

2012-11-13 Thread Carlos Martín Nieto
When given a variable without a value, such as '[section] var' and
asking git-config to treat it as a path, git_config_pathname returns
an error and doesn't modify its output parameter. show_config assumes
that the call is always successful and sets a variable to indicate
that vptr should be freed. In case of an error however, trying to do
this will cause the program to be killed, as it's pointing to memory
in the stack.

Set the must_free_vptr flag depending on the return value of
git_config_pathname so it's accurate.
---
 builtin/config.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 442ccc2..60220d5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -129,8 +129,7 @@ static int show_config(const char *key_, const char 
*value_, void *cb)
else
sprintf(value, "%d", v);
} else if (types == TYPE_PATH) {
-   git_config_pathname(&vptr, key_, value_);
-   must_free_vptr = 1;
+   must_free_vptr = !git_config_pathname(&vptr, key_, value_);
} else if (value_) {
vptr = value_;
} else {
-- 
1.8.0.316.g291341c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread Marc Khouzam
On Tue, Nov 13, 2012 at 6:46 PM, SZEDER Gábor  wrote:
> Hi,
>
> On Tue, Nov 13, 2012 at 03:12:44PM -0500, Marc Khouzam wrote:
>> >> +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"
>> >
>> > Where does the second argument come from?  Below you run this script
>> > as '${__git_tcsh_completion_script} "${COMMAND_LINE}"', i.e. $2 is
>> > never set.  Am I missing something?
>>
>> This second argument is optional and, if present, will be put in
>> $COMP_CWORD.  If not present, $COMP_CWORD must be computed
>> from $1.  Also see comment above _git_complete_with_output ().
>> tcsh does not provide me with this information, so I cannot make use of it.
>> However, I thought it would be more future-proof to allow it for other shells
>> which may have that information.
>>
>> It is not necessary for tcsh, so I can remove if you prefer?
>
> I see.  I read those comments and understood what it is about.  I was
> just surprised that the code is there to make use of it, yet it's not
> specified when invoking that function.
>
> Since it's a trivial piece of code, I would say let's keep it.  Could
> you please add a sentence about it (that it's for possible future
> users and it's not used at the moment) to the commit message for
> future reference?

Will do.

>> >> +complete git  'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
>> >> | sort | uniq`/'
>> >> +complete gitk 'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
>> >> | sort | uniq`/'
>> >
>> > Is the 'sort | uniq' really necessary?  After the completion function
>> > returns Bash automatically sorts the elements in COMPREPLY and removes
>> > any duplicates.  Doesn't tcsh do the same?  I have no idea about tcsh
>> > completion.
>>
>> On my machine, tcsh does not remove duplicates.  It does sort the results
>> but that is done after I've run 'uniq', which is too late.  I'm not
>> happy about this
>> either, but the other option is to improve git-completion.bash to
>> avoid duplicates,
>> which seemed less justified.
>
> Ok.  Then keep it for the time being, and we'll see what we can do to
> avoid those duplicates.

Thanks.

>> > Does the git completion script returns any duplicates at all?
>>
>> It does.  'help' is returned twice for example.
>
> Right.  Now that you mentioned it, I remember I noticed it a while
> ago, too.  I even wrote a patch to fix it, but not sure what became of
> it.  Will try to dig it up.

Thanks for already posting the patch.

>> Also, when completing 'git checkout ' in the git repo, I can see multiple
>> 'todo' branches, as well as 'master', 'pu', 'next', etc.
>>
>> You can actually try it without tcsh by running my proposed version of
>> git-completion.bash like this:
>>
>> cd git/contrib/completion
>> bash git-completion.bash "git checkout " | sort | uniq --repeated
>
> Interesting, I can't reproduce.  Are the duplicates also there, if you
> start a bash, source git-completion.bash, and run __git_refs ?

Running __git_refs does not show the duplicates, but running
__git refs '' 1
does show them.
That second parameter causes __git_refs to
"use the guess heuristic employed by checkout for tracking branches"

I don't quite understand this, but what I can see is that my remote
branches GitHub/master and origin/master each cause another
'master' to be listed:

$ __git_refs '' 1|grep master
master
GitHub/master
origin/master
master
master

All fixes are done and I'll post a second version of the patch
as soon as I can figure out the formatting properly.

Thanks again

Marc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread Marc Khouzam
Thanks for the review.  I wasn't aware that you were doing
a similar effort for zsh.

On Tue, Nov 13, 2012 at 1:31 PM, Felipe Contreras
 wrote:
> On Mon, Nov 12, 2012 at 9:07 PM, Marc Khouzam  wrote:
>
>> 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.
>
> Maybe you can try to take a look at the same for zsh:
> http://article.gmane.org/gmane.comp.version-control.git/208173

Cool.  The major difference is that (as Gábor mentioned) zsh understands bash
syntax but tcsh does not.  tcsh doesn't even allow to define
functions.  So we have
to take a different approach to get the bash completion script to be
used by tcsh.

>> ---
>>  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]}
>
> You might want to use __${COMP_WORDS[0]}_main instead.
>
>> +
>> +   # 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
>
> Why do you need this function in this file? You can very easily add
> this function to git-completion.tcsh.

tcsh does not allow to define functions, so it is not aware of any
of the git-completion.bash functions.  So, git-completion.tcsh
cannot call anything from git-completion.bash.

>> diff --git a/contrib/completion/git-completion.tcsh
>> b/contrib/completion/git-completion.tcsh
>> new file mode 100755
>> index 000..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 
>> +# 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

Re: Bug? Subtree merge seems to choke on trailing slashes.

2012-11-13 Thread Jack O'Connor
Do I have the right list for bug reports? Apologies if not.

On Tue, Nov 6, 2012 at 5:58 PM, Jack O'Connor  wrote:
>
> I'm summarizing from here:
> http://stackoverflow.com/questions/5904256/git-subtree-merge-into-a-deeply-nested-subdirectory
>
> Quick repro:
> 1) I do an initial subtree merge in what I think is the standard way
> (http://www.kernel.org/pub/software/scm/git/docs/howto/using-merge-subtree.html).
> My prefix is simply "test/".
> 2) I try to merge more upstream changes on top of that with the
> following command:
> git merge --strategy-option=subtree='test/' $upstream_stuff
> 3) Git fails with an obscure error:
> fatal: entry  not found in tree daf4d0f0a20b8b6ec007be9fcafeac84a6eba4f0
>
> If I remove the trailing slash from the command in step 2, it works just fine:
> git merge --strategy-option=subtree='test' $upstream_stuff
>
> Note in the error message above, there's a double space after "entry".
> Is it looking for a tree with an empty name? Did my trailing slash
> imply a directory named empty-string?
>
> Thanks for your help.
>
> -- Jack O'Connor
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: What's cooking in git.git (Nov 2012, #03; Tue, 13)

2012-11-13 Thread Mark Levedahl

On 11/13/2012 03:45 PM, Torsten Bögershausen wrote:

* ml/cygwin-mingw-headers (2012-11-12) 1 commit
  - Update cygwin.c for new mingw-64 win32 api headers

  Make git work on newer cygwin.

  Will merge to 'next'.

(Sorry for late answer, I managed to test the original patch minutes before 
Peff merged it to pu)
(And thanks for maintaining git)

Is everybody using cygwin happy with this?

I managed to compile on a fresh installed cygwin,
but failed to compile under 1.7.7, see below.
Is there a way we can achieve to compile git both under "old" and "new" cygwin 
1.7 ?
Or is this not worth the effort?
/Torsten



I found no version info defined that could be used to automatically 
switch between the old and current headers. You can always


make V15_MINGW_HEADERS=1 ...

to force using the old set if you do not wish to update your installation.

Mark
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] completion: remove 'help' duplicate from porcelain commands

2012-11-13 Thread SZEDER Gábor
The list of all git commands is computed from the output of 'git help
-a', which already includes 'help', so there is no need to explicitly
add it once more when computing the list of porcelain commands.

Note that 'help' wasn't actually offered twice because of this,
because Bash filters duplicates from possible completion words.

Signed-off-by: SZEDER Gábor 
---

> > > Does the git completion script returns any duplicates at all?
> > 
> > It does.  'help' is returned twice for example.
> 
> Right.  Now that you mentioned it, I remember I noticed it a while
> ago, too.  I even wrote a patch to fix it, but not sure what became of
> it.  Will try to dig it up.

Here it is.  It turns out I wrote it in May this year, but according to
gmane and my mailbox never sent it out.

 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index bc0657a2..b7b1a834 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -585,7 +585,7 @@ __git_list_porcelain_commands ()
 {
local i IFS=" "$'\n'
__git_compute_all_commands
-   for i in "help" $__git_all_commands
+   for i in $__git_all_commands
do
case $i in
*--*) : helper pattern;;
-- 
1.8.0.128.g441b4b3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread SZEDER Gábor
On Tue, Nov 13, 2012 at 07:31:45PM +0100, Felipe Contreras wrote:
> On Mon, Nov 12, 2012 at 9:07 PM, Marc Khouzam  wrote:
> > +   # Call _git() or _gitk() of the bash script, based on the first
> > +   # element of the command-line
> > +   _${COMP_WORDS[0]}
> 
> You might want to use __${COMP_WORDS[0]}_main instead.

That wouldn't work.  __git_main() doesn't set up the
command-line-specific variables, but the wrapper around it does.


> > +# Make the script executable if it is not
> > +if ( ! -x ${__git_tcsh_completion_script} ) then
> > +   chmod u+x ${__git_tcsh_completion_script}
> > +endif
> 
> Why not just source it?

The goal is to re-use a Bash script to do completion in tcsh.  They
are two different breeds, tcsh doesn't grok bash.  So sourcing the
completion script is not an option, but we can still run it via Bash
and use it's results.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread SZEDER Gábor
Hi,


I've got two more comments.

On Mon, Nov 12, 2012 at 03:07:46PM -0500, Marc Khouzam wrote:
> @@ -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 ()

We differentiate between _git_whatever() and __git_whatever()
functions.  The former performs completion for the 'whatever' git
command/alias, the latter is a completion helper function.  This
is a helper function, so it should begin with double underscores.

> +{
> +   # 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

A while ago the completion script was made 'set -u'-clean.  (If 'set
-u' is enabled, then it's an error to access undefined variables).
I'm not sure how many people are out there who'd use this script for
tcsh while having 'set -u' in their profile...  probably not that
many.  Still, I think it would be great to keep it up.

Here $2 would be undefined, so accessingit it would cause an error
under those semantincs.  Please use ${2-} instead (use empty string
when undefined).

> +if [ -n "$1" ] ; then

Same here.

> +  # 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"

And here.

Thanks
Gábor

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 0/5] push: update remote tags only with force

2012-11-13 Thread Drew Northup
On Sun, Nov 11, 2012 at 11:08 PM, Chris Rorvick  wrote:
> Minor changes since from v2 set.
.

>  An email thread initiated by Angelo Borsotti did not come to a
>  consensus on how push should behave with regard to tag references.

Minor Nit: Without the link to gmane it is an exercise left to the
reviewer to find that you're talking about this thread:
http://thread.gmane.org/gmane.comp.version-control.git/208354

Cheers.

-- 
-Drew Northup
--
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv3 3/4] git-status: show short sequencer state

2012-11-13 Thread Phil Hord
Phil Hord wrote:
> Junio C Hamano wrote:
>> Phil Hord  writes:
>>
>>> State token strings which may be emitted and their meanings:
>>> merge  a merge is in progress
>>> am an am is in progress
>>> am-is-emptythe am patch is empty
>>> rebase a rebase is in progress
>>> rebase-interactive an interactive rebase is in progress
>>> cherry-picka cherry-pick is in progress
>>> bisect a bisect is in progress
>>> conflicted there are unresolved conflicts
>>> commit-pending a commit operation is waiting to be completed
>>> splitting  interactive rebase, commit is being split
>>>
>>> I also considered adding these tokens, but I decided it was not
>>> appropriate since these changes are not sequencer-related.  But
>>> it is possible I am being too short-sighted or have chosen the
>>> switch name poorly.
>>> changed-index  Changes exist in the index
>>> changed-files  Changes exist in the working directory
>>> untracked  New files exist in the working directory
>> I tend to agree; unlike all the normal output from "status -s" that
>> are per-file, the above are the overall states of the working tree.
>>
>> It is just that most of the "overall states" look as if they are
>> dominated by "sequencer states", but that is only because you chose
>> to call states related to things like "am" and "bisect" that are not
>> sequencer states as such.
>>
>> It probably should be called the tree state, working tree state, or
>> somesuch.
> I think you are agreeing that I chose the switch name poorly, right?
>
> Do you think '--tree-state' is an acceptable switch or do you have other
> suggestions?
>

I've been calling these 'tokens' myself.  A token is a word-or-phrase I
can parse easily with the default $IFS, for simpler script handling.

I'm happy to make that official and use --tokens and -T, but I suspect a
more appropriate name is available.

Phil

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread SZEDER Gábor
Hi,

On Tue, Nov 13, 2012 at 03:12:44PM -0500, Marc Khouzam wrote:
> >> +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"
> >
> > Where does the second argument come from?  Below you run this script
> > as '${__git_tcsh_completion_script} "${COMMAND_LINE}"', i.e. $2 is
> > never set.  Am I missing something?
> 
> This second argument is optional and, if present, will be put in
> $COMP_CWORD.  If not present, $COMP_CWORD must be computed
> from $1.  Also see comment above _git_complete_with_output ().
> tcsh does not provide me with this information, so I cannot make use of it.
> However, I thought it would be more future-proof to allow it for other shells
> which may have that information.
> 
> It is not necessary for tcsh, so I can remove if you prefer?

I see.  I read those comments and understood what it is about.  I was
just surprised that the code is there to make use of it, yet it's not
specified when invoking that function.

Since it's a trivial piece of code, I would say let's keep it.  Could
you please add a sentence about it (that it's for possible future
users and it's not used at the moment) to the commit message for
future reference?

> >> +complete git  'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
> >> | sort | uniq`/'
> >> +complete gitk 'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
> >> | sort | uniq`/'
> >
> > Is the 'sort | uniq' really necessary?  After the completion function
> > returns Bash automatically sorts the elements in COMPREPLY and removes
> > any duplicates.  Doesn't tcsh do the same?  I have no idea about tcsh
> > completion.
> 
> On my machine, tcsh does not remove duplicates.  It does sort the results
> but that is done after I've run 'uniq', which is too late.  I'm not
> happy about this
> either, but the other option is to improve git-completion.bash to
> avoid duplicates,
> which seemed less justified.

Ok.  Then keep it for the time being, and we'll see what we can do to
avoid those duplicates.

> > Does the git completion script returns any duplicates at all?
> 
> It does.  'help' is returned twice for example.

Right.  Now that you mentioned it, I remember I noticed it a while
ago, too.  I even wrote a patch to fix it, but not sure what became of
it.  Will try to dig it up.

> Also, when completing 'git checkout ' in the git repo, I can see multiple
> 'todo' branches, as well as 'master', 'pu', 'next', etc.
> 
> You can actually try it without tcsh by running my proposed version of
> git-completion.bash like this:
> 
> cd git/contrib/completion
> bash git-completion.bash "git checkout " | sort | uniq --repeated

Interesting, I can't reproduce.  Are the duplicates also there, if you
start a bash, source git-completion.bash, and run __git_refs ?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: checkout from neighbour branch undeletes a path?

2012-11-13 Thread Junio C Hamano
Peter Vereshagin  writes:

>   $ rm -r pathdir
>   $ git checkout branch00 pathdir
>   $ find pathdir/
>   pathdir/
>   pathdir/file00.txt
>   pathdir/file01.txt
>   $

Hasn't this been fixed at 0a1283b (checkout $tree $path: do not
clobber local changes in $path not in $tree, 2011-09-30)?

Are you using 1.7.7.1 or newer?  If not, please upgrade.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: bug? git format-patch -M -D then git am fails

2012-11-13 Thread Junio C Hamano
Joe Perches  writes:

> I don't believe that reversibility
> is a really useful aspect of deletion patches
> when there are known git repositories involved.

You can read "reversibility" as "safety" if you want.  We would want
to make sure we know what we are deleting before deleting a path.

The history that the receiver of such a patch has may have further
changes that are relevant that the sender of the deletion patch did
not know about, and removing the path in such a case would make the
result inconsistent.  If the sender did his work on top of the newer
version with the change in the path, the sender's patch may still
have deleted the path but would have had changes to other paths to
compensate for the loss of that change.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] pickaxe: use textconv for -S counting

2012-11-13 Thread Junio C Hamano
Jeff King  writes:

> We currently just look at raw blob data when using "-S" to
> pickaxe. This is mostly historical, as pickaxe predates the
> textconv feature. If the user has bothered to define a
> textconv filter, it is more likely that their search string will be
> on the textconv output, as that is what they will see in the
> diff (and we do not even provide a mechanism for them to
> search for binary needles that contain NUL characters).

Oookay, I suppose...

>  static int has_changes(struct diff_filepair *p, struct diff_options *o,
>  regex_t *regexp, kwset_t kws)
>  {
> + struct userdiff_driver *textconv_one = get_textconv(p->one);
> + struct userdiff_driver *textconv_two = get_textconv(p->two);
> + mmfile_t mf1, mf2;
> + int ret;
> +
>   if (!o->pickaxe[0])
>   return 0;
>  
> - if (!DIFF_FILE_VALID(p->one)) {
> - if (!DIFF_FILE_VALID(p->two))
> - return 0; /* ignore unmerged */

What happened to this part that avoids showing nonsense for unmerged
paths?

> + /*
> +  * If we have an unmodified pair, we know that the count will be the
> +  * same and don't even have to load the blobs. Unless textconv is in
> +  * play, _and_ we are using two different textconv filters (e.g.,
> +  * because a pair is an exact rename with different textconv attributes
> +  * for each side, which might generate different content).
> +  */
> + if (textconv_one == textconv_two && diff_unmodified_pair(p))
> + return 0;

I am not sure about this part that cares about the textconv.

Wouldn't the normal "git diff A B" skip the filepair that are
unmodified in the first place at the object name level without even
looking at the contents (see e.g. diff_flush_patch())?

Shouldn't this part of the code emulating that behaviour no matter
what textconv filter(s) are configured for these paths?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: bug? git format-patch -M -D then git am fails

2012-11-13 Thread Joe Perches
On Tue, 2012-11-13 at 14:55 -0800, Junio C Hamano wrote:
> Joe Perches  writes:
> 
> > (Sorry about the partial message.
> >  evolution and ctrl-enter sends, grumble...)
> >
> > If a file is deleted with git rm and a patch
> > is then generated with git format-patch -M -D
> > git am is unable to apply the resultant patch.
> >
> > Is this working as designed?
> 
> I would say it is broken as designed and it is even documented.
> 
> Please run "git format-patch --help | less" and then type
> "/--irreversible-delete" to find:
> 
> The resulting patch is not meant to be applied with patch nor
> git apply; this is solely for people who want to just
> concentrate on reviewing the text after the change.

yeah, it's just that not using -D can result in
some unfortunately large patches being sent to
mailing lists.  I don't believe that reversibility
is a really useful aspect of deletion patches
when there are known git repositories involved.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: What's cooking in git.git (Nov 2012, #03; Tue, 13)

2012-11-13 Thread Junio C Hamano
Jeff King  writes:

> What's cooking in git.git (Nov 2012, #03; Tue, 13)
> --
>
> Here are the topics that have been cooking.  Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'.
>
> This is my final "what's cooking" as interim maintainer. I didn't
> graduate anything to master, but I updated my plans for each topic to
> give Junio an idea of where I was.
>
> You can find the changes described here in the integration branches of
> my repository at:
>
>   git://github.com/peff/git.git
>
> Until Junio returns, kernel.org and the other "usual" places will not be
> updated.

And now the "usual places" have been updated with the same tips of
integration branches (the broken-out https://github.com/gitster/git
repository, too).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: bug? git format-patch -M -D then git am fails

2012-11-13 Thread Junio C Hamano
Joe Perches  writes:

> (Sorry about the partial message.
>  evolution and ctrl-enter sends, grumble...)
>
> If a file is deleted with git rm and a patch
> is then generated with git format-patch -M -D
> git am is unable to apply the resultant patch.
>
> Is this working as designed?

I would say it is broken as designed and it is even documented.

Please run "git format-patch --help | less" and then type
"/--irreversible-delete" to find:

The resulting patch is not meant to be applied with patch nor
git apply; this is solely for people who want to just
concentrate on reviewing the text after the change.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


bug? git format-patch -M -D then git am fails

2012-11-13 Thread Joe Perches
(Sorry about the partial message.
 evolution and ctrl-enter sends, grumble...)

If a file is deleted with git rm and a patch
is then generated with git format-patch -M -D
git am is unable to apply the resultant patch.

Is this working as designed?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


bug? git format-patch -M -D then git am fails

2012-11-13 Thread Joe Perches
If a file is deleted with git rm and a patch
is then generated with git format-patch -M -

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [regression] Newer gits cannot clone any remote repos

2012-11-13 Thread Andreas Schwab
Torsten Bögershausen  writes:

> Are there more people running PowerPC (on the server side) ?

I cannot reproduce the problem (on openSUSE 12.2).

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance

2012-11-13 Thread Karsten Blees
Am 13.11.2012 17:46, schrieb Junio C Hamano:
> karsten.bl...@dcon.de writes:
> 
> If anything, "fix your mailer" probably is the policy you are
> looking for, I think.

Well then...I've cloned myself @gmail, I hope this is better.

Just some provoking thoughts...(if I may):

RFC-5322 recommends wrapping lines at 78, and mail relays and gateways are 
allowed to change message content according to the capabilities of the receiver 
(RFC-5598). In essence, plaintext mail is completely unsuitable for 
preformatted text such as source code.

On the other hand, git tries to solve the very problem of distributed source 
code management, and consistency by strong sha-1 checksums is on the top of the 
feature list.

It is somehow hard to believe that contributing to git itself should only be 
possible using the most unreliable of protocols. Don't you trust your own 
software?


-- >8 --
Subject: [PATCH] update-index/diff-index: use core.preloadindex to improve 
performance

'update-index --refresh' and 'diff-index' (without --cached) don't honor
the core.preloadindex setting yet. Porcelain commands using these (such as
git [svn] rebase) suffer from this, especially on Windows.

Use read_cache_preload to improve performance.

Additionally, in builtin/diff.c, don't preload index status if we don't
access the working copy (--cached).

Results with msysgit on WebKit repo (2GB in 200k files):

| update-index | diff-index | rebase
+--++-
msysgit-v1.8.0  |   9.157s |10.536s | 42.791s
+ preloadindex  |   9.157s |10.536s | 28.725s
+ this patch|   2.329s | 2.752s | 15.152s
+ fscache [1]   |   0.731s | 1.171s |  8.877s

[1] https://github.com/kblees/git/tree/kb/fscache-v3

Thanks-to: Albert Krawczyk 
Signed-off-by: Karsten Blees 
---

Can also be pulled from: 
https://github.com/kblees/git/tree/kb/update-diff-index-preload-upstream

More performance figures (for msysgit) can be found in this discussion: 
https://github.com/pro-logic/git/commit/32c03dd8


 builtin/diff-index.c   |  8 ++--
 builtin/diff.c | 12 
 builtin/update-index.c |  1 +
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 2eb32bd..1c737f7 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -41,9 +41,13 @@ int cmd_diff_index(int argc, const char **argv, const char 
*prefix)
if (rev.pending.nr != 1 ||
rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
usage(diff_cache_usage);
-   if (!cached)
+   if (!cached) {
setup_work_tree();
-   if (read_cache() < 0) {
+   if (read_cache_preload(rev.diffopt.pathspec.raw) < 0) {
+   perror("read_cache_preload");
+   return -1;
+   }
+   } else if (read_cache() < 0) {
perror("read_cache");
return -1;
}
diff --git a/builtin/diff.c b/builtin/diff.c
index 9650be2..198b921 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -130,8 +130,6 @@ static int builtin_diff_index(struct rev_info *revs,
usage(builtin_diff_usage);
argv++; argc--;
}
-   if (!cached)
-   setup_work_tree();
/*
 * Make sure there is one revision (i.e. pending object),
 * and there is no revision filtering parameters.
@@ -140,8 +138,14 @@ static int builtin_diff_index(struct rev_info *revs,
revs->max_count != -1 || revs->min_age != -1 ||
revs->max_age != -1)
usage(builtin_diff_usage);
-   if (read_cache_preload(revs->diffopt.pathspec.raw) < 0) {
-   perror("read_cache_preload");
+   if (!cached) {
+   setup_work_tree();
+   if (read_cache_preload(revs->diffopt.pathspec.raw) < 0) {
+   perror("read_cache_preload");
+   return -1;
+   }
+   } else if (read_cache() < 0) {
+   perror("read_cache");
return -1;
}
return run_diff_index(revs, cached);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 74986bf..ada1dff 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -593,6 +593,7 @@ struct refresh_params {
 static int refresh(struct refresh_params *o, unsigned int flag)
 {
setup_work_tree();
+   read_cache_preload(NULL);
*o->has_errors |= refresh_cache(o->flags | flag);
return 0;
 }
-- 
1.8.0.msysgit.0.3.g7d9d98c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 0/5] push: update remote tags only with force

2012-11-13 Thread Junio C Hamano
Chris Rorvick  writes:

> Minor changes since from v2 set.  Reposting primarily because I mucked
> up the Cc: list (again) and hoping to route feedback to the appropriate
> audience.
>
> This patch set can be divided into two sets:
>
>   1. Provide useful advice for rejected tag references.
>
>  push: return reject reasons via a mask
>  push: add advice for rejected tag reference
>
>  Recommending a merge to resolve a rejected tag update seems
>  nonsensical since the tag does not come along for the ride.  These
>  patches change the advice for rejected tags to suggest using
>  "push -f".

Below, I take that you mean by "tag reference" everything under
refs/tags/ (not limited to "annotated tag objects", but also
lightweight tags).

Given that the second point below is to strongly discourage updating
of existing any tag, it might be even better to advise *not* to push
tags in the first place, instead of destructive "push -f", no?

>   2. Require force when updating tag references, even on a fast-forward.
>
>  push: flag updates
>  push: flag updates that require force
>  push: update remote tags only with force
>
>  An email thread initiated by Angelo Borsotti did not come to a
>  consensus on how push should behave with regard to tag references.

I think the original motivation of allowing fast-forward updates to
tags was for people who wanted to have "today's recommended version"
tag that can float from day to day. I tend to think that was a
misguided notion and it is better implemented with a tip of a
branch (iow, I personally am OK with the change to forbid tag
updates altogether, without --force).

>  I think a key point is that you currently cannot be sure your push
>  will not clobber a tag (lightweight or not) in the remote.

"Do not update, only add new" may be a good feature, but at the same
time I have this suspicion that its usefulness may not necessarily
be limited to refs/tags/* hierarchy.

I dunno.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [regression] Newer gits cannot clone any remote repos

2012-11-13 Thread Torsten Bögershausen
On 13.11.12 19:55, Ramsay Jones wrote:
> Douglas Mencken wrote:
>> *Any* git clone fails with:
>>
>> fatal: premature end of pack file, 106 bytes missing
>> fatal: index-pack failed
>>
>> At first, I tried 1.8.0, and it failed. Then I tried to build 1.7.10.5
>> then, and it worked. Then I tried 1.7.12.2, but it fails the same way
>> as 1.8.0.
>> So I decided to git bisect.
>>
>> b8a2486f1524947f232f657e9f2ebf44e3e7a243 is the first bad commit
>> ``index-pack: support multithreaded delta resolving''
> 
> This looks like the same problem I had on cygwin, which lead to
> commit c0f86547c ("index-pack: Disable threading on cygwin", 26-06-2012).
> 
> I didn't notice which platform you are on, but maybe you also have a
> thread-unsafe pread()? Could you try re-building git with the
> NO_THREAD_SAFE_PREAD build variable set?
> 
> HTH.
> 
> ATB,
> Ramsay Jones

This is interesting.
I had the same problem on a PowerPC 
(Old PowerBook G4 running Linux).

Using NO_THREAD_SAFE_PREAD helped, thanks for the hint.
(After recompiling without NO_THREAD_SAFE_PREAD I could clone
from this machine again, so the problem is not really reproducable)

Are there more people running PowerPC (on the server side) ?
/Torsten

 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks

2012-11-13 Thread David Aguilar
On Mon, Nov 12, 2012 at 9:47 AM, Junio C Hamano  wrote:
> Michael Haggerty  writes:
>
>> The log message of the original commit (0454dd93bf) described the
>> following scenario: a /home partition under which user home directories
>> are automounted, and setting GIT_CEILING_DIRECTORIES=/home to avoid
>> hitting /home/.git, /home/.git/objects, and /home/objects (which would
>> attempt to automount those directories).  I believe that this scenario
>> would not be slowed down by my patches.
>>
>> How do you use GIT_CEILING_DIRECTORIES that the proposed changes cause a
>> slowdown?
>
> Yeah, I was also wondering about that.
>
> David?

I double-checked our configuration and all the parent directories
of those listed in GIT_CEILING_DIRECTORIES are local,
so our particular configuration would not have a performance hit.

We do have multiple directories listed there.  Some of them share
a parent directory.  I'm assuming the implementation is simple and
does not try and avoid repeating the check when the parent dir is
the same across multiple entries.

In any case, it won't be a problem in practice based on my
reading of the current code.



>>> Is there another way to accomplish this without the performance hit?
>>> Maybe something that can be solved with configuration?
>>
>> Without doing the symlink expansion there is no way for git to detect
>> that GIT_CEILING_DIRECTORIES contains symlinks and is therefore
>> ineffective.  So the user has no warning about the misconfiguration
>> (except that git runs slowly).
>>
>> On 10/29/2012 02:42 AM, Junio C Hamano wrote:
>>> Perhaps not canonicalize elements on the CEILING list ourselves? If
>>> we make it a user error to put symlinked alias in the variable, and
>>> document it clearly, wouldn't it suffice?
>>
>> There may be no other choice.  (That, and fix the test suite in another
>> way to tolerate a $PWD that involves symlinks.)
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: What's cooking in git.git (Nov 2012, #03; Tue, 13)

2012-11-13 Thread Pyeron, Jason J CTR (US)
> -Original Message-
> From: Torsten Bögershausen
> Sent: Tuesday, November 13, 2012 3:45 PM
> 
> > * ml/cygwin-mingw-headers (2012-11-12) 1 commit
> >  - Update cygwin.c for new mingw-64 win32 api headers
> >
> >  Make git work on newer cygwin.
> >
> >  Will merge to 'next'.
> 
> (Sorry for late answer, I managed to test the original patch minutes
> before Peff merged it to pu)
> (And thanks for maintaining git)
> 
> Is everybody using cygwin happy with this?
> 
> I managed to compile on a fresh installed cygwin,
> but failed to compile under 1.7.7, see below.
> Is there a way we can achieve to compile git both under "old" and "new"
> cygwin 1.7 ?
> Or is this not worth the effort?

Only supporting the new cygwin would make sense. You have to work hard at using 
older cygwin environments. I will give it a spin later today or tomorrow.

-Jason 


smime.p7s
Description: S/MIME cryptographic signature


Re: What's cooking in git.git (Nov 2012, #03; Tue, 13)

2012-11-13 Thread Torsten Bögershausen
> * ml/cygwin-mingw-headers (2012-11-12) 1 commit
>  - Update cygwin.c for new mingw-64 win32 api headers
> 
>  Make git work on newer cygwin.
> 
>  Will merge to 'next'.

(Sorry for late answer, I managed to test the original patch minutes before 
Peff merged it to pu)
(And thanks for maintaining git)

Is everybody using cygwin happy with this?

I managed to compile on a fresh installed cygwin,
but failed to compile under 1.7.7, see below.
Is there a way we can achieve to compile git both under "old" and "new" cygwin 
1.7 ?
Or is this not worth the effort?
/Torsten



CC compat/cygwin.o
In file included from compat/../git-compat-util.h:90,
 from compat/cygwin.c:9:
/usr/lib/gcc/i686-pc-cygwin/3.4.4/../../../../include/w32api/winsock2.h:103:2: 
warning: #warning "fd_set and associated macros have been defined in sys/types. 
 This may cause runtime problems with W32 sockets"
In file included from /usr/include/sys/socket.h:16,
 from compat/../git-compat-util.h:131,
 from compat/cygwin.c:9:
/usr/include/cygwin/socket.h:29: error: redefinition of `struct sockaddr'
/usr/include/cygwin/socket.h:41: error: redefinition of `struct 
sockaddr_storage'
In file included from /usr/include/sys/socket.h:16,
 from compat/../git-compat-util.h:131,
 from compat/cygwin.c:9:
/usr/include/cygwin/socket.h:59: error: redefinition of `struct linger'
In file included from compat/../git-compat-util.h:131,
 from compat/cygwin.c:9:
/usr/include/sys/socket.h:30: error: conflicting types for 'accept'
/usr/lib/gcc/i686-pc-cygwin/3.4.4/../../../../include/w32api/winsock2.h:536: 
error: previous declaration of 'accept' was here

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] send-email: add proper default sender

2012-11-13 Thread Felipe Contreras
On Tue, Nov 13, 2012 at 5:48 PM, Jeff King  wrote:
> On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:

>> I think you are the one that is not understanding what I'm saying. But
>> I don't think it matters.
>>
>> This is what I'm saying; the current situation with 'git commit' is
>> not OK, _both_ 'git commit' and 'git send-email' should change.
>
> Perhaps I am being dense, but this is the first time it became apparent
> to me that you are arguing for a change in "git commit".

Miscomunication then. When you mentioned 'it has always been the case
that you can use git without setting
user.*', I directed my comments at git in general, not git send-email.

>> Indeed I would, but there's other people that would benefit from this
>> patch. I'm sure I'm not the only person that doesn't have
>> sendmail.from configured, but does have user.name/user.email, and is
>> constantly typing enter.
>>
>> And the difference is that I'm _real_, the hypothetical user that
>> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced
>> otherwise if some evidence was presented that such a user is real
>> though.
>
> Sadly we cannot poll the configuration of every user, nor expect them
> all to pay attention to this discussion on the list. So we cannot take
> the absence of comment from such users as evidence that they do not
> exist.

And we cannot take it as evidence that these users do exist either.

The absence of evidence simply means that... we don't have evidence.

> Instead we must use our judgement about what is being changed,
> and we tend to err on the side of keeping the status quo, since that is
> what the silent majority is busy _not_ complaining about.

Yes, the keyword is *tend*; this wouldn't be first or the last time
that a behavior change happens.

We have to be careful about these changes, but we do them.

> We use the same judgement on the other side, too. Right now your
> evidence is "1 user wants this, 0 users do not".

1 is infinitely greater than 0.

> But we can guess that
> there are other people who would like the intent of your patch, but did
> not care enough or are not active enough on the list to write the patch
> themselves or comment on this thread.

Yes, that would be an educated guess. IMO the fact that people use
GIT_AUTHOR_ variables to send mail is not. There's many ways to
achieve that: sendemail.from, --from, user configuration, $EMAIL,
--compose and From:, etc. each and every one of them much more likely
to be used than GIT_AUTHOR_.

But I'm tired of arguing how extremely unlikely it is that such people
don't exist. Lets agree to disagree. Either way it doesn't matter,
because nobody is proposing a patch that would affect these
hypothetical users.

>> And to balance you need to *measure*, and that means taking into
>> consideration who actually uses the features, if there's any. And it
>> looks to me this is a feature nobody uses.
>
> You said "measure" and then "it looks to me like". What did you measure?
> Did you consider systematic bias in your measurement, like the fact that
> people who are using the feature have no reason to come on the list and
> announce it?

measure != scientific measurement.

I used common sense, because that's the only tool available.

GIT_AUTHOR is plumbing, not very well known, it's cumbersome (you need
to export two variables), it can be easily confused with
GIT_COMMITTER, which wouldn't work on this case. And there's plenty of
tools that much simpler to use, starting with 'git send-email --from',
which is so user friendly it's in the --help. There's absolutely no
reason why anybody would want to use GIT_AUTHOR.

But lets agree to disagree.

>> But listen closely to what you said:
>>
>> > I actually think it would make more sense to drop the prompt entirely and 
>> > just die when the user has not given us a usable ident.
>>
>> Suppose somebody has a full name, and a fully qualified domain name,
>> and he can receive mails to it directly. Such a user would not need a
>> git configuration, and would not need $EMAIL, or anything.
>>
>> Currently 'git send-email' will throw 'Felipe Contreras
>> ' which would actually work, but is not explicit.
>>
>> You are suggesting to break that use-case. You are introducing a
>> regression. And this case is realistic, unlike the
>> GIT_AUTHOR_NAME/EMAIL. Isn't it?
>
> Yes, dying would be a regression, in that you would have to configure
> your name via the environment and re-run rather than type it at the
> prompt. You raise a good point that for people who _could_ take the
> implicit default, hitting "enter" is working fine now, and we would lose
> that.  I'd be fine with also just continuing to prompt in the implicit
> case.
>
> But that is a much smaller issue to me than having send-email fail to
> respect environment variables and silently use user.*, which is what
> started this whole discussion. And I agree it is worth considering as a
> regression we should avoid.

It might be smaller,

[PATCH] gitk - fix a problem with multiline author names

2012-11-13 Thread Tomo Krajina
When commiting with "git-commit" no newline in the author string
is possible. But other git clients don't have the same validations
for the author name. And, it is possible to have a commit like:

commit 
Merge: a b
Author: User Name
 
Date:   Thu Nov 8 17:01:02 2012 +0100

Merge branch 'master' of ...

Note that the "Author:" string is split in two lines.

The git-log command work without problems with a commit like this, but
in gitk there is a problem because it splits the headers by a newline
character and that's why the email and time is not correctly parsed
and the history tree below this commit is not shown.

Signed-off-by: Tomo Krajina 
---
 gitk-git/gitk |   16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 6f24f53..87300db 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1672,13 +1672,15 @@ proc parsecommit {id contents listed} {
 foreach line [split $header "\n"] {
  set line [split $line " "]
  set tag [lindex $line 0]
- if {$tag == "author"} {
-set audate [lrange $line end-1 end]
-set auname [join [lrange $line 1 end-2] " "]
- } elseif {$tag == "committer"} {
-set comdate [lrange $line end-1 end]
-set comname [join [lrange $line 1 end-2] " "]
- }
+if {$tag == "author"} {
+regexp -lineanchor {\nauthor([^<]*)<([^>]*)>\s+([^\n]+)}
$header all auname email audate
+set auname [string trim $auname]
+set auname "$auname <$email>"
+} elseif {$tag == "committer"} {
+regexp -lineanchor
{\ncommitter([^<]*)<([^>]*)>\s+([^\n]+)} $header all comname email
comdate
+set comname [string trim $comname]
+set comname "$comname <$email>"
+}
 }
 set headline {}
 # take the first non-blank line of the comment as the headline
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread Marc Khouzam
Thanks for the review.

On Tue, Nov 13, 2012 at 6:14 AM, SZEDER Gábor  wrote:
> Hi,
>
> On Mon, Nov 12, 2012 at 03:07:46PM -0500, Marc Khouzam wrote:
>> Hi,
>
> [...]
>
>> Signed-off-by: Marc Khouzam 
>
> [...]
>
>> 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
>
> Please have a look at Documentation/SubmittingPatches to see how to
> properly format the commit message, i.e. no greeting and sign-off in
> the commit message part, and the S-o-b line should be the last before
> the '---'.

Sorry about that, since I should have noticed it in the doc.
I will do my best to address all submission issues mentioned
when I post the next version of the patch.

> Your patch seems to be severely line-wrapped.  That document also
> contains a few MUA-specific tips to help avoid that.
>
> Other than that, it's a good description of the changes and
> considerations.  I agree that this approach seems to be the best from
> the three.
>
>> 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
>
> See comment near the end.

Great suggestion below.  I removed the above change.

>> +   # Set COMP_WORDS to the command-line as bash would.
>> +   COMP_WORDS=($1)
>
> That comment is only true for older Bash versions.  Since v4 Bash
> splits the command line at characters that the readline library treats
> as word separators when performing word completion.  But the
> completion script has functions to deal with both, so this shouldn't
> be a problem.

I've updated the comment to be more general and left the code
the same since it is supported by the script.

>
>> +   # Print the result that is stored in the bash variable ${COMPREPLY}
>
> Really? ;)

Removed :)

>> +   for i in ${COMPREPLY[@]}; do
>> +   echo "$i"
>> +   done
>
> There is no need for the loop here to print the array one element per
> line:
>
> local IFS=$'\n'
> echo "${COMPREPLY[*]}"

Better.  Thanks.

>> +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"
>
> Where does the second argument come from?  Below you run this script
> as '${__git_tcsh_completion_script} "${COMMAND_LINE}"', i.e. $2 is
> never set.  Am I missing something?

This second argument is optional and, if present, will be put in
$COMP_CWORD.  If not present, $COMP_CWORD must be computed
from $1.  Also see comment above _git_complete_with_output ().
tcsh does not provide me with this information, so I cannot make use of it.
However, I thought it would be more future-proof to allow it for other shells
which may have that information.

It is not necessary for tcsh, so I can remove if you prefer?

>> +# Make the script executable if it is not
>> +if ( ! -x ${__git_tcsh_completion_script} ) then
>> +   chmod u+x ${__git_tcsh_completion_script}
>> +endif
>
> Not sure about this.  If I source a script to provide completion for a
> command, then I definitely don't expect it to change file permissions.
>
> However, even if the git completion script is not executable, you can
> still run it with 'bash ${__git_tcsh_completion_script}'.  This way
> neither the user would need to set permissions, not the script would
> need to set it behind the users back.  Furthermore, this would also
> make changing the shebang line unnecessary.

Very nice!  Done.

>> +complete git  'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
>> | sort | uniq`/'
>> +complete gitk 'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
>> | sort | uniq`/'
>
> Is the 'sort | uniq' really necessary?  After the completion function
> returns Bash automatically sorts the elements in COMPREPLY and removes
> any duplicates.  Doesn't tcsh do the same?  I have no idea about tcsh
> completion.

On my machine, tcsh does not remove duplicates.  It does sort the results
but that is done after I've run 'uniq', which is too late.  I'm not
happy about this
either, but the other option is to improve git-completion.bash to
avoid duplicates,
which seemed less justified.

> Does the git completion script returns any duplicates at all?

It does.  'help' is returned twice for example.
Also, when completing 'git checkout ' in the git repo, I can see multiple
'todo' branches, as well as 'master', 'pu', 'next', etc.

You can actually try it without tcsh by running my proposed version

Re: What's cooking in git.git (Nov 2012, #03; Tue, 13)

2012-11-13 Thread Junio C Hamano
Jeff King  writes:

> This is my final "what's cooking" as interim maintainer. I didn't
> graduate anything to master, but I updated my plans for each topic to
> give Junio an idea of where I was.

After exploding the first-parent history between your master..pu
into component topics and recreating one new merge-fix for
nd/wildmatch topic, I think I now know how to rebuild your
integration branches.

I still haven't caught up with the past discussions (and still am
slightly jetlagged), but I think I can take it over from here with
help from contributors.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH nd/wildmatch] Correct Git's version of isprint and isspace

2012-11-13 Thread Linus Torvalds
On Tue, Nov 13, 2012 at 11:40 AM, Linus Torvalds
 wrote:
>
> I have to wonder why you care? As far as I'm concerned, the only valid
> space is space, TAB and CR/LF.
>
> Anything else is *noise*, not space. What's the reason for even caring?

Btw, expanding the whitespace selection may actually be very
counter-productive. It is used primarily for things like removing
extraneous space at the end of lines etc, and for that, the current
selection of SPACE, TAB and LF/CR is the right thing to do.

Adding things like FF etc - that are *technically* whitespace, but
aren't the normal kind of silent whitespace - is potentially going to
change things too much. People might *want* a form-feed in their
messages, for all we know.

So I really object to changing things "just because". There's a reason
we do our own ctype.c: it avoids the crazy crap. It avoids the idiotic
localization issues, and it avoids the ambiguous cases.

So just let it be, unless you have some major real reason to actually
care about a real-world case. And if you do, please explain it. Don't
change things just because.

   Linus
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH nd/wildmatch] Correct Git's version of isprint and isspace

2012-11-13 Thread Linus Torvalds
On Tue, Nov 13, 2012 at 11:15 AM, René Scharfe
 wrote:
>
> Linus, do you remember if you left them out on purpose?

Umm, no.

I have to wonder why you care? As far as I'm concerned, the only valid
space is space, TAB and CR/LF.

Anything else is *noise*, not space. What's the reason for even caring?

  Linus
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH nd/wildmatch] Correct Git's version of isprint and isspace

2012-11-13 Thread Johannes Sixt
Am 13.11.2012 11:46, schrieb Nguyễn Thái Ngọc Duy:
> @@ -14,11 +14,11 @@ enum {
>   P = GIT_PATHSPEC_MAGIC, /* other non-alnum, except for ] and } */
>   X = GIT_CNTRL,
>   U = GIT_PUNCT,
> - Z = GIT_CNTRL | GIT_SPACE
> + Z = GIT_CNTRL_SPACE
>  };
>  
> -const unsigned char sane_ctype[256] = {
> - X, X, X, X, X, X, X, X, X, Z, Z, X, X, Z, X, X, /*   0.. 15 */
> +const unsigned int sane_ctype[256] = {
> + X, X, X, X, X, X, X, X, X, Z, Z, Z, Z, Z, X, X, /*   0.. 15 */
>   X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, /*  16.. 31 */
>   S, P, P, P, R, P, P, P, R, R, G, R, P, P, R, P, /*  32.. 47 */
>   D, D, D, D, D, D, D, D, D, D, P, P, P, P, P, G, /*  48.. 63 */
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 02f48f6..4ed3f94 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -474,8 +474,8 @@ extern const char tolower_trans_tbl[256];
>  #undef ispunct
>  #undef isxdigit
>  #undef isprint
> -extern const unsigned char sane_ctype[256];
> -#define GIT_SPACE 0x01
> +extern const unsigned int sane_ctype[256];
> +#define GIT_CNTRL_SPACE 0x01
>  #define GIT_DIGIT 0x02
>  #define GIT_ALPHA 0x04
>  #define GIT_GLOB_SPECIAL 0x08
> @@ -483,9 +483,10 @@ extern const unsigned char sane_ctype[256];
>  #define GIT_PATHSPEC_MAGIC 0x20
>  #define GIT_CNTRL 0x40
>  #define GIT_PUNCT 0x80
> -#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
> +#define GIT_SPACE 0x100
> +#define sane_istest(x,mask) ((sane_ctype[(unsigned int)(x)] & (mask)) != 0)
>  #define isascii(x) (((x) & ~0x7f) == 0)
> -#define isspace(x) sane_istest(x,GIT_SPACE)
> +#define isspace(x) sane_istest(x,GIT_SPACE | GIT_CNTRL_SPACE)
>  #define isdigit(x) sane_istest(x,GIT_DIGIT)
>  #define isalpha(x) sane_istest(x,GIT_ALPHA)
>  #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
> @@ -493,7 +494,7 @@ extern const unsigned char sane_ctype[256];
>  #define isupper(x) sane_iscase(x, 0)
>  #define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)
>  #define is_regex_special(x) sane_istest(x,GIT_GLOB_SPECIAL | 
> GIT_REGEX_SPECIAL)
> -#define iscntrl(x) (sane_istest(x,GIT_CNTRL))
> +#define iscntrl(x) (sane_istest(x,GIT_CNTRL | GIT_CNTRL_SPACE))
>  #define ispunct(x) sane_istest(x, GIT_PUNCT | GIT_REGEX_SPECIAL | \
>   GIT_GLOB_SPECIAL | GIT_PATHSPEC_MAGIC)
>  #define isxdigit(x) (hexval_table[x] != -1)

So we have two properties that overlap:

  SS
   

You seem to generate partions:

   XXXYZ

then assign individual bits to each partition. Now each entry in the
lookup table has only one bit set. Then you define isxxx() to check for
one of the two possible bits:

   iscntrl is X or Y
   isspace is Y or Z

But shouldn't you just assign one bit for S and another one for C, have
entries in the lookup table with more than one bit set, and check for
only one bit in the isxxx macro?

That way you don't run out of bits as easily as you do with this patch.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] send-email: stop asking when we have an ident

2012-11-13 Thread Felipe Contreras
From: Felipe Contreras 2nd 

Currently we keep getting questions even when the user has properly
configured his full name and password:

 Who should the emails appear to be from?
 [Felipe Contreras ]

And once a question pops up, other questions are turned on. This is
annoying.

The reason this is safe is because currently the script fails completely
when the autohor (or committer) is not correct, so we won't even be
reaching this point in the code.

The scenarios, and the current situation:

1) No information at all, no fully qualified domain name

fatal: empty ident name (for ) not allowed

2) Only full name

fatal: unable to auto-detect email address (got 'felipec@nysa.(none)')

3) Full name + fqdm

Who should the emails appear to be from?
[Felipe Contreras ]

4) Full name + EMAIL

Who should the emails appear to be from?
[Felipe Contreras ]

5) User configured
6) GIT_COMMITTER
7) GIT_AUTHOR

All these are the same as 4)

After this patch:

1) 2) won't change: git send-email would still die

4) 5) 6) 7) will change: git send-email won't ask the user

This is good, that's what we would expect, because the identity is
explicit.

3) will change: git send-email won't ask the user

This is bad, because we will try with an address such as
'feli...@nysa.felipec.org', which is most likely not what the user
wants, but the user will get warned by default, and if not, most likley
the sending won't work, which the user can easily fix.

The worst possible scenario is that such a mail address does work, and
the user sends an email from that addres unintentionally, when in fact
the user expected to correct that address in the propmpt.

This is a very, very, very unlikely scenario, with many dependencies:

1) No configured user.name/user.email
2) No specified $EMAIL
3) No configured sendemail.from
4) No specified --from argument
5) A fully qualified domain name
6) A full name in the geckos field
7) A sendmail configuration that allows sending from this domain name
8) confirm=never, or
8.1) confirm configuration not hitting, or
8.2) Getting the error, not being aware of it
9) The user expecting to correct this address in the prompt

In a more likely scenario where 7) is not the case (can't send from
nysa.felipec.org), the user will simply see the mail was not sent
properly, and fix the problem.

The much more likely scenario though, is where 5) is not the case
(nysa.(none)), and git send-email will fail right away like it does now.

So the likelyhood of this affecting anybody seriously is very very slim,
and the chances of this affecting somebody slightly are still very
small. The vast majority, if not all, of git users won't be affected
negatively, and a lot will benefit from this.

Signed-off-by: Felipe Contreras 
---
 git-send-email.perl | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index aea66a0..503e551 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -748,16 +748,11 @@ if (!$force) {
}
 }
 
-my $prompting = 0;
 if (!defined $sender) {
$sender = $repoauthor || $repocommitter || '';
-   $sender = ask("Who should the emails appear to be from? [$sender] ",
- default => $sender,
- valid_re => qr/\@.*\./, confirm_only => 1);
-   print "Emails will be sent from: ", $sender, "\n";
-   $prompting++;
 }
 
+my $prompting = 0;
 if (!@initial_to && !defined $to_cmd) {
my $to = ask("Who should the emails be sent to (if any)? ",
 default => "",
-- 
1.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH nd/wildmatch] Correct Git's version of isprint and isspace

2012-11-13 Thread René Scharfe

Am 13.11.2012 11:46, schrieb Nguyễn Thái Ngọc Duy:

Git's ispace does not include 11 and 12.  [...]

> According to glibc-2.14.1 on C locale on Linux, this is wrong.

11 and 12 being vertical tab (\v) and form-feed (\f).  This lack goes 
back to the introduction of git's own character classifier macros seven 
years ago in 4546738b (Unlocalized isspace and friends).


Linus, do you remember if you left them out on purpose?

Thanks,
René

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH nd/wildmatch] Correct Git's version of isprint and isspace

2012-11-13 Thread René Scharfe

Am 13.11.2012 11:46, schrieb Nguyễn Thái Ngọc Duy:

Git's isprint includes
control space characters (10-13). According to glibc-2.14.1 on C
locale on Linux, this is wrong. This patch fixes it.


isprint() is not in master, yet.  Can we perhaps still introduce it in 
such a way that we never have an incorrect version in master's history?


And could you please update test-ctype.c to match the change to 
isspace()?  The tests there just documented the status quo before I made 
changes to ctype.c long ago, so it's definitions are just as correct (or 
wrong) as the original implementation.


Thanks,
René

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH nd/wildmatch] Correct Git's version of isprint and isspace

2012-11-13 Thread Jan H. Schönherr
Hi.

Am 13.11.2012 11:46, schrieb Nguyễn Thái Ngọc Duy:
> Git's ispace does not include 11 and 12. Git's isprint includes
> control space characters (10-13). According to glibc-2.14.1 on C
> locale on Linux, this is wrong. This patch fixes it.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  I wrote a small C program to compare the result of all is* functions
>  that Git replaces against the libc version. These are the only ones that
>  differ. Which matches what Jan Schönherr commented.
> 
>  ctype.c   |  6 +++---
>  git-compat-util.h | 11 ++-
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/ctype.c b/ctype.c
> index 0bfebb4..71311a3 100644
> --- a/ctype.c
> +++ b/ctype.c
> @@ -14,11 +14,11 @@ enum {
>   P = GIT_PATHSPEC_MAGIC, /* other non-alnum, except for ] and } */
>   X = GIT_CNTRL,
>   U = GIT_PUNCT,
> - Z = GIT_CNTRL | GIT_SPACE
> + Z = GIT_CNTRL_SPACE
>  };
>  
> -const unsigned char sane_ctype[256] = {
> - X, X, X, X, X, X, X, X, X, Z, Z, X, X, Z, X, X, /*   0.. 15 */
> +const unsigned int sane_ctype[256] = {
> + X, X, X, X, X, X, X, X, X, Z, Z, Z, Z, Z, X, X, /*   0.. 15 */
>   X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, /*  16.. 31 */
>   S, P, P, P, R, P, P, P, R, R, G, R, P, P, R, P, /*  32.. 47 */
>   D, D, D, D, D, D, D, D, D, D, P, P, P, P, P, G, /*  48.. 63 */

An alternative to switching from 1-byte to 4-byte values (don't we have
a 2-byte datatype?), would be to free up GIT_CNTRL and simply do:

#define iscntrl(x) ((x) < 0x20)


> diff --git a/git-compat-util.h b/git-compat-util.h
> index 02f48f6..4ed3f94 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
[...]
> @@ -483,9 +483,10 @@ extern const unsigned char sane_ctype[256];
>  #define GIT_PATHSPEC_MAGIC 0x20
>  #define GIT_CNTRL 0x40
>  #define GIT_PUNCT 0x80
> -#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
> +#define GIT_SPACE 0x100
> +#define sane_istest(x,mask) ((sane_ctype[(unsigned int)(x)] & (mask)) != 0)

That should better be left "(unsigned char)"? We might access values after the
array otherwise.

(That said, it wasn't really correct before either, when there really is a
possibility that x >= 0x100.)

Regards
Jan

PS: It looks like my isprint() version was given precedence over your
isprint() version during the merge into next. That should also be sorted out,
but I've no idea which one is actually better: two comparisons versus one
cache lookup and a bitop... (though my guess is that comparisons are cheaper,
but then we should also convert isdigit()...)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [regression] Newer gits cannot clone any remote repos

2012-11-13 Thread Ramsay Jones
Douglas Mencken wrote:
> *Any* git clone fails with:
> 
> fatal: premature end of pack file, 106 bytes missing
> fatal: index-pack failed
> 
> At first, I tried 1.8.0, and it failed. Then I tried to build 1.7.10.5
> then, and it worked. Then I tried 1.7.12.2, but it fails the same way
> as 1.8.0.
> So I decided to git bisect.
> 
> b8a2486f1524947f232f657e9f2ebf44e3e7a243 is the first bad commit
> ``index-pack: support multithreaded delta resolving''

This looks like the same problem I had on cygwin, which lead to
commit c0f86547c ("index-pack: Disable threading on cygwin", 26-06-2012).

I didn't notice which platform you are on, but maybe you also have a
thread-unsafe pread()? Could you try re-building git with the
NO_THREAD_SAFE_PREAD build variable set?

HTH.

ATB,
Ramsay Jones

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: What's cooking in git.git (Oct 2012, #09; Mon, 29)

2012-11-13 Thread Ramsay Jones
Jeff King wrote:
> On Sat, Nov 10, 2012 at 06:33:38PM +, Ramsay Jones wrote:
> 
>>> We should probably wrap it. I'm planning to queue this on top of Chris's
>>> patch:
>>
>> Unfortunately, I haven't had time yet to test this patch. (Early this week, I
>> went into hospital for a "minor" surgical procedure - I have not yet fully
>> recovered). The patch looks good and I don't anticipate any problems. I will
>> hopefully test it soon (I see it's in pu now) and let you know.
> 
> Thanks. I merged it to "next" on Friday, so we will see if anybody
> screams. But please take your time and recover. Git will wait. :)

I have tested this patch and, as expected, it fixes things up for me.
(To be more precise: the current next branch works, but if I revert
commit c2b3af05, then t9604-cvsimport-timestamps.sh fails in the
same way as before!)

Thanks!

ATB,
Ramsay Jones


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: checkout from neighbour branch undeletes a path?

2012-11-13 Thread Peter Vereshagin
Hello.

2012/11/13 08:43:31 -0800 Junio C Hamano  => To Peter 
Vereshagin :
JCH> Peter Vereshagin  writes:
JCH> 
JCH> > Am wondering if 'checkout branch path' undeletes the files?
JCH> 
JCH> "git checkout branch path" (by the way, "branch" does not have to be
JCH> a branch name; any commit object name would do, like "git checkout
JCH> HEAD^^ hello.c") is a way to check out named path(s) out of the
JCH> named commit.
JCH> 
JCH> If the commit "branch" has "path" in it, its contents are checked
JCH> out to "path" in your current working tree (and the entry in the
JCH> index updated to match it).
JCH> 
JCH> If you happen to have removed "path" in your current working tree
JCH> before running that command, it might look as if there is some
JCH> undelete going on, but that is a wrong way to look at things.  The
JCH> version of "path" in the "branch" may or may not be similar to what
JCH> you have removed earlier.
JCH> 


Hello.

I solved my problem by mean of 'git rm' instead of 'rm'.

I knew what you said here. Shortly, the difference for my case was:

 - I check out the pathdir from the commit in which the pathdir/file00 was
   already removed.

 - The current branch 'branch01' has no idea the file00 was removed. But I
   removed file00 and this is what 'branch01' assumes when I commit n ext time.

But git assumes I need 'file00' although it doesn't exist in the commit I
checkout path from. It doesn't exist by itself before I checkout that path
neither.

How can I know which one of the 'file00' versions is being checked out: the one
that did exist in the 'branch00' (the where I checkout path from) before I
removed it or the one existing in HEAD but not in the work-tree? And why this
and not that?

If it is the one that exists in a current branch but was deleted from trhe
work-tree [d4f7c70] than why it is being checked out not from the commit
supplied as an argument to git?

If it is the one that existed [c3e78ff] before the commit I checkout path from
than why it is being checked out while it doesn't exist in that commit already?

Thank you.

--
Peter Vereshagin  (http://vereshagin.org) pgp: A0E26627 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread Felipe Contreras
On Mon, Nov 12, 2012 at 9:07 PM, Marc Khouzam  wrote:

> 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.

Maybe you can try to take a look at the same for zsh:
http://article.gmane.org/gmane.comp.version-control.git/208173

> ---
>  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]}

You might want to use __${COMP_WORDS[0]}_main instead.

> +
> +   # 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

Why do you need this function in this file? You can very easily add
this function to git-completion.tcsh.

> diff --git a/contrib/completion/git-completion.tcsh
> b/contrib/completion/git-completion.tcsh
> new file mode 100755
> index 000..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 
> +# 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

Why not just source it?

> +complete git  'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
> | sort | uni

Re: RFD: fast-import is picky with author names (and maybe it should - but how much so?)

2012-11-13 Thread Felipe Contreras
On Tue, Nov 13, 2012 at 11:15 AM, Michael J Gruber
 wrote:
> Felipe Contreras venit, vidit, dixit 12.11.2012 23:47:
>> On Mon, Nov 12, 2012 at 10:41 PM, Jeff King  wrote:
>>> 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  King"
>>>   email=""
>>>
>>> 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).
>>
>> Right. Unfortunately I'm not aware of any DSCM that does that.
>>
>>> 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.
>>
>> CVS might be the only one that has such structured data. I think in
>> subversion the username has no meaning. A 'felipec' subversion
>> username is as bad as a mercurial 'felipec' username.
>
> In subversion, the username has the clearly defined meaning of being a
> username on the subversion host. If the host is, e.g., a sourceforge
> site then I can easily look up the user profile and convert the username
> into a valid e-mail address (@users.sf.net). That is the
> advantage that the exporter (together with user knowledge) has over the
> importer.
>
> If the initial clone process aborts after every single "unknown" user
> it's no fun, of course. On the other hand, if an incremental clone
> (fetch) let's commits with unknown author sneak in it's no fun either
> (because I may want to fetch in crontab and publish that converted beast
> automatically). That is why I proposed neither approach.
>
> Most conveniently, the export side of a remote helper would
>
> - do "obvious" automatic lossless transformations
> - use an author map for other names

This should be done by fast-import. It doesn't make any sense that
every remote helper and fast-exporter out there have their own way of
mapping authors (or none).

> - For names not covered by the above (or having an empty map entry):
> Stop exporting commits but continue parsing commits and amend the author
> map with any unknown usernames (empty entry), and warn the user.
> (crontab script can notify me based on the return code.)

Stop exporting commits but continue parsing commits? I don't know what
that means.

fast-import should try it's best to clean it up, warn the user, sure,
but also store the missing entry on a file, so that it can be filed
later (if the user so wishes).

> If the cloning involves a "foreign clone" (like the hg clone behind the
> scene) then the runtime of the second pass should be much smaller. In
> principle, one could even store all blobs and trees on the first run and
> skip that step on the second, but that would rely on immutability on the
> foreign side, so I dunno. (And to check the sha1, we have to get the
> blob anyways.)

No. There's no concept of partial clones... Either you clone, or you don't.

Wait if the remote helper didn't notice that the author was bad?
fast-import could just just leave everything up to that point, warn
abut what happened, and exit, but the exporter side would die in the
middle of exporting, and it might end up in a bad state, not saving
marks, or who knows what.

It wouldn't work.

The cloning should be full, and the bad authors stored in a scaffold author map.

> As for the format for incomplete entries (foo ), a technical
> guideline should suffice for those that follow guidelines.

fast-import should do that.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Notes in format-patch

2012-11-13 Thread Junio C Hamano
Junio C Hamano  writes:

> ... and it is broken X-<.
>
> The blank line should be added before the diffstat, not after the
> notes message (t3307 shows a case where we give notes without
> diffstat, and we shouldn't be adding an extra blank line in that
> case.

Second try.

-- >8 --
Subject: format-patch: add a blank line between notes and diffstat

The last line of the note text comes immediately before the diffstat
block, making the latter unnecessarily harder to view.

Signed-off-by: Junio C Hamano 
---

 log-tree.c | 31 +--
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git c/log-tree.c w/log-tree.c
index 712a22b..4f86def 100644
--- c/log-tree.c
+++ w/log-tree.c
@@ -727,15 +727,16 @@ int log_tree_diff_flush(struct rev_info *opt)
}
 
if (opt->loginfo && !opt->no_commit_id) {
-   /* When showing a verbose header (i.e. log message),
-* and not in --pretty=oneline format, we would want
-* an extra newline between the end of log and the
-* output for readability.
-*/
show_log(opt);
if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
opt->verbose_header &&
opt->commit_format != CMIT_FMT_ONELINE) {
+   /*
+* When showing a verbose header (i.e. log message),
+* and not in --pretty=oneline format, we would want
+* an extra newline between the end of log and the
+* diff/diffstat output for readability.
+*/
int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
if (opt->diffopt.output_prefix) {
struct strbuf *msg = NULL;
@@ -743,11 +744,21 @@ int log_tree_diff_flush(struct rev_info *opt)
opt->diffopt.output_prefix_data);
fwrite(msg->buf, msg->len, 1, stdout);
}
-   if (!opt->shown_dashes) {
-   if ((pch & opt->diffopt.output_format) == pch)
-   printf("---");
-   putchar('\n');
-   }
+
+   /*
+* We may have shown three-dashes line early
+* between notes and the log message, in which
+* case we only want a blank line after the
+* notes without (an extra) three-dashes line.
+* Otherwise, we show the three-dashes line if
+* we are showing the patch with diffstat, but
+* in that case, there is no extra blank line
+* after the three-dashes line.
+*/
+   if (!opt->shown_dashes &&
+   (pch & opt->diffopt.output_format) == pch)
+   printf("---");
+   putchar('\n');
}
}
diff_flush(&opt->diffopt);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 15/13] compat/fnmatch: fix off-by-one character class's length check

2012-11-13 Thread Johannes Sixt
Am 11.11.2012 11:13, schrieb Nguyễn Thái Ngọc Duy:
> - if (c1 == CHAR_CLASS_MAX_LENGTH)
> + if (c1 > CHAR_CLASS_MAX_LENGTH)

Nice catch! With this one and 14/13, all tests in t3070 pass on Windows.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 14/13] test-wildmatch: avoid Windows path mangling

2012-11-13 Thread Johannes Sixt
Am 13.11.2012 11:06, schrieb Nguyễn Thái Ngọc Duy:
> On Windows, arguments starting with a forward slash is mangled as if
> it were full pathname. This causes the patterns beginning with a slash
> not to be passed to test-wildmatch correctly. Avoid mangling by never
> accepting patterns starting with a slash. Those arguments must be
> rewritten with a leading "XXX" (e.g. "/abc" becomes "XXX/abc"), which
> will be removed by test-wildmatch itself before feeding the patterns
> to wildmatch() or fnmatch().
> 
> Reported-by: Johannes Sixt 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  On Sun, Nov 11, 2012 at 5:47 PM, Junio C Hamano  wrote:
>  > The title taken together with the above explanation makes it sound
>  > as if wildmatch code does not work with the pattern /foo on Windows
>  > at all and to avoid the issue (instead of fixing the breakage) this
>  > patch removes such tests
> 
>  OK how about this?

Thanks, the patch lets the tests pass on Windows.

-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


What's cooking in git.git (Nov 2012, #03; Tue, 13)

2012-11-13 Thread Jeff King
What's cooking in git.git (Nov 2012, #03; Tue, 13)
--

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

This is my final "what's cooking" as interim maintainer. I didn't
graduate anything to master, but I updated my plans for each topic to
give Junio an idea of where I was.

You can find the changes described here in the integration branches of
my repository at:

  git://github.com/peff/git.git

Until Junio returns, kernel.org and the other "usual" places will not be
updated.

--
[New Topics]

* jk/maint-gitweb-xss (2012-11-12) 1 commit
 - gitweb: escape html in rss title

 Fixes an XSS vulnerability in gitweb.

 Will merge to 'next'.


* jk/send-email-sender-prompt (2012-11-13) 6 commits
 - send-email: do not prompt for explicit repo ident
 - Git.pm: teach "ident" to query explicitness
 - var: provide explicit/implicit ident information
 - var: accept multiple variables on the command line
 - ident: keep separate "explicit" flags for author and committer
 - ident: make user_ident_explicitly_given private

 Avoid annoying sender prompt in git-send-email, but only when it is
 safe to do so.

 Needs review.


* mg/replace-resolve-delete (2012-11-13) 1 commit
 - replace: parse revision argument for -d

 Be more user friendly to people using "git replace -d".

 Will merge to 'next'.


* ml/cygwin-mingw-headers (2012-11-12) 1 commit
 - Update cygwin.c for new mingw-64 win32 api headers

 Make git work on newer cygwin.

 Will merge to 'next'.

--
[Stalled]

* rc/maint-complete-git-p4 (2012-09-24) 1 commit
  (merged to 'next' on 2012-10-29 at af52cef)
 + Teach git-completion about git p4

 Comment from Pete will need to be addressed in a follow-up patch.


* as/test-tweaks (2012-09-20) 7 commits
 - tests: paint unexpectedly fixed known breakages in bold red
 - tests: test the test framework more thoroughly
 - [SQUASH] t/t-basic.sh: quoting of TEST_DIRECTORY is screwed up
 - tests: refactor mechanics of testing in a sub test-lib
 - tests: paint skipped tests in bold blue
 - tests: test number comes first in 'not ok $count - $message'
 - tests: paint known breakages in bold yellow

 Various minor tweaks to the test framework to paint its output
 lines in colors that match what they mean better.

 Has the "is this really blue?" issue Peff raised resolved???


* jc/maint-name-rev (2012-09-17) 7 commits
 - describe --contains: use "name-rev --algorithm=weight"
 - name-rev --algorithm=weight: tests and documentation
 - name-rev --algorithm=weight: cache the computed weight in notes
 - name-rev --algorithm=weight: trivial optimization
 - name-rev: --algorithm option
 - name_rev: clarify the logic to assign a new tip-name to a commit
 - name-rev: lose unnecessary typedef

 "git name-rev" names the given revision based on a ref that can be
 reached in the smallest number of steps from the rev, but that is
 not useful when the caller wants to know which tag is the oldest one
 that contains the rev.  This teaches a new mode to the command that
 uses the oldest ref among those which contain the rev.

 I am not sure if this is worth it; for one thing, even with the help
 from notes-cache, it seems to make the "describe --contains" even
 slower. Also the command will be unusably slow for a user who does
 not have a write access (hence unable to create or update the
 notes-cache).

 Stalled mostly due to lack of responses.


* jc/xprm-generation (2012-09-14) 1 commit
 - test-generation: compute generation numbers and clock skews

 A toy to analyze how bad the clock skews are in histories of real
 world projects.

 Stalled mostly due to lack of responses.


* jc/blame-no-follow (2012-09-21) 2 commits
 - blame: pay attention to --no-follow
 - diff: accept --no-follow option

 Teaches "--no-follow" option to "git blame" to disable its
 whole-file rename detection.

 Stalled mostly due to lack of responses.


* jc/doc-default-format (2012-10-07) 2 commits
 - [SQAUSH] allow "cd Doc* && make DEFAULT_DOC_TARGET=..."
 - Allow generating a non-default set of documentation

 Need to address the installation half if this is to be any useful.


* mk/maint-graph-infinity-loop (2012-09-25) 1 commit
 - graph.c: infinite loop in git whatchanged --graph -m

 The --graph code fell into infinite loop when asked to do what the
 code did not expect ;-)

 Anybody who worked on "--graph" wants to comment?
 Stalled mostly due to lack of responses.


* jc/add-delete-default (2012-08-13) 1 commit
 - git add: notice removal of tracked paths by default

 "git add dir/" updated modified files and added new files, but does
 not notice removed files, which may be "Huh?" to some users.  They
 can of course use "git add -A dir/", but why should they?

 Resurrected from graveyard, as I thought it was a worthwhil

Re: Notes in format-patch

2012-11-13 Thread Junio C Hamano
Junio C Hamano  writes:

> As the topic seems to be already in Peff's next, here is a trivial
> fix for this in incremental form.
>
> -- >8 --
> Subject: format-patch: add a blank line between notes and diffstat
>
> The last line of the note text comes immediately before the diffstat
> block, making the latter unnecessarily harder to view.
>
> Signed-off-by: Junio C Hamano 
> ---
>  log-tree.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git i/log-tree.c w/log-tree.c
> index 712a22b..9303fd8 100644
> --- i/log-tree.c
> +++ w/log-tree.c
> @@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
>   opt->shown_dashes = 1;
>   }
>   strbuf_addstr(&msgbuf, ctx.notes_message);
> + strbuf_addch(&msgbuf, '\n');
>   }
>  
>   if (opt->show_log_size) {

... and it is broken X-<.

The blank line should be added before the diffstat, not after the
notes message (t3307 shows a case where we give notes without
diffstat, and we shouldn't be adding an extra blank line in that
case.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 5/6] Git.pm: teach "ident" to query explicitness

2012-11-13 Thread Jeff King
On Tue, Nov 13, 2012 at 09:23:00AM -0800, Matt Kraai wrote:

> Minor nits:
> 
> On Tue, Nov 13, 2012 at 11:53:20AM -0500, Jeff King wrote:
> > @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an 
> > array with the fields parsed.
> >  Alternatively, it can take a prepared ident string (e.g. from the commit
> >  object) and just parse it.
> >  
> > +If the C option is set to 1, the returned array will contain an
> > +additional boolean specifying whether the ident was configure explicitly 
> > by the
> 
> s/configure/configured/

Thanks.

> > if (wantarray) {
> > -   return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
> > +   my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
> > +   if ($options{explicit} && defined $explicit) {
> > +   push @ret, $explicit if defined $explicit;
> 
> Isn't the test on this line redundant given that defined $explicit is
> already guaranteed by the condition on the previous line?

Yes, thanks (left over from an earlier attempt that tried to avoid
having $options{explicit}).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] send-email: add proper default sender

2012-11-13 Thread Junio C Hamano
Jeff King  writes:

> On Tue, Nov 13, 2012 at 08:13:04AM -0800, Junio C Hamano wrote:
>
>> >> That's right, AUTHOR_IDENT would fall back to the default email and full 
>> >> name.
>> >
>> > Yeah, I find that somewhat questionable in the current behavior, and I'd
>> > consider it a bug. Typically we prefer the committer ident when given a
>> > choice (e.g., for writing reflog entries).
>> 
>> Just to make sure I follow the discussion correctly, do you mean
>> that the bug is that we pick a fuzzy AUTHOR when COMMITTER is
>> specified more concretely and we usually use COMMIITTER for this
>> kind of thing in the first place but send-email does not in this
>> case (I do not see "git var GIT_AUTHOR_IDENT" returning value from
>> the implicit logic as a bug in this case---just making sure).
>
> Having discussed more, I think there are two questionable things:
>
>   1. Preferring author over committer
>
>   2. Failing to fall back to committer when author is implicit or bogus
>  (because "git var" dies).
>
> I think (1) may fall into the "this is not how I would do it today, but
> it is not worth a possible regression" category.
>
> I think (2) might be worth fixing, though. Certainly when the author is
> bogus (by IDENT_STRICT rules), which I think was the original intent of
> the "$repoauthor || $repocommitter" code. Probably when the author ident
> is implicit, though that is more hazy to me.

I agree with both points.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] gitweb: XSS vulnerability of RSS feed

2012-11-13 Thread Jakub Narębski
On Tue, Nov 13, 2012 at 6:04 PM, Jeff King  wrote:
> On Tue, Nov 13, 2012 at 09:44:06AM -0500, Drew Northup wrote:

>> Besides, inserting one call to esc_html only fixes one attack path. I
>> didn't look to see if all others were already covered.
>
> Properly quoting output is something that the web framework should do
> for you. gitweb uses CGI.pm, which does help with that, but we do not
> use it consistently. If there are other problematic areas, I think the
> best path forward is to use our framework more.

Well, calling CGI.pm a _framework_ is overly generous, but it does
include some HTML generation subroutines / methods, and gitweb
makes use of them, especially $cgi->a() for links.

But it cannot help in this case, because here we are generating XML:
RSS or Atom feed. There was proposal some time ago to switch
to using XML::FeedPP or XML::Atom::Feed + XML::RSS::Feed for
feed generation.

Perhaps it is high time to switch to some Perl web (micro)framework,
like Dancer, Mojolicious or Catalyst... but not requiring extra modules
has its advantages (and there always exist Gitalist).
-- 
Jakub Narebski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] send-email: add proper default sender

2012-11-13 Thread Erik Faye-Lund
On Tue, Nov 13, 2012 at 12:35 AM, Jeff King  wrote:
> On Sun, Nov 11, 2012 at 06:06:50PM +0100, Felipe Contreras wrote:
>
>> There's no point in asking this over and over if the user already
>> properly configured his/her name and email.
>>
>> Signed-off-by: Felipe Contreras 
>> ---
>>
>> I got really tired of 'git send-email' always asking me from which address 
>> to send mails... that's already configured.
>
> It should be defaulting to your regular git ident, and you just have to
> hit enter, right?
>
> I think it's probably reasonable to skip that "enter" in most cases. But
> I'm not sure why we ever asked in the first place. What do people input
> there if they are not taking the default?
>

I usually input "Linus Torvalds ", to
give my patch high priority ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] send-email: add proper default sender

2012-11-13 Thread Jeff King
On Tue, Nov 13, 2012 at 08:13:04AM -0800, Junio C Hamano wrote:

> >> That's right, AUTHOR_IDENT would fall back to the default email and full 
> >> name.
> >
> > Yeah, I find that somewhat questionable in the current behavior, and I'd
> > consider it a bug. Typically we prefer the committer ident when given a
> > choice (e.g., for writing reflog entries).
> 
> Just to make sure I follow the discussion correctly, do you mean
> that the bug is that we pick a fuzzy AUTHOR when COMMITTER is
> specified more concretely and we usually use COMMIITTER for this
> kind of thing in the first place but send-email does not in this
> case (I do not see "git var GIT_AUTHOR_IDENT" returning value from
> the implicit logic as a bug in this case---just making sure).

Having discussed more, I think there are two questionable things:

  1. Preferring author over committer

  2. Failing to fall back to committer when author is implicit or bogus
 (because "git var" dies).

I think (1) may fall into the "this is not how I would do it today, but
it is not worth a possible regression" category.

I think (2) might be worth fixing, though. Certainly when the author is
bogus (by IDENT_STRICT rules), which I think was the original intent of
the "$repoauthor || $repocommitter" code. Probably when the author ident
is implicit, though that is more hazy to me.

> For a change with low benefit/cost ratio like this, the best course
> of action often is to stop paying attention to the thread that has
> become unproductive and venomous, and resurrect the topic after
> people forgot about it and doing it right yourself ;-).

I came to the same conclusion, but decided to simply do it right now
while it was in my head. Hopefully we can progress by reviewing the
series I just posted.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] gitweb: XSS vulnerability of RSS feed

2012-11-13 Thread Jeff King
On Tue, Nov 13, 2012 at 09:44:06AM -0500, Drew Northup wrote:

> I don't buy the argument that we don't need to clean up the input as
> well. There are scant few of us that are going to name a file
> "alert("Something Awful")" in this world (I am
> probably one of them). Input validation is key to keeping problems
> like this from coming up repeatedly as those writing the guts of
> programs are typically more interested in getting the "assigned task"
> done and reporting the output to the user in a safe manner.

Oh, you absolutely do need to clean up the input side. And we do. Notice
how validate_pathname cleans out dots that could allow an attacker to do
a "../../etc/passwd" attack. But the input validation is _different_
than the output escaping. We are turning arbitrary junk from the user
into something we know is safe to treat as a filename. Our goal is
protecting the filesystem and the server, and we do that already.
Protecting the browser on output is a different problem, and happens
only when we are sending to the browser.

As far as "people will not use 

Re: Notes in format-patch

2012-11-13 Thread Junio C Hamano
Junio C Hamano  writes:

> Michael J Gruber  writes:
>
>> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
>>> '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 
>>> ---
>>> 
>>> Notes:
>>> v3 safeguards the hex buffer against reuse
>>>  builtin/replace.c  | 16 ++--
>>>  t/t6050-replace.sh | 11 +++
>>>  2 files changed, 21 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/builtin/replace.c b/builtin/replace.c
>>
>> By the way - Junio, is that the intented outcome of "format-patch
>> --notes"? I would rather put the newline between the note and the
>> diffstat...
>
> I do not mind (actually I personally would prefer to see) a blank
> line between the three-dash and "Notes:", but I agree that we should
> have a blank line before the diffstat block.

As the topic seems to be already in Peff's next, here is a trivial
fix for this in incremental form.

-- >8 --
Subject: format-patch: add a blank line between notes and diffstat

The last line of the note text comes immediately before the diffstat
block, making the latter unnecessarily harder to view.

Signed-off-by: Junio C Hamano 
---
 log-tree.c | 1 +
 1 file changed, 1 insertion(+)

diff --git i/log-tree.c w/log-tree.c
index 712a22b..9303fd8 100644
--- i/log-tree.c
+++ w/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
opt->shown_dashes = 1;
}
strbuf_addstr(&msgbuf, ctx.notes_message);
+   strbuf_addch(&msgbuf, '\n');
}
 
if (opt->show_log_size) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 6/6] send-email: do not prompt for explicit repo ident

2012-11-13 Thread Jeff King
If git-send-email is configured with sendemail.from, we will
not prompt the user for the "From" address of the emails.
If it is not configured, we prompt the user, but provide the
repo author or committer as a default.  Even though we
probably have a sensible value for the default, the prompt
is a safety check in case git generated an incorrect
implicit ident string.

Now that Git.pm will tell us whether the ident is implicit or
explicit, we can stop prompting in the explicit case, saving
most users from having to see the prompt at all.

The test scripts need to be adjusted to not expect a prompt
for the sender, since they always have the author explicitly
defined in the environment. Unfortunately, we cannot
reliably test that prompting still happens in the implicit
case, as send-email will produce inconsistent results
depending on the machine config (if we cannot find a FQDN,
"git var" will barf, causing us to exit early; if we can,
send-email will continue but prompt).

Signed-off-by: Jeff King 
---
 git-send-email.perl   | 22 +-
 t/t9001-send-email.sh |  5 ++---
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..0c49b32 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -436,9 +436,8 @@ if (0) {
}
 }
 
-my ($repoauthor, $repocommitter);
-($repoauthor) = Git::ident_person(@repo, 'author');
-($repocommitter) = Git::ident_person(@repo, 'committer');
+my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');
+my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 
'committer');
 
 # Verify the user input
 
@@ -755,12 +754,17 @@ if (!$force) {
 
 my $prompting = 0;
 if (!defined $sender) {
-   $sender = $repoauthor || $repocommitter || '';
-   $sender = ask("Who should the emails appear to be from? [$sender] ",
- default => $sender,
- valid_re => qr/\@.*\./, confirm_only => 1);
-   print "Emails will be sent from: ", $sender, "\n";
-   $prompting++;
+   ($sender, my $explicit) =
+   defined $repoauthor ? ($repoauthor, $author_explicit) :
+   defined $repocommitter ? ($repocommitter, $committer_explicit) :
+   ('', 0);
+   if (!$explicit) {
+   $sender = ask("Who should the emails appear to be from? 
[$sender] ",
+ default => $sender,
+ valid_re => qr/\@.*\./, confirm_only => 1);
+   print "Emails will be sent from: ", $sender, "\n";
+   $prompting++;
+   }
 }
 
 if (!@initial_to && !defined $to_cmd) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 6c6af7d..c5d66cf 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -191,14 +191,13 @@ test_expect_success $PREREQ 'Show all headers' '
 
 test_expect_success $PREREQ 'Prompting works' '
clean_fake_sendmail &&
-   (echo "Example "
-echo "t...@example.com"
+   (echo "t...@example.com"
 echo ""
) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
--smtp-server="$(pwd)/fake.sendmail" \
$patches \
2>errors &&
-   grep "^From: Example \$" msgtxt1 &&
+   grep "^From: A U Thor \$" msgtxt1 &&
grep "^To: t...@example.com\$" msgtxt1
 '
 
-- 
1.8.0.207.gdf2154c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 5/6] Git.pm: teach "ident" to query explicitness

2012-11-13 Thread Jeff King
"git var" recently learned to report on whether an ident we
fetch from it was configured explicitly or implicitly. Let's
make that information available to callers of the ident
function.

Because evaluating "ident" in an array versus scalar context
already has a meaning, we cannot return our extra value in a
backwards compatible way. Instead, we require the caller to
add an extra "explicit" flag to request the information.
The ident_person function, on the other hand, always returns
a scalar, so we are free to overload it in an array context.

Signed-off-by: Jeff King 
---
 perl/Git.pm | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..1994ec1 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -737,7 +737,7 @@ sub remote_refs {
 }
 
 
-=item ident ( TYPE | IDENTSTR )
+=item ident ( TYPE | IDENTSTR [, options] )
 
 =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
 
@@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array 
with the fields parsed.
 Alternatively, it can take a prepared ident string (e.g. from the commit
 object) and just parse it.
 
+If the C option is set to 1, the returned array will contain an
+additional boolean specifying whether the ident was configure explicitly by the
+user.
+
 C returns the person part of the ident - name and email;
 it can take the same arguments as C or the array returned by C.
 
@@ -763,17 +767,22 @@ The synopsis is like:
 =cut
 
 sub ident {
-   my ($self, $type) = _maybe_self(@_);
-   my $identstr;
+   my ($self, $type, %options) = _maybe_self(@_);
+   my ($identstr, $explicit);
if (lc $type eq lc 'committer' or lc $type eq lc 'author') {
-   my @cmd = ('var', 'GIT_'.uc($type).'_IDENT');
+   my $uc = uc($type);
+   my @cmd = ('var', "GIT_${uc}_IDENT", "GIT_${uc}_EXPLICIT");
unshift @cmd, $self if $self;
-   $identstr = command_oneline(@cmd);
+   ($identstr, $explicit) = command(@cmd);
} else {
$identstr = $type;
}
if (wantarray) {
-   return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+   my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+   if ($options{explicit} && defined $explicit) {
+   push @ret, $explicit if defined $explicit;
+   }
+   return @ret;
} else {
return $identstr;
}
@@ -781,8 +790,11 @@ sub ident {
 
 sub ident_person {
my ($self, @ident) = _maybe_self(@_);
-   $#ident == 0 and @ident = $self ? $self->ident($ident[0]) : 
ident($ident[0]);
-   return "$ident[0] <$ident[1]>";
+   $#ident == 0 and @ident = $self ?
+ $self->ident($ident[0], explicit => 1) :
+ ident($ident[0], explicit => 1);
+   my $ret = "$ident[0] <$ident[1]>";
+   return wantarray ? ($ret, @ident[3]) : $ret;
 }
 
 
-- 
1.8.0.207.gdf2154c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 4/6] var: provide explicit/implicit ident information

2012-11-13 Thread Jeff King
Internally, we keep track of whether the author or committer
ident information was provided by the user, or whether it
was implicitly determined by the system. However, there is
currently no way for external programs or scripts to get
this information without re-implementing the ident logic
themselves. Let's provide a way for them to do so.

Signed-off-by: Jeff King 
---
 Documentation/git-var.txt |  8 
 builtin/var.c | 27 +++
 t/t0007-git-var.sh| 36 
 3 files changed, 71 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 53abba5..963b8d4 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -39,9 +39,17 @@ VARIABLES
 GIT_AUTHOR_IDENT::
 The author of a piece of code.
 
+GIT_AUTHOR_EXPLICIT::
+Outputs "1" if the author identity was provided explicitly by the
+user (in the config or environment), or "0" if it was determined
+implicitly from the system.
+
 GIT_COMMITTER_IDENT::
 The person who put a piece of code into git.
 
+GIT_COMMITTER_EXPLICIT::
+Like GIT_AUTHOR_EXPLICIT, but for the committer ident.
+
 GIT_EDITOR::
 Text editor for use by git commands.  The value is meant to be
 interpreted by the shell when it is used.  Examples: `~/bin/vi`,
diff --git a/builtin/var.c b/builtin/var.c
index 49b48f5..ce28101 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -26,13 +26,40 @@ static const char *pager(int flag)
return pgm;
 }
 
+static const char *explicit_ident(const char * (*get_ident)(int),
+ int (*check_ident)(void))
+{
+   /*
+* Prime the "explicit" flag by getting the identity.
+* We do not want to be strict here, because we would not want
+* to die on bogus implicit values, but instead just report that they
+* are not explicit.
+*/
+   get_ident(0);
+   return check_ident() ? "1" : "0";
+}
+
+static const char *committer_explicit(int flag)
+{
+   return explicit_ident(git_committer_info,
+ committer_ident_sufficiently_given);
+}
+
+static const char *author_explicit(int flag)
+{
+   return explicit_ident(git_author_info,
+ author_ident_sufficiently_given);
+}
+
 struct git_var {
const char *name;
const char *(*read)(int);
 };
 static struct git_var git_vars[] = {
{ "GIT_COMMITTER_IDENT", git_committer_info },
+   { "GIT_COMMITTER_EXPLICIT", committer_explicit },
{ "GIT_AUTHOR_IDENT",   git_author_info },
+   { "GIT_AUTHOR_EXPLICIT", author_explicit },
{ "GIT_EDITOR", editor },
{ "GIT_PAGER", pager },
{ "", NULL },
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 45a5f66..66b9810 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -26,4 +26,40 @@ test_expect_success 'git var can show multiple values' '
test_cmp expect actual
 '
 
+for type in AUTHOR COMMITTER; do
+   test_expect_success "$type ident can detect implicit values" "
+   echo 0 >expect &&
+   (
+   sane_unset GIT_${type}_NAME &&
+   sane_unset GIT_${type}_EMAIL &&
+   sane_unset EMAIL &&
+   git var GIT_${type}_EXPLICIT >actual
+   ) &&
+   test_cmp expect actual
+   "
+
+   test_expect_success "$type ident is explicit via environment" "
+   echo 1 >expect &&
+   (
+   GIT_${type}_NAME='A Name' &&
+   export GIT_${type}_NAME &&
+   GIT_${type}_EMAIL='n...@example.com' &&
+   export GIT_${type}_EMAIL &&
+   git var GIT_${type}_EXPLICIT >actual
+   ) &&
+   test_cmp expect actual
+   "
+
+   test_expect_success "$type ident is explicit via config" "
+   echo 1 >expect &&
+   test_config user.name 'A Name' &&
+   test_config user.email 'n...@example.com' &&
+   (
+   sane_unset GIT_${type}_NAME &&
+   sane_unset GIT_${type}_EMAIL &&
+   git var GIT_${type}_EXPLICIT >actual
+   )
+   "
+done
+
 test_done
-- 
1.8.0.207.gdf2154c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 3/6] var: accept multiple variables on the command line

2012-11-13 Thread Jeff King
Git-var currently only accepts a single value to print. This
is inefficient if the caller is interested in finding
multiple values, as they must invoke git-var multiple times.

This patch lets callers specify multiple variables, and
prints one per line.

Signed-off-by: Jeff King 
---
This will later let us get the "explicit" flag for free.

 Documentation/git-var.txt |  9 +++--
 builtin/var.c | 13 +++--
 t/t0007-git-var.sh| 29 +
 3 files changed, 43 insertions(+), 8 deletions(-)
 create mode 100755 t/t0007-git-var.sh

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 67edf58..53abba5 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -9,11 +9,16 @@ git-var - Show a git logical variable
 SYNOPSIS
 
 [verse]
-'git var' ( -l |  )
+'git var' ( -l | ... )
 
 DESCRIPTION
 ---
-Prints a git logical variable.
+Prints one or more git logical variables, separated by newlines.
+
+Note that some variables may contain newlines themselves (e.g.,
+`GIT_EDITOR`), and it is therefore possible to receive ambiguous output
+when requesting multiple variables. This can be mitigated by putting any
+such variables at the end of the list.
 
 OPTIONS
 ---
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..49b48f5 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, 
void *cb)
 
 int cmd_var(int argc, const char **argv, const char *prefix)
 {
-   const char *val = NULL;
-   if (argc != 2)
+   if (argc < 2)
usage(var_usage);
 
if (strcmp(argv[1], "-l") == 0) {
@@ -83,11 +82,13 @@ int cmd_var(int argc, const char **argv, const char *prefix)
return 0;
}
git_config(git_default_config, NULL);
-   val = read_var(argv[1]);
-   if (!val)
-   usage(var_usage);
 
-   printf("%s\n", val);
+   while (*++argv) {
+   const char *val = read_var(*argv);
+   if (!val)
+   usage(var_usage);
+   printf("%s\n", val);
+   }
 
return 0;
 }
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
new file mode 100755
index 000..45a5f66
--- /dev/null
+++ b/t/t0007-git-var.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='basic sanity checks for git var'
+. ./test-lib.sh
+
+test_expect_success 'get GIT_AUTHOR_IDENT' '
+   test_tick &&
+   echo "A U Thor  1112911993 -0700" >expect &&
+   git var GIT_AUTHOR_IDENT >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'get GIT_COMMITTER_IDENT' '
+   test_tick &&
+   echo "C O Mitter  1112912053 -0700" >expect &&
+   git var GIT_COMMITTER_IDENT >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'git var can show multiple values' '
+   cat >expect <<-\EOF &&
+   A U Thor  1112912053 -0700
+   C O Mitter  1112912053 -0700
+   EOF
+   git var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&
+   test_cmp expect actual
+'
+
+test_done
-- 
1.8.0.207.gdf2154c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 2/6] ident: keep separate "explicit" flags for author and committer

2012-11-13 Thread Jeff King
We keep track of whether the user ident was given to us
explicitly, or if we guessed at it from system parameters
like username and hostname. However, we kept only a single
variable. This covers the common cases (because the author
and committer will usually come from the same explicit
source), but can miss two cases:

  1. GIT_COMMITTER_* is set explicitly, but we fallback for
 GIT_AUTHOR. We claim the ident is explicit, even though
 the author is not.

  2. GIT_AUTHOR_* is set and we ask for author ident, but
 not committer ident. We will claim the ident is
 implicit, even though it is explicit.

This patch uses two variables instead of one, updates both
when we set the "fallback" values, and updates them
individually when we read from the environment.

Rather than keep user_ident_sufficiently_given as a
compatibility wrapper, we update the only two callers to
check the committer_ident, which matches their intent and
what was happening already.

Signed-off-by: Jeff King 
---
Case 1 made me initially think might need to check
author_ident_sufficiently_given when deciding whether to show the
"Author:" line during commit.  But I don't think it is necessary, as we
already check !strcmp(author, committer); if the committer is explicit
and the author is identical, even if it is implicit, there is no point
in telling the user.

 builtin/commit.c |  4 ++--
 cache.h  |  3 ++-
 ident.c  | 32 +---
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..d6dd3df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
ident_shown++ ? "" : "\n",
author_ident->buf);
 
-   if (!user_ident_sufficiently_given())
+   if (!committer_ident_sufficiently_given())
status_printf_ln(s, GIT_COLOR_NORMAL,
_("%s"
"Committer: %s"),
@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const 
unsigned char *sha1,
strbuf_addstr(&format, "\n Author: ");
strbuf_addbuf_percentquote(&format, &author_ident);
}
-   if (!user_ident_sufficiently_given()) {
+   if (!committer_ident_sufficiently_given()) {
strbuf_addstr(&format, "\n Committer: ");
strbuf_addbuf_percentquote(&format, &committer_ident);
if (advice_implicit_identity) {
diff --git a/cache.h b/cache.h
index 50d9eea..18fdd18 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,7 +1149,8 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-extern int user_ident_sufficiently_given(void);
+extern int committer_ident_sufficiently_given(void);
+extern int author_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
diff --git a/ident.c b/ident.c
index 733d69d..ac9672f 100644
--- a/ident.c
+++ b/ident.c
@@ -14,7 +14,8 @@ static char git_default_date[50];
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int user_ident_explicitly_given;
+static int committer_ident_explicitly_given;
+static int author_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
@@ -113,7 +114,8 @@ const char *ident_default_email(void)
 
if (email && email[0]) {
strbuf_addstr(&git_default_email, email);
-   user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+   committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+   author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
} else
copy_email(xgetpwuid_self(), &git_default_email);
strbuf_trim(&git_default_email);
@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)
 
 const char *git_author_info(int flag)
 {
+   if (getenv("GIT_AUTHOR_NAME"))
+   author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+   if (getenv("GIT_AUTHOR_EMAIL"))
+   author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return fmt_ident(getenv("GIT_AUTHOR_NAME"),
 getenv("GIT_AUTHOR_EMAIL"),
 getenv("GIT_AUTHOR_DATE"),
@@ -340,16 +346,16 @@ const char *git_author_info(int flag)
 const char *git_committer_info(int flag)
 {
if (getenv("GIT_COMMITTER_NAME"))
-   user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+   committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
if (getenv("GIT_COMMITTER_EMAIL"))
-   user_ident_explicitly_given |= IDENT_MAIL_GIVEN;

[PATCH 1/6] ident: make user_ident_explicitly_given private

2012-11-13 Thread Jeff King
There are no users of this global variable, as queriers
go through the user_ident_sufficiently_given accessor.
Let's make it private, which will enable further
refactoring.

Signed-off-by: Jeff King 
---
 cache.h | 4 
 ident.c | 6 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index dbd8018..50d9eea 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,10 +1149,6 @@ struct config_include_data {
 #define CONFIG_INCLUDE_INIT { 0 }
 extern int git_config_include(const char *name, const char *value, void *data);
 
-#define IDENT_NAME_GIVEN 01
-#define IDENT_MAIL_GIVEN 02
-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-extern int user_ident_explicitly_given;
 extern int user_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
diff --git a/ident.c b/ident.c
index a4bf206..733d69d 100644
--- a/ident.c
+++ b/ident.c
@@ -10,7 +10,11 @@
 static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static char git_default_date[50];
-int user_ident_explicitly_given;
+
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
+static int user_ident_explicitly_given;
 
 #ifdef NO_GECOS_IN_PWENT
 #define get_gecos(ignored) "&"
-- 
1.8.0.207.gdf2154c

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] send-email: add proper default sender

2012-11-13 Thread Jeff King
On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:

> > Those people would also not be using a new version of git-send-email,
> > and it will always prompt. I thought we were talking about what
> > send-email should do in future versions. Namely, loosening that safety
> > valve (the prompt) because it is inconvenient, but tightening the checks
> > so that losing the safety valve is not a problem.
> 
> Yeah, but all I'm saying is that the issue happens, you seemed to
> suggest that it doesn't.

What is "it"?

If it is bad guesses like "user@host.(none)" on unconfigured systems,
then I did not suggest it doesn't happen. I said that it happened with
older versions of git, but that it has now been fixed. Of course it will
continue to happen for people on older versions of git until they
upgrade. I cannot go back in time and fix released versions.

If it is guessing at all to end up with "user@host.domain" for a host
that does not receive mail, yes, that happens, and I have been very
clear that it does. The safety valve is showing the ident and a warning
to the user during the commit process.

I do not see any point in discussing the former when considering future
changes to git. It is already disallowed by current versions of git, and
we are just waiting for the whole world to upgrade to a fixed version.

I can see the argument for tightening the check to disallow the latter.
But it would also hurt real people who are relying on the feature.
Perhaps it would make sense to adopt the implicit_ident_advice in other
code paths besides "git commit" (e.g., via "git var).

> I think you are the one that is not understanding what I'm saying. But
> I don't think it matters.
> 
> This is what I'm saying; the current situation with 'git commit' is
> not OK, _both_ 'git commit' and 'git send-email' should change.

Perhaps I am being dense, but this is the first time it became apparent
to me that you are arguing for a change in "git commit".

> Indeed I would, but there's other people that would benefit from this
> patch. I'm sure I'm not the only person that doesn't have
> sendmail.from configured, but does have user.name/user.email, and is
> constantly typing enter.
> 
> And the difference is that I'm _real_, the hypothetical user that
> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced
> otherwise if some evidence was presented that such a user is real
> though.

Sadly we cannot poll the configuration of every user, nor expect them
all to pay attention to this discussion on the list. So we cannot take
the absence of comment from such users as evidence that they do not
exist. Instead we must use our judgement about what is being changed,
and we tend to err on the side of keeping the status quo, since that is
what the silent majority is busy _not_ complaining about.

We use the same judgement on the other side, too. Right now your
evidence is "1 user wants this, 0 users do not". But we can guess that
there are other people who would like the intent of your patch, but did
not care enough or are not active enough on the list to write the patch
themselves or comment on this thread.

It is also very easy to me accept the status quo when it is not in
fundamental conflict with the proposed improvement, but simply a matter
of implementation (which it is the case for send-email stopping
prompting in some cases, though it is not for changing the behavior of
git-commit).

> And to balance you need to *measure*, and that means taking into
> consideration who actually uses the features, if there's any. And it
> looks to me this is a feature nobody uses.

You said "measure" and then "it looks to me like". What did you measure?
Did you consider systematic bias in your measurement, like the fact that
people who are using the feature have no reason to come on the list and
announce it?

> But listen closely to what you said:
> 
> > I actually think it would make more sense to drop the prompt entirely and 
> > just die when the user has not given us a usable ident.
> 
> Suppose somebody has a full name, and a fully qualified domain name,
> and he can receive mails to it directly. Such a user would not need a
> git configuration, and would not need $EMAIL, or anything.
> 
> Currently 'git send-email' will throw 'Felipe Contreras
> ' which would actually work, but is not explicit.
> 
> You are suggesting to break that use-case. You are introducing a
> regression. And this case is realistic, unlike the
> GIT_AUTHOR_NAME/EMAIL. Isn't it?

Yes, dying would be a regression, in that you would have to configure
your name via the environment and re-run rather than type it at the
prompt. You raise a good point that for people who _could_ take the
implicit default, hitting "enter" is working fine now, and we would lose
that.  I'd be fine with also just continuing to prompt in the implicit
case.

But that is a much smaller issue to me than having send-email fail to
respect environment variables and silently use user.

Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance

2012-11-13 Thread Junio C Hamano
karsten.bl...@dcon.de writes:

> Jeff King  wrote on 02.11.2012 16:38:00:
>
>> On Fri, Nov 02, 2012 at 11:26:16AM -0400, Jeff King wrote:
>> 
>> > Still, I don't think we need to worry about performance regressions,
>> > because people who don't have a setup suitable for it will not turn on
>> > core.preloadindex in the first place. And if they have it on, the more
>> > places we use it, probably the better.
>> 
>> BTW, your patch was badly damaged in transit (wrapped, and tabs
>> converted to spaces). I was able to fix it up, but please check your
>> mailer's settings.
>> 
>
> Yes, I feared as much, that's why I included the pull URL (the company MTA 
> only accepts outbound mail from Notes clients, sorry).
>
> Is there a policy for people with broken mailers (send patch as 
> attachment, add pull URL more prominently, don't include plaintext patch 
> at all...)?

If anything, "fix your mailer" probably is the policy you are
looking for, I think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: checkout from neighbour branch undeletes a path?

2012-11-13 Thread Junio C Hamano
Peter Vereshagin  writes:

> Am wondering if 'checkout branch path' undeletes the files?

"git checkout branch path" (by the way, "branch" does not have to be
a branch name; any commit object name would do, like "git checkout
HEAD^^ hello.c") is a way to check out named path(s) out of the
named commit.

If the commit "branch" has "path" in it, its contents are checked
out to "path" in your current working tree (and the entry in the
index updated to match it).

If you happen to have removed "path" in your current working tree
before running that command, it might look as if there is some
undelete going on, but that is a wrong way to look at things.  The
version of "path" in the "branch" may or may not be similar to what
you have removed earlier.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Commit message problem of reverting multiple commits

2012-11-13 Thread Junio C Hamano
乙酸鋰  writes:

> I ran git 1.8.0 command line
>
> git revert --no-commit rev1 rev2
>
> I see a prepared commit message like
>
> Revert ""
> This reverts commit .
>
>
> The actual revert content is correct - it is all the relevant commits
> that were selected. I expect the message to reflect this:
>
> Revert "", ""
> This reverts commits , .

Hrmph.  I actually think the revert-content is not correct.

I think the command should not take more than one commit on the
command line when --no-commit is in use in the first place (the same
thing can be said for cherry-pick).  After all, "git revert rev1
rev2" is to revert rev1 and then rev2 independently, so unless that
option is spelled "--squash", the resulting history should have two
commits that reverts rev1 and rev2 independently.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Notes in format-patch

2012-11-13 Thread Junio C Hamano
Michael J Gruber  writes:

> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
>> '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 
>> ---
>> 
>> Notes:
>> v3 safeguards the hex buffer against reuse
>>  builtin/replace.c  | 16 ++--
>>  t/t6050-replace.sh | 11 +++
>>  2 files changed, 21 insertions(+), 6 deletions(-)
>> 
>> diff --git a/builtin/replace.c b/builtin/replace.c
>
> By the way - Junio, is that the intented outcome of "format-patch
> --notes"? I would rather put the newline between the note and the
> diffstat...

I do not mind (actually I personally would prefer to see) a blank
line between the three-dash and "Notes:", but I agree that we should
have a blank line before the diffstat block.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable

2012-11-13 Thread Junio C Hamano
Ramkumar Ramachandra  writes:

> Jeff King wrote:
>> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
>>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char 
>>> *value, void *cb)
>>>   return 0;
>>>   }
>>>
>>> + if (!strcmp(var, "diff.submodule")) {
>>
>> Shouldn't this be in git_diff_ui_config so it does not affect scripts
>> calling plumbing?
>
> I honestly didn't understand the difference between
> git_diff_basic_config and git_diff_ui_config.  The latter function
> calls the former function at the end of its body.  Why are they two
> separate functions in the first place?

In case you meant s/didn't/don't/, git_diff_ui_config() should be
called only by human-facing Porcelain commands where their
behaviours can and should be affected by end user configuration
variables.

When a configuration variable should not affect output from plumbing
commands like diff-files, diff-index, and diff-tree, it must not be
read in git_diff_basic_config(), but in git_diff_ui_config().

The output from "git format-patch" is consumed by "git apply" that
expects "Subproject commit %s\n" with fully spelled object name, so
your configuration must not affect the output of format-patch,
either.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] send-email: add proper default sender

2012-11-13 Thread Junio C Hamano
Jeff King  writes:

> On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:
> ...
>> 5) GIT_COMMITTER
>> 
>> Who should the emails appear to be from? [Felipe Contreras 2nd
>> ]
>> 
>> Whoa, what happened there?
>> 
>> Well:
>> 
>>   $sender = $repoauthor || $repocommitter || '';
>>   ($repoauthor) = Git::ident_person(@repo, 'author');
>>   % ./git var GIT_AUTHOR_IDENT
>>   Felipe Contreras 2nd  1352783223 +0100
>> 
>> That's right, AUTHOR_IDENT would fall back to the default email and full 
>> name.
>
> Yeah, I find that somewhat questionable in the current behavior, and I'd
> consider it a bug. Typically we prefer the committer ident when given a
> choice (e.g., for writing reflog entries).

Just to make sure I follow the discussion correctly, do you mean
that the bug is that we pick a fuzzy AUTHOR when COMMITTER is
specified more concretely and we usually use COMMIITTER for this
kind of thing in the first place but send-email does not in this
case (I do not see "git var GIT_AUTHOR_IDENT" returning value from
the implicit logic as a bug in this case---just making sure).

>> 6.1) GIT_AUTHOR without anything else
>> 
>> fatal: empty ident name (for ) not allowed
>> var GIT_COMMITTER_IDENT: command returned error: 128
>
> Doesn't that seem like a regression? It used to work.

I do not think we mind a change to favor GIT_COMMITTER setting over
GIT_AUTHOR to be consistent with others too much, but that indeed is
a big change.  People who are used to the inconsistency that
send-email favors AUTHOR over COMMITTER will certainly find it as a
regression.

> The explicitness needs to be tied to the specific ident we grabbed.
> Probably adding a "git var GIT_AUTHOR_EXPLICIT" would be enough, or
> alternatively, adding a flag to "git var" to error out rather than
> return a non-explicit ident (this may need to adjust the error
> handling of the "git var" calls from send-email).

Yeah, something like that would be necessary for "git var" users to
make this kind of decision.

> While simultaneously breaking "git commit" for people who are happily
> using the implicit generation. I can see the appeal of doing so; I was
> tempted to suggest it when I cleaned up IDENT_STRICT a few months back.
> But do we have any data on how many people are currently using that
> feature that would be annoyed by it?

As we didn't break "git commit" when you worked on IDENT_STRICT, I
do not think we have any data on it ;-)

> ...
> It may be something I would work on myself in the future, but I have
> other things to work on at the moment, and since you are interested in
> the topic, I thought you would be a good candidate to polish it enough
> to be suitable upstream. But instead I see a lot of push-back on what I
> considered to be a fairly straightforward technical comment on a
> regression.
> ...
> But I feel like I am fighting an uphill battle just to convince you that
> regressions are bad, and that I am having to make the same points
> repeatedly.  That makes me frustrated and less excited about reviewing
> your patches; and when I say "it is not my itch", that is my most polite
> way of saying "If that is going to be your attitude, then I do not feel
> like dealing with you anymore on this topic".

For a change with low benefit/cost ratio like this, the best course
of action often is to stop paying attention to the thread that has
become unproductive and venomous, and resurrect the topic after
people forgot about it and doing it right yourself ;-).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] gitweb: XSS vulnerability of RSS feed

2012-11-13 Thread Jakub Narębski
On Tue, Nov 13, 2012 at 4:45 PM, Kevin  wrote:
> The problem with input filtering is that you can only filter for one
> output scenario. What if the the input is going to be output in a wiki
> like environment, or to pdf, or whatever? Then you have to unescape
> the data again, and maybe apply filtering/escaping for those
> environments.
>
> You only know how to escape data when you are going to output it, so
> then is the the best moment to escape it.

Also there are so many ways to evade XSS filtering

  https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet

If you can and should escape data (like in our case), it cannot not work;
not possible to evade it.
-- 
Jakub Narebski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] gitweb: XSS vulnerability of RSS feed

2012-11-13 Thread Kevin
The problem with input filtering is that you can only filter for one
output scenario. What if the the input is going to be output in a wiki
like environment, or to pdf, or whatever? Then you have to unescape
the data again, and maybe apply filtering/escaping for those
environments.

You only know how to escape data when you are going to output it, so
then is the the best moment to escape it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable

2012-11-13 Thread Ramkumar Ramachandra
Jeff King wrote:
> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char 
>> *value, void *cb)
>>   return 0;
>>   }
>>
>> + if (!strcmp(var, "diff.submodule")) {
>
> Shouldn't this be in git_diff_ui_config so it does not affect scripts
> calling plumbing?

I honestly didn't understand the difference between
git_diff_basic_config and git_diff_ui_config.  The latter function
calls the former function at the end of its body.  Why are they two
separate functions in the first place?

Btw, I just posted a follow-up.

Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 4/4] submodule: display summary header in bold

2012-11-13 Thread Ramkumar Ramachandra
Currently, 'git diff --submodule' displays output with a bold diff
header for non-submodules.  So this part is in bold:

diff --git a/file1 b/file1
index 30b2f6c..2638038 100644
--- a/file1
+++ b/file1

For submodules, the header looks like this:

Submodule submodule1 012b072..248d0fd:

Unfortunately, it's easy to miss in the output because it's not bold.
Change this.

Signed-off-by: Ramkumar Ramachandra 
---
 diff.c  |2 +-
 submodule.c |8 
 submodule.h |2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index ffaed72..1065978 100644
--- a/diff.c
+++ b/diff.c
@@ -2261,7 +2261,7 @@ static void builtin_diff(const char *name_a,
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one ? one->path : two->path,
one->sha1, two->sha1, two->dirty_submodule,
-   del, add, reset);
+   meta, del, add, reset);
return;
}
 
diff --git a/submodule.c b/submodule.c
index e3e0b45..2f55436 100644
--- a/submodule.c
+++ b/submodule.c
@@ -258,7 +258,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, 
const char *arg)
 
 void show_submodule_summary(FILE *f, const char *path,
unsigned char one[20], unsigned char two[20],
-   unsigned dirty_submodule,
+   unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset)
 {
struct rev_info rev;
@@ -292,15 +292,15 @@ void show_submodule_summary(FILE *f, const char *path,
return;
}
 
-   strbuf_addf(&sb, "Submodule %s %s..", path,
+   strbuf_addf(&sb, "%sSubmodule %s %s..", meta, path,
find_unique_abbrev(one, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
if (message)
-   strbuf_addf(&sb, " %s\n", message);
+   strbuf_addf(&sb, " %s%s\n", message, reset);
else
-   strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
+   strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", 
reset);
fwrite(sb.buf, sb.len, 1, f);
 
if (!message) {
diff --git a/submodule.h b/submodule.h
index f2e8271..3dc1b3f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -20,7 +20,7 @@ void handle_ignore_submodules_arg(struct diff_options 
*diffopt, const char *);
 int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
 void show_submodule_summary(FILE *f, const char *path,
unsigned char one[20], unsigned char two[20],
-   unsigned dirty_submodule,
+   unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
 void set_config_fetch_recurse_submodules(int value);
 void check_for_new_submodule_commits(unsigned char new_sha1[20]);
-- 
1.7.8.1.362.g5d6df.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 3/4] diff: rename "set" variable

2012-11-13 Thread Ramkumar Ramachandra
From: Jeff King 

Once upon a time the builtin_diff function used one color, and the color
variables were called "set" and "reset". Nowadays it is a much longer
function and we use several colors (e.g., "add", "del"). Rename "set" to
"meta" to show that it is the color for showing diff meta-info (it still
does not indicate that it is a "color", but at least it matches the
scheme of the other color variables).

Signed-off-by: Jeff King 
Signed-off-by: Ramkumar Ramachandra 
---
 diff.c |   12 ++--
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index 7f2a255..ffaed72 100644
--- a/diff.c
+++ b/diff.c
@@ -2240,7 +2240,7 @@ static void builtin_diff(const char *name_a,
mmfile_t mf1, mf2;
const char *lbl[2];
char *a_one, *b_two;
-   const char *set = diff_get_color_opt(o, DIFF_METAINFO);
+   const char *meta = diff_get_color_opt(o, DIFF_METAINFO);
const char *reset = diff_get_color_opt(o, DIFF_RESET);
const char *a_prefix, *b_prefix;
struct userdiff_driver *textconv_one = NULL;
@@ -2287,24 +2287,24 @@ static void builtin_diff(const char *name_a,
b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
-   strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, set, 
a_one, b_two, reset);
+   strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, 
a_one, b_two, reset);
if (lbl[0][0] == '/') {
/* /dev/null */
-   strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, 
set, two->mode, reset);
+   strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, 
meta, two->mode, reset);
if (xfrm_msg)
strbuf_addstr(&header, xfrm_msg);
must_show_header = 1;
}
else if (lbl[1][0] == '/') {
-   strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", 
line_prefix, set, one->mode, reset);
+   strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", 
line_prefix, meta, one->mode, reset);
if (xfrm_msg)
strbuf_addstr(&header, xfrm_msg);
must_show_header = 1;
}
else {
if (one->mode != two->mode) {
-   strbuf_addf(&header, "%s%sold mode %06o%s\n", 
line_prefix, set, one->mode, reset);
-   strbuf_addf(&header, "%s%snew mode %06o%s\n", 
line_prefix, set, two->mode, reset);
+   strbuf_addf(&header, "%s%sold mode %06o%s\n", 
line_prefix, meta, one->mode, reset);
+   strbuf_addf(&header, "%s%snew mode %06o%s\n", 
line_prefix, meta, two->mode, reset);
must_show_header = 1;
}
if (xfrm_msg)
-- 
1.7.8.1.362.g5d6df.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 2/4] diff: introduce diff.submodule configuration variable

2012-11-13 Thread Ramkumar Ramachandra
Introduce a diff.submodule configuration variable corresponding to the
'--submodule' command-line option of 'git diff'.

Signed-off-by: Ramkumar Ramachandra 
---
 Documentation/diff-config.txt|7 +++
 Documentation/diff-options.txt   |3 ++-
 diff.c   |   32 
 t/t4041-diff-submodule-option.sh |   30 +-
 4 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index decd370..89dd634 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,13 @@ diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
before each empty output line. Defaults to false.
 
+diff.submodule::
+   Specify the format in which differences in submodules are
+   shown.  The "log" format lists the commits in the range like
+   linkgit:git-submodule[1] `summary` does.  The "short" format
+   format just shows the names of the commits at the beginning
+   and end of the range.  Defaults to short.
+
 diff.wordRegex::
A POSIX Extended Regular Expression used to determine what is a "word"
when performing word-by-word difference calculations.  Character
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cf4b216..f4f7e25 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -170,7 +170,8 @@ any of those replacements occurred.
the commits in the range like linkgit:git-submodule[1] `summary` does.
Omitting the `--submodule` option or specifying `--submodule=short`,
uses the 'short' format. This format just shows the names of the commits
-   at the beginning and end of the range.
+   at the beginning and end of the range.  Can be tweaked via the
+   `diff.submodule` configuration variable.
 
 --color[=]::
Show colored diff.
diff --git a/diff.c b/diff.c
index e89a201..7f2a255 100644
--- a/diff.c
+++ b/diff.c
@@ -123,6 +123,17 @@ static int parse_dirstat_params(struct diff_options 
*options, const char *params
return ret;
 }
 
+static int parse_submodule_params(struct diff_options *options, const char 
*value)
+{
+   if (!strcmp(value, "log"))
+   DIFF_OPT_SET(options, SUBMODULE_LOG);
+   else if (!strcmp(value, "short"))
+   DIFF_OPT_CLR(options, SUBMODULE_LOG);
+   else
+   return -1;
+   return 0;
+}
+
 static int git_config_rename(const char *var, const char *value)
 {
if (!value)
@@ -178,6 +189,13 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
if (!strcmp(var, "diff.ignoresubmodules"))
handle_ignore_submodules_arg(&default_diff_options, value);
 
+   if (!strcmp(var, "diff.submodule")) {
+   if (parse_submodule_params(&default_diff_options, value))
+   warning(_("Unknown value for 'diff.submodule' config 
variable: '%s'"),
+   value);
+   return 0;
+   }
+
if (git_color_config(var, value, cb) < 0)
return -1;
 
@@ -3475,6 +3493,14 @@ static int parse_dirstat_opt(struct diff_options 
*options, const char *params)
return 1;
 }
 
+static int parse_submodule_opt(struct diff_options *options, const char *value)
+{
+   if (parse_submodule_params(options, value))
+   die(_("Failed to parse --submodule option parameter: '%s'"),
+   value);
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
const char *arg = av[0];
@@ -3655,10 +3681,8 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
handle_ignore_submodules_arg(options, arg + 20);
} else if (!strcmp(arg, "--submodule"))
DIFF_OPT_SET(options, SUBMODULE_LOG);
-   else if (!prefixcmp(arg, "--submodule=")) {
-   if (!strcmp(arg + 12, "log"))
-   DIFF_OPT_SET(options, SUBMODULE_LOG);
-   }
+   else if (!prefixcmp(arg, "--submodule="))
+   return parse_submodule_opt(options, arg + 12);
 
/* misc options */
else if (!strcmp(arg, "-z"))
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 6c01d0c..e401814 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -33,6 +33,7 @@ test_create_repo sm1 &&
 add_file . foo >/dev/null
 
 head1=$(add_file sm1 foo1 foo2)
+fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
 
 test_expect_success 'added submodule' "
git add sm1 &&
@@ -43,6 +44,34 @@ EOF
test_cmp expected actual
 "
 
+test_expect_success 'added submodule, set diff.submodule' "
+   git config diff.submodule log &&
+   git add sm1 &&
+   git diff --cached >actual &&
+ 

[PATCH v4 1/4] Documentation: move diff.wordRegex from config.txt to diff-config.txt

2012-11-13 Thread Ramkumar Ramachandra
19299a8 (Documentation: Move diff..* from config.txt to
diff-config.txt, 2011-04-07) moved the diff configuration options to
diff-config.txt, but forgot about diff.wordRegex, which was left
behind in config.txt.  Fix this.

Signed-off-by: Ramkumar Ramachandra 
---
 Documentation/config.txt  |6 --
 Documentation/diff-config.txt |6 ++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9a0544c..e70216d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -962,12 +962,6 @@ difftool..cmd::
 difftool.prompt::
Prompt before each invocation of the diff tool.
 
-diff.wordRegex::
-   A POSIX Extended Regular Expression used to determine what is a "word"
-   when performing word-by-word difference calculations.  Character
-   sequences that match the regular expression are "words", all other
-   characters are *ignorable* whitespace.
-
 fetch.recurseSubmodules::
This option can be either set to a boolean value or to 'on-demand'.
Setting it to a boolean changes the behavior of fetch and pull to
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 75ab8a5..decd370 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,12 @@ diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
before each empty output line. Defaults to false.
 
+diff.wordRegex::
+   A POSIX Extended Regular Expression used to determine what is a "word"
+   when performing word-by-word difference calculations.  Character
+   sequences that match the regular expression are "words", all other
+   characters are *ignorable* whitespace.
+
 diff..command::
The custom diff driver command.  See linkgit:gitattributes[5]
for details.
-- 
1.7.8.1.362.g5d6df.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v4 0/4] Introduce diff.submodule

2012-11-13 Thread Ramkumar Ramachandra
v1 is here: 
http://mid.gmane.org/1349196670-2844-1-git-send-email-artag...@gmail.com
v2 is here: 
http://mid.gmane.org/1351766630-4837-1-git-send-email-artag...@gmail.com
v3 is here: 
http://mid.gmane.org/1352653146-3932-1-git-send-email-artag...@gmail.com

This version was prepared in response to Peff's review of v3.
What changed:
* Functional code simplified and moved to git_diff_ui_config.
* Peff contributed one additional patch to the series.

Thanks.

Ram

Jeff King (1):
  diff: rename "set" variable

Ramkumar Ramachandra (3):
  Documentation: move diff.wordRegex from config.txt to diff-config.txt
  diff: introduce diff.submodule configuration variable
  submodule: display summary header in bold

 Documentation/config.txt |6 -
 Documentation/diff-config.txt|   13 ++
 Documentation/diff-options.txt   |3 +-
 diff.c   |   46 -
 submodule.c  |8 +++---
 submodule.h  |2 +-
 t/t4041-diff-submodule-option.sh |   30 -
 7 files changed, 84 insertions(+), 24 deletions(-)

-- 
1.7.8.1.362.g5d6df.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance

2012-11-13 Thread karsten . blees
Jeff King  wrote on 02.11.2012 16:38:00:

> On Fri, Nov 02, 2012 at 11:26:16AM -0400, Jeff King wrote:
> 
> > Still, I don't think we need to worry about performance regressions,
> > because people who don't have a setup suitable for it will not turn on
> > core.preloadindex in the first place. And if they have it on, the more
> > places we use it, probably the better.
> 
> BTW, your patch was badly damaged in transit (wrapped, and tabs
> converted to spaces). I was able to fix it up, but please check your
> mailer's settings.
> 

Yes, I feared as much, that's why I included the pull URL (the company MTA 
only accepts outbound mail from Notes clients, sorry).

Is there a policy for people with broken mailers (send patch as 
attachment, add pull URL more prominently, don't include plaintext patch 
at all...)?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance

2012-11-13 Thread karsten . blees
Jeff King  wrote on 02.11.2012 16:26:16:

> On Tue, Oct 30, 2012 at 10:50:42AM +0100, karsten.bl...@dcon.de wrote:
> 
> > 'update-index --refresh' and 'diff-index' (without --cached) don't 
honor
> > the core.preloadindex setting yet. Porcelain commands using these 
(such as
> > git [svn] rebase) suffer from this, especially on Windows.
> > 
> > Use read_cache_preload to improve performance.
> > 
> > Additionally, in builtin/diff.c, don't preload index status if we 
don't
> > access the working copy (--cached).
> > 
> > Results with msysgit on WebKit repo (2GB in 200k files):
> > 
> > | update-index | diff-index | rebase
> > +--++-
> > msysgit-v1.8.0  |   9.157s |10.536s | 42.791s
> > + preloadindex  |   9.157s |10.536s | 28.725s
> > + this patch|   2.329s | 2.752s | 15.152s
> > + fscache [1]   |   0.731s | 1.171s |  8.877s
> 
> Cool numbers. On my quad-core SSD Linux box, I saw a few speedups, too.
> Here are the numbers for "update-index --refresh" on the WebKit repo
> (all are wall clock time, best-of-five):
> 
>  | before | after
>   ---++
>   cold cache | 4.513s | 2.059s
>   warm cache | 0.252s | 0.164s
>

Great, and thanks for testing on Linux (I only have Linux VMs for testing, 
and I couldn't get meaningful performance data from those).
 
> Not as dramatic, but still nice. I wonder how a spinning disk would fare
> on the cold-cache case, though.  I also tried it with all but one CPU
> disabled, and the warm cache case was a little bit slower.
> 

Unfortunately, with a 'real' disk, cold cache times increase with the 
number of threads. I've played around with '#define MAX_PARALLEL 20' in 
preload-index.c, with the following results ('git update-index --refresh' 
on WebKit repo, Win7 x64, Core i7 860 (2.8GHz 4 Core HT), WD VelociRaptor 
(300G 10krpm 8ms), msysgit 1.8.0 + preload-index patch + fscache patch):

MAX_PARALLEL | cold cache | warm cache
-++
no preload   | 49.938 | 9.204 (*)
   1 | 45.412 | 1.622
   2 | 55.334 | 1.123
   3 | 65.973 | 0.982
   4 | 67.579 | 0.889
   5 | 76.159 | 0.827
   6 | 81.510 | 0.811
   7 | 86.269 | 0.858
   8 | 85.862 | 0.827
...
  10 | 87.953 | 0.717
...
  20 |176.251 | 0.749

(*) core.preloadindex currently also disables fscache, thus the 9s

With more threads, Windows resource monitor also shows increasing disk 
queue length and response times.

It seems clear that more concurrent disk seeks hurt cold cache performance 
pretty badly. On the other hand, warm cache improvements for #threads > 
#cores are only about 10 - 20%.

I don't know if Linux is better at caching / prefetching directory 
listings (might depend on file system, too), but perhaps MAX_PARALLEL 
should be set to a more reasonable value, or be made configurable (e.g. 
core.preloadIndexThreads)?

Karsten
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


checkout from neighbour branch undeletes a path?

2012-11-13 Thread Peter Vereshagin
Hello.

Am wondering if 'checkout branch path' undeletes the files? For the example
below I'd like the 'file00.txt' to be deleted and never checked out from the
previous branch... How can I do that?

  $ git init
 
  Initialized empty Git repository in /tmp/repo00/.git/
  $ mkdir pathdir
  $ echo test00 > pathdir/file00.txt
  $ git add pathdir
  $ git commit -am 'added file00.txt'
  [master (root-commit) d4f7c70] added file00.txt
   1 files changed, 1 insertions(+), 0 deletions(-)
   create mode 100644 pathdir/file00.txt
  $ git branch -m master branch00
  $ git branch branch01
  $ rm pathdir/file00.txt
  $ echo test01 > pathdir/file01.txt
  $ git add pathdir
  $ git status
  $ git commit -am 'added file01.txt; removed file00.txt'
  [branch00 c3e78ff] added file01.txt; removed file00.txt
   2 files changed, 1 insertions(+), 1 deletions(-)
   delete mode 100644 pathdir/file00.txt
   create mode 100644 pathdir/file01.txt
  $ git checkout branch01
  Switched to branch 'branch01'
  $ rm -r pathdir
  $ git checkout branch00 pathdir
  $ find pathdir/
  pathdir/
  pathdir/file00.txt
  pathdir/file01.txt
  $

I know about 'merge' and it's not the what I need:  to import only the
particular subdirectory from the previous branch.

Thank you.

--
Peter Vereshagin  (http://vereshagin.org) pgp: A0E26627
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] gitweb: XSS vulnerability of RSS feed

2012-11-13 Thread Jakub Narębski
On Tue, Nov 13, 2012 at 3:44 PM, Drew Northup  wrote:
> On Mon, Nov 12, 2012 at 3:24 PM, Jeff King  wrote:
>> On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:

>>> +   # No XSS  inclusions
>>> +   if ($input =~ m!()(.*)()!){
>>> +   return undef;
>>> +   }

>> 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 "foo". 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.
>
> I don't buy the argument that we don't need to clean up the input as
> well. There are scant few of us that are going to name a file
> "alert("Something Awful")" in this world (I am
> probably one of them). Input validation is key to keeping problems
> like this from coming up repeatedly as those writing the guts of
> programs are typically more interested in getting the "assigned task"
> done and reporting the output to the user in a safe manner.

Input cleanup or blacklisting *does not* prevent code injection (XSS
in this case). This is a myth.

Input validation has its place, and is done by gitweb when possible
(see e.g. evaluate_and_validate_params, validate_project, etc.).

But the proposed solution is not input validation.
'alert("Something Awful")' is a perfectly valid filename.
As is more realistic "<>.uml" or "File > Open screenshot.png".

And last and most important you have to escape output anyway;
filename is not HTML. Without escaping it would be rendered incorrectly.
And HTML escaping prevents XSS.

>> 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.
>
> Escaping the output protects the user, but it DOES NOT protect the
> server. We MUST handle both possibilities.

Errr, what?

If you are thinking about shell injection, we are covered.
Gitweb uses list form of open which is for shell what prepared
statements are for SQL. In one or two cases where we need to
use pipe we do shell escaping.

> Besides, inserting one call to esc_html only fixes one attack path. I
> didn't look to see if all others were already covered.

They should be covered. This case slipped.

-- 
Jakub Narebski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Commit message problem of reverting multiple commits

2012-11-13 Thread 乙酸鋰
Hi,

I ran git 1.8.0 command line

git revert --no-commit rev1 rev2

I see a prepared commit message like

Revert ""
This reverts commit .


The actual revert content is correct - it is all the relevant commits
that were selected. I expect the message to reflect this:

Revert "", ""
This reverts commits , .

Regards,
ch3cooli
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [BUG] gitweb: XSS vulnerability of RSS feed

2012-11-13 Thread Drew Northup
On Mon, Nov 12, 2012 at 3:24 PM, Jeff King  wrote:
> On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:
>
>> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron  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  inclusions
>> +   if ($input =~ m!()(.*)()!){
>> +   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 "foo". 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.

I don't buy the argument that we don't need to clean up the input as
well. There are scant few of us that are going to name a file
"alert("Something Awful")" in this world (I am
probably one of them). Input validation is key to keeping problems
like this from coming up repeatedly as those writing the guts of
programs are typically more interested in getting the "assigned task"
done and reporting the output to the user in a safe manner.

>   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 "foo" as bold. I would also not be
>  surprised if there are other cross-site attacks one can do without
>  using 

Re: [PATCH 0/5] win32: support echo for terminal-prompt

2012-11-13 Thread Erik Faye-Lund
Sorry, I messed up the subject (lacking RFC-prefix), so I aborted
after sending the cover-letter. I'll resend with a proper prefix right
away.

On Tue, Nov 13, 2012 at 3:01 PM, Erik Faye-Lund  wrote:
> We currently only support getpass, which does not echo at all, for
> git_terminal_prompt on Windows. The Windows console is perfectly
> capable of doing this, so let's make it so.
>
> This implementation tries to reuse the /dev/tty-code as much as
> possible.
>
> The big reason that this becomes a bit hairy is that Ctrl+C needs
> to be handled correctly, so we don't leak the console state to a
> non-echoing setting when a user aborts.
>
> Windows makes this bit a little bit tricky, in that we need to
> implement SIGINT for fgetc. However, I suspect that this is a good
> thing to do in the first place.
>
> An earlier iteration was also breifly discussed here:
> http://mid.gmane.org/cabpqnsaucedu4+2n63n0k_xwsxop_ifzg3geyspsbpcsvv8...@mail.gmail.com
>
> The series can also be found here, only with an extra patch that
> makes the (interactive) testing a bit easier:
>
> https://github.com/kusma/git/tree/work/terminal-cleanup
>
> Erik Faye-Lund (5):
>   mingw: make fgetc raise SIGINT if apropriate
>   compat/terminal: factor out echo-disabling
>   compat/terminal: separate input and output handles
>   mingw: reuse tty-version of git_terminal_prompt
>   mingw: get rid of getpass implementation
>
>  compat/mingw.c|  91 +++---
>  compat/mingw.h|   8 +++-
>  compat/terminal.c | 129 
> --
>  3 files changed, 169 insertions(+), 59 deletions(-)
>
> --
> 1.8.0.7.gbeffeda
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH/RFC 4/5] mingw: reuse tty-version of git_terminal_prompt

2012-11-13 Thread Erik Faye-Lund
The getpass-implementation we use on Windows isn't at all ideal;
it works in raw-mode (as opposed to cooked mode), and as a result
does not deal correcly with deletion, arrow-keys etc.

Instead, use cooked mode to read a line at the time, allowing the
C run-time to process the input properly.

Since we set files to be opened in binary-mode by default on
Windows, introduce a FORCE_TEXT macro that expands to the "t"
modifier that forces the terminal to be opened in text-mode so we
do not have to deal with CRLF issues.

Signed-off-by: Erik Faye-Lund 
---
 compat/terminal.c | 69 +++
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 4a1fd3d..ce0fbd9 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -3,8 +3,22 @@
 #include "sigchain.h"
 #include "strbuf.h"
 
+#if defined(HAVE_DEV_TTY) || defined(WIN32)
+
+static void restore_term(void);
+
+static void restore_term_on_signal(int sig)
+{
+   restore_term();
+   sigchain_pop(sig);
+   raise(sig);
+}
+
 #ifdef HAVE_DEV_TTY
 
+#define INPUT_PATH "/dev/tty"
+#define OUTPUT_PATH "/dev/tty"
+
 static int term_fd = -1;
 static struct termios old_term;
 
@@ -18,13 +32,6 @@ static void restore_term(void)
term_fd = -1;
 }
 
-static void restore_term_on_signal(int sig)
-{
-   restore_term();
-   sigchain_pop(sig);
-   raise(sig);
-}
-
 static int disable_echo()
 {
struct termios t;
@@ -46,17 +53,61 @@ error:
return -1;
 }
 
+#elif defined(WIN32)
+
+#define INPUT_PATH "CONIN$"
+#define OUTPUT_PATH "CONOUT$"
+#define FORCE_TEXT "t"
+
+static HANDLE hconin = INVALID_HANDLE_VALUE;
+static DWORD cmode;
+
+static void restore_term(void)
+{
+   if (hconin == INVALID_HANDLE_VALUE)
+   return;
+
+   SetConsoleMode(hconin, cmode);
+   CloseHandle(hconin);
+   hconin = INVALID_HANDLE_VALUE;
+}
+
+static int disable_echo(void)
+{
+   hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
+   FILE_SHARE_READ, NULL, OPEN_EXISTING,
+   FILE_ATTRIBUTE_NORMAL, NULL);
+   if (hconin == INVALID_HANDLE_VALUE)
+   return -1;
+
+   GetConsoleMode(hconin, &cmode);
+   sigchain_push_common(restore_term_on_signal);
+   if (!SetConsoleMode(hconin, cmode & (~ENABLE_ECHO_INPUT))) {
+   CloseHandle(hconin);
+   hconin = INVALID_HANDLE_VALUE;
+   return -1;
+   }
+
+   return 0;
+}
+
+#endif
+
+#ifndef FORCE_TEXT
+#define FORCE_TEXT
+#endif
+
 char *git_terminal_prompt(const char *prompt, int echo)
 {
static struct strbuf buf = STRBUF_INIT;
int r;
FILE *input_fh, *output_fh;
 
-   input_fh = fopen("/dev/tty", "r");
+   input_fh = fopen(INPUT_PATH, "r" FORCE_TEXT);
if (!input_fh)
return NULL;
 
-   output_fh = fopen("/dev/tty", "w");
+   output_fh = fopen(OUTPUT_PATH, "w" FORCE_TEXT);
if (!output_fh) {
fclose(input_fh);
return NULL;
-- 
1.8.0.7.gbeffeda

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH/RFC 5/5] mingw: get rid of getpass implementation

2012-11-13 Thread Erik Faye-Lund
There's no remaining call-sites, and as pointed out in the
previous commit message, it's not quite ideal. So let's just
lose it.

Signed-off-by: Erik Faye-Lund 
---
 compat/mingw.c | 15 ---
 compat/mingw.h |  2 --
 2 files changed, 17 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 33ddfdf..5fc14b7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1758,21 +1758,6 @@ int link(const char *oldpath, const char *newpath)
return 0;
 }
 
-char *getpass(const char *prompt)
-{
-   struct strbuf buf = STRBUF_INIT;
-
-   fputs(prompt, stderr);
-   for (;;) {
-   char c = _getch();
-   if (c == '\r' || c == '\n')
-   break;
-   strbuf_addch(&buf, c);
-   }
-   fputs("\n", stderr);
-   return strbuf_detach(&buf, NULL);
-}
-
 pid_t waitpid(pid_t pid, int *status, int options)
 {
HANDLE h = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION,
diff --git a/compat/mingw.h b/compat/mingw.h
index 6b9e69a..f494ecb 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -55,8 +55,6 @@ struct passwd {
char *pw_dir;
 };
 
-extern char *getpass(const char *prompt);
-
 typedef void (__cdecl *sig_handler_t)(int);
 struct sigaction {
sig_handler_t sa_handler;
-- 
1.8.0.7.gbeffeda

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH/RFC 3/5] compat/terminal: separate input and output handles

2012-11-13 Thread Erik Faye-Lund
On Windows, the terminal cannot be opened in read-write mode, so
we need distinct pairs for reading and writing. Since this works
fine on other platforms as well, always open them in pairs.

Signed-off-by: Erik Faye-Lund 
---
 compat/terminal.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 3217838..4a1fd3d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -50,29 +50,36 @@ char *git_terminal_prompt(const char *prompt, int echo)
 {
static struct strbuf buf = STRBUF_INIT;
int r;
-   FILE *fh;
+   FILE *input_fh, *output_fh;
 
-   fh = fopen("/dev/tty", "w+");
-   if (!fh)
+   input_fh = fopen("/dev/tty", "r");
+   if (!input_fh)
return NULL;
 
+   output_fh = fopen("/dev/tty", "w");
+   if (!output_fh) {
+   fclose(input_fh);
+   return NULL;
+   }
+
if (!echo && disable_echo()) {
-   fclose(fh);
+   fclose(input_fh);
+   fclose(output_fh);
return NULL;
}
 
-   fputs(prompt, fh);
-   fflush(fh);
+   fputs(prompt, output_fh);
+   fflush(output_fh);
 
-   r = strbuf_getline(&buf, fh, '\n');
+   r = strbuf_getline(&buf, input_fh, '\n');
if (!echo) {
-   fseek(fh, SEEK_CUR, 0);
-   putc('\n', fh);
-   fflush(fh);
+   putc('\n', output_fh);
+   fflush(output_fh);
}
 
restore_term();
-   fclose(fh);
+   fclose(input_fh);
+   fclose(output_fh);
 
if (r == EOF)
return NULL;
-- 
1.8.0.7.gbeffeda

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH/RFC 2/5] compat/terminal: factor out echo-disabling

2012-11-13 Thread Erik Faye-Lund
By moving the echo-disabling code to a separate function, we can
implement OS-specific versions of it for non-POSIX platforms.

Signed-off-by: Erik Faye-Lund 
---
 compat/terminal.c | 43 +--
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index bbb038d..3217838 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -14,6 +14,7 @@ static void restore_term(void)
return;
 
tcsetattr(term_fd, TCSAFLUSH, &old_term);
+   close(term_fd);
term_fd = -1;
 }
 
@@ -24,6 +25,27 @@ static void restore_term_on_signal(int sig)
raise(sig);
 }
 
+static int disable_echo()
+{
+   struct termios t;
+
+   term_fd = open("/dev/tty", O_RDWR);
+   if (tcgetattr(term_fd, &t) < 0)
+   goto error;
+
+   old_term = t;
+   sigchain_push_common(restore_term_on_signal);
+
+   t.c_lflag &= ~ECHO;
+   if (!tcsetattr(term_fd, TCSAFLUSH, &t))
+   return 0;
+
+error:
+   close(term_fd);
+   term_fd = -1;
+   return -1;
+}
+
 char *git_terminal_prompt(const char *prompt, int echo)
 {
static struct strbuf buf = STRBUF_INIT;
@@ -34,24 +56,9 @@ char *git_terminal_prompt(const char *prompt, int echo)
if (!fh)
return NULL;
 
-   if (!echo) {
-   struct termios t;
-
-   if (tcgetattr(fileno(fh), &t) < 0) {
-   fclose(fh);
-   return NULL;
-   }
-
-   old_term = t;
-   term_fd = fileno(fh);
-   sigchain_push_common(restore_term_on_signal);
-
-   t.c_lflag &= ~ECHO;
-   if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
-   term_fd = -1;
-   fclose(fh);
-   return NULL;
-   }
+   if (!echo && disable_echo()) {
+   fclose(fh);
+   return NULL;
}
 
fputs(prompt, fh);
-- 
1.8.0.7.gbeffeda

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

2012-11-13 Thread Erik Faye-Lund
Set a control-handler to prevent the process from terminating, and
simulate SIGINT so it can be handled by a signal-handler as usual.

Signed-off-by: Erik Faye-Lund 
---
 compat/mingw.c | 76 ++
 compat/mingw.h |  6 +
 2 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 78e8f54..33ddfdf 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -319,6 +319,31 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)
return write(fd, buf, min(count, 31 * 1024 * 1024));
 }
 
+static BOOL WINAPI ctrl_ignore(DWORD type)
+{
+   return TRUE;
+}
+
+#undef fgetc
+int mingw_fgetc(FILE *stream)
+{
+   int ch;
+   if (!isatty(_fileno(stream)))
+   return fgetc(stream);
+
+   SetConsoleCtrlHandler(ctrl_ignore, TRUE);
+   while (1) {
+   ch = fgetc(stream);
+   if (ch != EOF || GetLastError() != ERROR_OPERATION_ABORTED)
+   break;
+
+   /* Ctrl+C was pressed, simulate SIGINT and retry */
+   mingw_raise(SIGINT);
+   }
+   SetConsoleCtrlHandler(ctrl_ignore, FALSE);
+   return ch;
+}
+
 #undef fopen
 FILE *mingw_fopen (const char *filename, const char *otype)
 {
@@ -1524,7 +1549,7 @@ static HANDLE timer_event;
 static HANDLE timer_thread;
 static int timer_interval;
 static int one_shot;
-static sig_handler_t timer_fn = SIG_DFL;
+static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;
 
 /* The timer works like this:
  * The thread, ticktack(), is a trivial routine that most of the time
@@ -1538,13 +1563,7 @@ static sig_handler_t timer_fn = SIG_DFL;
 static unsigned __stdcall ticktack(void *dummy)
 {
while (WaitForSingleObject(timer_event, timer_interval) == 
WAIT_TIMEOUT) {
-   if (timer_fn == SIG_DFL) {
-   if (isatty(STDERR_FILENO))
-   fputs("Alarm clock\n", stderr);
-   exit(128 + SIGALRM);
-   }
-   if (timer_fn != SIG_IGN)
-   timer_fn(SIGALRM);
+   mingw_raise(SIGALRM);
if (one_shot)
break;
}
@@ -1635,12 +1654,49 @@ int sigaction(int sig, struct sigaction *in, struct 
sigaction *out)
 sig_handler_t mingw_signal(int sig, sig_handler_t handler)
 {
sig_handler_t old = timer_fn;
-   if (sig != SIGALRM)
+
+   switch (sig) {
+   case SIGALRM:
+   timer_fn = handler;
+   break;
+
+   case SIGINT:
+   sigint_fn = handler;
+   break;
+
+   default:
return signal(sig, handler);
-   timer_fn = handler;
+   }
+
return old;
 }
 
+#undef raise
+int mingw_raise(int sig)
+{
+   switch (sig) {
+   case SIGALRM:
+   if (timer_fn == SIG_DFL) {
+   if (isatty(STDERR_FILENO))
+   fputs("Alarm clock\n", stderr);
+   exit(128 + SIGALRM);
+   } else if (timer_fn != SIG_IGN)
+   timer_fn(SIGALRM);
+   return 0;
+
+   case SIGINT:
+   if (sigint_fn == SIG_DFL)
+   exit(128 + SIGINT);
+   else if (sigint_fn != SIG_IGN)
+   sigint_fn(SIGINT);
+   return 0;
+
+   default:
+   return raise(sig);
+   }
+}
+
+
 static const char *make_backslash_path(const char *path)
 {
static char buf[PATH_MAX + 1];
diff --git a/compat/mingw.h b/compat/mingw.h
index 61a6521..6b9e69a 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -179,6 +179,9 @@ int mingw_open (const char *filename, int oflags, ...);
 ssize_t mingw_write(int fd, const void *buf, size_t count);
 #define write mingw_write
 
+int mingw_fgetc(FILE *stream);
+#define fgetc mingw_fgetc
+
 FILE *mingw_fopen (const char *filename, const char *otype);
 #define fopen mingw_fopen
 
@@ -287,6 +290,9 @@ static inline unsigned int git_ntohl(unsigned int x)
 sig_handler_t mingw_signal(int sig, sig_handler_t handler);
 #define signal mingw_signal
 
+int mingw_raise(int sig);
+#define raise mingw_raise
+
 /*
  * ANSI emulation wrappers
  */
-- 
1.8.0.7.gbeffeda

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH/RFC 0/5] win32: support echo for terminal-prompt

2012-11-13 Thread Erik Faye-Lund
We currently only support getpass, which does not echo at all, for
git_terminal_prompt on Windows. The Windows console is perfectly
capable of doing this, so let's make it so.

This implementation tries to reuse the /dev/tty-code as much as
possible.

The big reason that this becomes a bit hairy is that Ctrl+C needs
to be handled correctly, so we don't leak the console state to a
non-echoing setting when a user aborts.

Windows makes this bit a little bit tricky, in that we need to
implement SIGINT for fgetc. However, I suspect that this is a good
thing to do in the first place.

An earlier iteration was also breifly discussed here:
http://mid.gmane.org/cabpqnsaucedu4+2n63n0k_xwsxop_ifzg3geyspsbpcsvv8...@mail.gmail.com

The series can also be found here, only with an extra patch that
makes the (interactive) testing a bit easier:

https://github.com/kusma/git/tree/work/terminal-cleanup

Erik Faye-Lund (5):
  mingw: make fgetc raise SIGINT if apropriate
  compat/terminal: factor out echo-disabling
  compat/terminal: separate input and output handles
  mingw: reuse tty-version of git_terminal_prompt
  mingw: get rid of getpass implementation

 compat/mingw.c|  91 +++---
 compat/mingw.h|   8 +++-
 compat/terminal.c | 129 --
 3 files changed, 169 insertions(+), 59 deletions(-)

-- 
1.8.0.7.gbeffeda

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 0/5] win32: support echo for terminal-prompt

2012-11-13 Thread Erik Faye-Lund
We currently only support getpass, which does not echo at all, for
git_terminal_prompt on Windows. The Windows console is perfectly
capable of doing this, so let's make it so.

This implementation tries to reuse the /dev/tty-code as much as
possible.

The big reason that this becomes a bit hairy is that Ctrl+C needs
to be handled correctly, so we don't leak the console state to a
non-echoing setting when a user aborts.

Windows makes this bit a little bit tricky, in that we need to
implement SIGINT for fgetc. However, I suspect that this is a good
thing to do in the first place.

An earlier iteration was also breifly discussed here:
http://mid.gmane.org/cabpqnsaucedu4+2n63n0k_xwsxop_ifzg3geyspsbpcsvv8...@mail.gmail.com

The series can also be found here, only with an extra patch that
makes the (interactive) testing a bit easier:

https://github.com/kusma/git/tree/work/terminal-cleanup

Erik Faye-Lund (5):
  mingw: make fgetc raise SIGINT if apropriate
  compat/terminal: factor out echo-disabling
  compat/terminal: separate input and output handles
  mingw: reuse tty-version of git_terminal_prompt
  mingw: get rid of getpass implementation

 compat/mingw.c|  91 +++---
 compat/mingw.h|   8 +++-
 compat/terminal.c | 129 --
 3 files changed, 169 insertions(+), 59 deletions(-)

-- 
1.8.0.7.gbeffeda

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv4] replace: parse revision argument for -d

2012-11-13 Thread Jeff King
On Tue, Nov 13, 2012 at 11:34:11AM +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 
> ---
> 
> Notes:
> v4 names the aux variable more concisely and does away with a superfluous
> assignment.

Thanks. I agree the name "full_hex" is much better. Patch looks good to
me at this point.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Reviews on mailing-list

2012-11-13 Thread Nguyen Thai Ngoc Duy
On Mon, Nov 12, 2012 at 4:15 AM, David Lang  wrote:
> Using a web browser requires connectivity at the time you are doing the
> review.
>
> Mailing list based reviews can be done at times when you don't have
> connectivity.

I am not against email-based reviews but I'd like to point out that
with Google Gears (and HTML5 Storage?) Gerrit can be made work offline
too. I don't know how much work required though.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)

2012-11-13 Thread Jeff King
On Tue, Nov 13, 2012 at 11:30:19AM +0100, Michael J Gruber wrote:

> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
> > '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 
> > ---
> > 
> > Notes:
> > v3 safeguards the hex buffer against reuse
> >  builtin/replace.c  | 16 ++--
> >  t/t6050-replace.sh | 11 +++
> >  2 files changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/builtin/replace.c b/builtin/replace.c
> 
> By the way - Junio, is that the intented outcome of "format-patch
> --notes"? I would rather put the newline between the note and the
> diffstat (and omit the one after the ---) but may have goofed up a rebase:

I do not know was intended, but the above quoted output is hard to read,
and your suggested change looks way better.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Simple question ? [Git branch]

2012-11-13 Thread agatte
Hi All Users,

I am beginner in git. I am doing my first steps with this tool.
Now, I used git gui on linux OS.
I don't know what I could change branches ?
I need to change current working branch to do a commit.
I can see in the menu branch :
Create
checkout
rebase
Reset
--

Could anyone help me please ?


I would appreciate for any help.



agatte




--
View this message in context: 
http://git.661346.n2.nabble.com/Simple-question-Git-branch-tp7571115.html
Sent from the git mailing list archive at Nabble.com.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash

2012-11-13 Thread SZEDER Gábor
Hi,

On Mon, Nov 12, 2012 at 03:07:46PM -0500, Marc Khouzam wrote:
> Hi,

[...]

> Signed-off-by: Marc Khouzam 

[...]

> 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

Please have a look at Documentation/SubmittingPatches to see how to
properly format the commit message, i.e. no greeting and sign-off in
the commit message part, and the S-o-b line should be the last before
the '---'.

Your patch seems to be severely line-wrapped.  That document also
contains a few MUA-specific tips to help avoid that.

Other than that, it's a good description of the changes and
considerations.  I agree that this approach seems to be the best from
the three.

> 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

See comment near the end.

>  #
>  # 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)

That comment is only true for older Bash versions.  Since v4 Bash
splits the command line at characters that the readline library treats
as word separators when performing word completion.  But the
completion script has functions to deal with both, so this shouldn't
be a problem.

> +
> +   # 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}

Really? ;)

I like the above comments about setting COMP_CWORD, because they
explain why you do what you do, which would be otherwise difficult to
figure out.  But telling that an echo in a for loop over an array
prints that array is, well, probably not necessary.

> +   for i in ${COMPREPLY[@]}; do
> +   echo "$i"
> +   done

There is no need for the loop here to print the array one element per
line:

local IFS=$'\n'
echo "${COMPREPLY[*]}"

> +}
> +
> +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"

Where does the second argument come from?  Below you run this script
as '${__git_tcsh_completion_script} "${COMMAND_LINE}"', i.e. $2 is
never set.  Am I missing something?

> +fi
> diff --git a/contrib/completion/git-completion.tcsh
> b/contrib/completion/git-completion.tcsh
> new file mode 100755
> index 000..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 
> +# 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 u

[PATCH nd/wildmatch] Correct Git's version of isprint and isspace

2012-11-13 Thread Nguyễn Thái Ngọc Duy
Git's ispace does not include 11 and 12. Git's isprint includes
control space characters (10-13). According to glibc-2.14.1 on C
locale on Linux, this is wrong. This patch fixes it.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I wrote a small C program to compare the result of all is* functions
 that Git replaces against the libc version. These are the only ones that
 differ. Which matches what Jan Schönherr commented.

 ctype.c   |  6 +++---
 git-compat-util.h | 11 ++-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/ctype.c b/ctype.c
index 0bfebb4..71311a3 100644
--- a/ctype.c
+++ b/ctype.c
@@ -14,11 +14,11 @@ enum {
P = GIT_PATHSPEC_MAGIC, /* other non-alnum, except for ] and } */
X = GIT_CNTRL,
U = GIT_PUNCT,
-   Z = GIT_CNTRL | GIT_SPACE
+   Z = GIT_CNTRL_SPACE
 };
 
-const unsigned char sane_ctype[256] = {
-   X, X, X, X, X, X, X, X, X, Z, Z, X, X, Z, X, X, /*   0.. 15 */
+const unsigned int sane_ctype[256] = {
+   X, X, X, X, X, X, X, X, X, Z, Z, Z, Z, Z, X, X, /*   0.. 15 */
X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, /*  16.. 31 */
S, P, P, P, R, P, P, P, R, R, G, R, P, P, R, P, /*  32.. 47 */
D, D, D, D, D, D, D, D, D, D, P, P, P, P, P, G, /*  48.. 63 */
diff --git a/git-compat-util.h b/git-compat-util.h
index 02f48f6..4ed3f94 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -474,8 +474,8 @@ extern const char tolower_trans_tbl[256];
 #undef ispunct
 #undef isxdigit
 #undef isprint
-extern const unsigned char sane_ctype[256];
-#define GIT_SPACE 0x01
+extern const unsigned int sane_ctype[256];
+#define GIT_CNTRL_SPACE 0x01
 #define GIT_DIGIT 0x02
 #define GIT_ALPHA 0x04
 #define GIT_GLOB_SPECIAL 0x08
@@ -483,9 +483,10 @@ extern const unsigned char sane_ctype[256];
 #define GIT_PATHSPEC_MAGIC 0x20
 #define GIT_CNTRL 0x40
 #define GIT_PUNCT 0x80
-#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
+#define GIT_SPACE 0x100
+#define sane_istest(x,mask) ((sane_ctype[(unsigned int)(x)] & (mask)) != 0)
 #define isascii(x) (((x) & ~0x7f) == 0)
-#define isspace(x) sane_istest(x,GIT_SPACE)
+#define isspace(x) sane_istest(x,GIT_SPACE | GIT_CNTRL_SPACE)
 #define isdigit(x) sane_istest(x,GIT_DIGIT)
 #define isalpha(x) sane_istest(x,GIT_ALPHA)
 #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
@@ -493,7 +494,7 @@ extern const unsigned char sane_ctype[256];
 #define isupper(x) sane_iscase(x, 0)
 #define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)
 #define is_regex_special(x) sane_istest(x,GIT_GLOB_SPECIAL | GIT_REGEX_SPECIAL)
-#define iscntrl(x) (sane_istest(x,GIT_CNTRL))
+#define iscntrl(x) (sane_istest(x,GIT_CNTRL | GIT_CNTRL_SPACE))
 #define ispunct(x) sane_istest(x, GIT_PUNCT | GIT_REGEX_SPECIAL | \
GIT_GLOB_SPECIAL | GIT_PATHSPEC_MAGIC)
 #define isxdigit(x) (hexval_table[x] != -1)
-- 
1.8.0.rc2.23.g1fb49df

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCHv4] replace: parse revision argument for -d

2012-11-13 Thread Michael J Gruber
'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 
---

Notes:
v4 names the aux variable more concisely and does away with a superfluous
assignment.
 builtin/replace.c  | 15 +--
 t/t6050-replace.sh | 11 +++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index e3aaf70..398ccd5 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -46,24 +46,27 @@ 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, *full_hex;
char ref[PATH_MAX];
int had_error = 0;
unsigned char sha1[20];
 
for (p = argv; *p; p++) {
-   if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
-   >= sizeof(ref)) {
-   error("replace ref name too long: %.*s...", 50, *p);
+   if (get_sha1(*p, sha1)) {
+   error("Failed to resolve '%s' as a valid ref.", *p);
had_error = 1;
continue;
}
+   full_hex = sha1_to_hex(sha1);
+   snprintf(ref, sizeof(ref), "refs/replace/%s", full_hex);
+   /* read_ref() may reuse the buffer */
+   full_hex = ref + strlen("refs/replace/");
if (read_ref(ref, sha1)) {
-   error("replace ref '%s' not found.", *p);
+   error("replace ref '%s' not found.", full_hex);
had_error = 1;
continue;
}
-   if (fn(*p, ref, sha1))
+   if (fn(full_hex, ref, sha1))
had_error = 1;
}
return had_error;
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c87f28..decdc33 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -140,6 +140,17 @@ test_expect_success '"git replace" replacing' '
  test "$HASH2" = "$(git replace)"
 '
 
+test_expect_success '"git replace" resolves sha1' '
+ SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&
+ git replace -d $SHORTHASH2 &&
+ git replace $SHORTHASH2 $R &&
+ git show $HASH2 | grep "O Thor" &&
+ test_must_fail git replace $HASH2 $R &&
+ git replace -f $HASH2 $R &&
+ test_must_fail git replace -f &&
+ test "$HASH2" = "$(git replace)"
+'
+
 # This creates a side branch where the bug in H2
 # does not appear because P2 is created by applying
 # H2 and squashing H5 into it.
-- 
1.8.0.311.gdd08018

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)

2012-11-13 Thread Michael J Gruber
Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
> '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 
> ---
> 
> Notes:
> v3 safeguards the hex buffer against reuse
>  builtin/replace.c  | 16 ++--
>  t/t6050-replace.sh | 11 +++
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/replace.c b/builtin/replace.c

By the way - Junio, is that the intented outcome of "format-patch
--notes"? I would rather put the newline between the note and the
diffstat (and omit the one after the ---) but may have goofed up a rebase:

...

Signed-off-by: Michael J Gruber 
---
Notes:
v3 safeguards the hex buffer against reuse

 builtin/replace.c  | 16 ++--
...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCHv3] replace: parse revision argument for -d

2012-11-13 Thread Michael J Gruber
Jeff King venit, vidit, dixit 12.11.2012 21:42:
> 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 
>> ---
>>
>> 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?

That would be sooo inappropriate! ;)

Maybe "full_hex"?

I should also do away with the first replacement which really made sense
in v1 only.

v4 to follow.

Michael
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


  1   2   >