Re: [PATCH 1/2] transport-helper: report errors properly

[Felipe Contreras]

On Sat, Apr 13, 2013 at 1:00 AM, Jeff King  wrote:
> On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:
>
>> To me, the reality is obvious: my patch didn't require such a big
>> commit message, the short version was fine, the only reason Jeff King
>> insisted on a longer version is because the patch came from me.
>
> Get over yourself. The reason I suggested a longer commit message for
> your commit is because after spending several hours figuring out what
> the current code did, and what it should be doing instead, I wanted to
> document that effort so that I and other readers did not have to do it
> again later. I didn't even review the other patch you mention, so I
> could not possibly have come to the same point with it.

The double standard might not come from you, perhaps you subject all
the patches you review to the same standard, it comes from the fact
that the patches you review have an unfair disadvantage.

> But hey, if you want to have paranoid fantasies that I'm persecuting you
> (by writing the longer commit messages for you!), go ahead.

You don't persecute me, you persecute my patches. I could almost
picture the moment you see a patch is coming from me, you have already
decided to rewrite the commit message, even before reading it. Antoine
is not me, so you simply didn't review that patch.

> If you don't want me to review your patches, that's fine by me, too; our
> discussions often end up frustrating, and it's clear we do not agree on
> very much with respect to process or design. But if you don't want that,
> please stop cc'ing me when you send out the patches.

This comment was directed towards Junio, I do hope he is able to see
the double standard. As for you, I think your reviews have value, but
I also think you dwelling in irrelevant details do slow things down,
which is not too bad, what is bad is that you assume that your
opinions are facts (e.g. the commit message need to be bigger), and
get angry when somebody disagrees with them.

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: [BUG] shallow clones over http

[Jeff King]

On Wed, Apr 10, 2013 at 12:48:51PM +, Tom wrote:

> The problem occurs to me also when I want to "deepen" a shallow clone of
> MediaWiki via https://
> 
>  git clone --depth 1 https://gerrit.wikimedia.org/r/p/mediawiki/core.git
>  git pull --depth=9
> 
> fatal: git fetch-pack: expected shallow list.
> 
> Perhaps it helps someone to find the reason.
> 
> UPDATE:
> 
> This however works
> 
>  git clone --depth=2 https://gerrit.wikimedia.org/r/p/mediawiki/core.git
>  git fetch --depth=5

Yes, I experienced the same thing. I don't think the problem is related
to the depth per se, but rather the particular pattern of ACKs that
upload-pack sends based on which commits are being selected.

-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 1/2] transport-helper: report errors properly

[Jeff King]

On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote:

> To me, the reality is obvious: my patch didn't require such a big
> commit message, the short version was fine, the only reason Jeff King
> insisted on a longer version is because the patch came from me.

Get over yourself. The reason I suggested a longer commit message for
your commit is because after spending several hours figuring out what
the current code did, and what it should be doing instead, I wanted to
document that effort so that I and other readers did not have to do it
again later. I didn't even review the other patch you mention, so I
could not possibly have come to the same point with it.

But hey, if you want to have paranoid fantasies that I'm persecuting you
(by writing the longer commit messages for you!), go ahead.

If you don't want me to review your patches, that's fine by me, too; our
discussions often end up frustrating, and it's clear we do not agree on
very much with respect to process or design. But if you don't want that,
please stop cc'ing me when you send out the patches.

-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 2/2] transport-helper: update remote helper namespace

[Felipe Contreras]

Hi,

Why wasn't this patch merged to 'pu'? To my knowledge nobody raised
any real concerns.

Should I explain in every commit that touches transport-helper how
remote-helpers without marks are impossible? I know I said I was going
to update the commit message, but I don't think that reason to not put
it in 'pu'. Also, the only reason I said so was to make Jeff happy,
but now that I think again, it doesn't really belong there; remote
helpers cannot be using these refs, they just can't. They cannot work
without marks, it's not possible. To think otherwise is simply a
mistake.

On Thu, Apr 11, 2013 at 12:18 AM, Felipe Contreras
 wrote:
> On Thu, Apr 11, 2013 at 12:05 AM, Jeff King  wrote:
>> On Wed, Apr 10, 2013 at 11:53:38PM -0500, Felipe Contreras wrote:
>>
>>> > But if we push some commits to the helper, moving Y up to Z, then it
>>> > would build the new commit (which contains the foreign-vcs's equivalent of
>>> > Y..Z) on top of Z, not Y.
>>>
>>> Why would it do that? If X points to say revision 100, presumably it
>>> was stored somewhere while doing a fetch. Similarly, if foreign
>>> version of Z is 150, it can update that number while doing a push. The
>>> next fetch it would start from 151.
>>
>> I think the only reason not to bump the marker forward during the push
>> would be if the helper wants for some reason to "re-import" from the
>> foreign source rather than accepting the git versions of the commits.
>> Something like git-svn's markup of the commit messages with revision ids
>> comes to mind.
>
> Yeah, but that's already a second level hypothesis. First,
> remote-helpers would need to be able to work without marks, and they
> can't.
>
>> But if it matters, then by definition that would mean
>> that the import/export is not bidirectionally clean.
>
> I don't see how would that matter.
>
>> So I can buy the argument that bumping it forward ourselves will not
>> matter for any well-implemented helper.
>
> Or any helper.
>
>> That is the sort of thing that might be helpful to include in the commit
>> message; if somebody does run across such a helper and bisects to your
>> commit, then they can understand the rationale for the decision.
>
> If it did matter, it would be mentioned. I will updated it later if
> there's no further comments.

-- 
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: [PATCH 1/2] transport-helper: report errors properly

[Felipe Contreras]

On Thu, Apr 11, 2013 at 6:05 PM, Junio C Hamano  wrote:
> Felipe Contreras  writes:
>
>> And if you must, you might was well label them with "REMINDER", no,
>> wait, that's what "TODO" comments are for, where people can see them,
>> and not *forget* them.
>
> Yeah, good point.

Moreover, I think there's a clear double standard. Consider this commit:

commit 99d3206010ba1fcc9311cbe8376c0b5e78f4a136
Author: Antoine Pelisse 
Date:   Sat Mar 23 18:23:28 2013 +0100

combine-diff: coalesce lost lines optimally

This replaces the greedy implementation to coalesce lost lines by using
dynamic programming to find the Longest Common Subsequence.

The O(n²) time complexity is obviously bigger than previous
implementation but it can produce shorter diff results (and most likely
easier to read).

List of lost lines is now doubly-linked because we reverse-read it when
reading the direction matrix.

The commit message is 9 lines, and the diffstat 320 insertions(+), 64
deletions(-). Moreover, there are some important bits of information
on the mailing list that never made it to the commit message:

---
Best-case analysis:
All p parents have the same n lines.
We will find LCS and provide a n lines (the same lines) new list in
O(n²), and then run it again in O(n²) with the next parent, etc.
It will end-up being O(pn²).

Worst-case analysis:
All p parents have no lines in common.
We will find LCS and provide a 2n new list in O(n²).
Then we run it again in O(2n x n), and again O(3n x n), etc, until
O(pn x n).
When we sum these all, we end-up with O(p² x n²)
---

---
Unfortunately on a commit that would remove A LOT of lines (1)
from 7 parents, the times goes from 0.01s to 1.5s... I'm pretty sure
that scenario is quite uncommon though.
---

This is not mentioned in the commit message; on which situations this
implementation would be worst and why it's OK either way.

---
As you can see the last test is broken because the solution is not
optimal for more than two parents. It would probably require to extend
the dynamic programming to a k-dimension matrix (for k parents) but the
result would end-up being O(n^k) (when removing n consecutives lines
from p parents). I'm not sure there is any better solution known yet to
the k-LCS problem.
Implementing the dynamic solution with the k-dimension matrix would
probably require to re-hash the strings (I guess it's already done by
xdiff), as the number of string comparisons would increase.
---

The fact that the last test is broken is not mentioned at all.

Now let's compare to the final version of my patch which is 19 lines
40 insertions(+), 1 deletion(-). The ration of commit message lines
vs. code changed lines is 19/41(0.46) whereas Antoine's patch is
3/128(0.02), a difference of over 19 times. Granted, some single-line
changes do require a good chunk of explanation, but this is not one of
them; this single line patch doesn't even change the behavior of the
code, simply changes a silent error exit to a verbose error exit,
that's all. Antoine's patch has a lot more potential to trigger
something unexpected.

And the chances that somebody would have to look at Antoine's patch is
quite high, especially since a failing test-case is introduced. The
chances that anybody would look at mine are very very low.

So either Antoine's commit message was fine, and so was mine, or it
was sorely lacking explanation.

To me, the reality is obvious: my patch didn't require such a big
commit message, the short version was fine, the only reason Jeff King
insisted on a longer version is because the patch came from me.
Antoine's patch might have benefited from a little more explanation,
but not every issue that was discussed in the mailing list was
necessary (in my patch virtually every issue discussed was added to
the commit message).

This is the definition of double standard.

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: [ITCH] Specify refspec without remote

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> When pushing into other kinds of repositories (e.g. you can update
> some but not all of the branches, or you want to touch only some of
> them and not others even if you have enough privilege to update any
> of them) or when you do not "batch" and push out one branch as work
> on it is done, while other branches that you would eventually
> publish are still not ready, "matching" is not for you.

I agree that we need to get a "batching" push.default corresponding to
"matching" for multiple-remote setups.  However, I think we should
hold it off until my implicit-push patch is finished.  After using it
for a few days, I'll get a good idea about what this new push.default
setting should look like.

> If "implicit-push" branch at "ram" is updated by other people and
> you may have to pull back from, you would need this for "git pull"
> (without arguments) while on that branch, I guess.  But I got the
> impression from your scenario that "ram" won't be updated by anybody
> but you.
>
> So I am guessing that this may not be needed.

In my opinion, it is a fundamental mistake to have more than one
person working on a branch.  There is one exception to this rule: it
is alright when there are only two people working on it, and one of
them is a "reliable fast-forward-only read-only upstream".  Let me
illustrate this with an example: I sometimes find myself working on
the master branch of git.git (fetch from origin: git/git.git, publish
to ram: artagnon/git.git).  This is because origin/master is an
"reliable fast-forward-only read-only upstream" (read-only in the
sense that it can only be updated with a git fetch).  My interaction
with it is limited to 'git rebase origin/master' on the master branch.
 I will never find myself manipulating it, and the rebase will never
fail unless my patches conflict with the new upstream.

As to why the setting is needed: I often work on more than one device*,
and I suspect a lot of people do this today.  I always fetch all
changes on my private branches before beginning work, unless I want to
end up in a confusing mess (I often rewrite history).

> This becomes necessary only if you use push.default set to "current"
> (or "upstream").  If you mistakenly say "git push" (no other
> arguments), without this configuration you will end up pushing the
> branch out.

Right.  The objective is to get 'git push' to _always_ DTRT.

> It may be that adding push.default=current-but-do-not-create-anew
> could help.  It is a cross between 'matching' and 'current', to say
> "consider pushing out the current one, but only when the other side
> already has one", and may help people who do not "batch".

Hm.  I would argue that exploding push.default options is unhealthy,
and that we should move towards thinking of more fine-grained control
with different orthogonal options.  I'll first do it for pull
(autostash has been in progress for some time); then we can port the
relevant options to push.

* I still haven't made much progress on a design for config-sharing.  I
think I'm missing something big.
--
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: [RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The primary reason is the confusion factor Jeff mentioned in the
> thread that inspired this patch.  People would realize it is very
> natural to decide where to push to based on what branch is being
> pushed, but only after they think it long and hard enough [*1*].  I
> suspect that it is an equally natural expectation for casual users
> that the destination is chosen based on the current branch, if only
> because that is what they are used to seeing when they say "git
> push" without any argument.

I agree with you largely, but I would still argue that choosing a
destination based on the current branch is a historical mistake made
by "matching".  We don't have to be stuck with this historical
mistake, because this is a new syntax: when users read about it in the
documentation/ What's New in git.git type email, they will also learn
that it chooses the destination based on the refspec.

> Even though I personally am in favor of this "destination is tied to
> what is pushed out", not "destination is chosen based on the current
> branch", I can understand why some people would prefer the latter,
> and why they find it simpler and easier to explain.

Agreed.  This is a consequence of not introducing triangular workflows
earlier, and getting our users used to distributed workflows.  With
this patch, users must mandatorily know about remote.pushdefault and
branch..pushremote, if they want to work in multiple-remote
scenarios.  My argument for that is that multiple-remote workflows
have always been a hack until now, and users of that setup will thank
us for fixing this.

> The second reason is purely on the differences between what the
> above clean-nice explanation says and what the patch actually does.
>
> I think "is-possible-refspec" and "pushremote-get-for-refspec" are
> both way over-engineered, even for people who agree with me and the
> above introduction for this change to favor "destination depends on
> what branch is pushed out".  If is-possible-refspec is replaced with
> a much simpler to understand logic, "Is this a local branch name?",
> possibly combined with "There is no such path on the filesystem" and
> "It's not a defined remote" (iow, reject "git push master:next" and
> anything more complex) [*2*], I suspect it would be a bit more
> sellable.

I don't feel strongly either way, as I just want a simple 'git push
master next +pu' to DTRT.  I rarely, if ever, specify the : part
of the refpsec.  Just so we're clear, we want:

- In git push master, master is verified not to be a path on the
filesystem, not a remote, and finally a local branch.

- In git push master:next, master:next is interpreted as a destination.

- In git push master next:pu, master is verified as usual, and next:pu
is pushed to the remote specified by next.  My patch currently does
this (checks that  and  are branches).

- In git push master next:refs/tags/v3.1 and git push master
v3.1:refs/heads/next, master is verified as usual and the refspec is
pushed to the remote specified by remote.pushdefault, falling back to
origin (since the  is not a branch).  My patch currently pushes
it to the current branch's configuration, and I've already marked it
as a TODO.
--
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] i18n: branch: mark strings for translation

[Duy Nguyen]

On Sat, Apr 13, 2013 at 12:22 PM, Jiang Xin  wrote:
> diff --git a/branch.c b/branch.c
> index 6ae6a..c8745 100644
> else
> -   die("BUG: impossible combination of %d and %p",
> +   die(_("BUG: impossible combination of %d and %p"),
> remote_is_branch, origin);
> }
>  }

You might want to leave this out. I can hardly happen in real life. If
it is, the unfortunate user just needs to copy it straight to
git@vger.
--
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: [PATCH v2] config: allow inaccessible configuration under $HOME

[Mike Galbraith]

Tested, original setup works fine.

On Fri, 2013-04-12 at 14:03 -0700, Jonathan Nieder wrote: 
> The changes v1.7.12.1~2^2~4 (config: warn on inaccessible files,
> 2012-08-21) and v1.8.1.1~22^2~2 (config: treat user and xdg config
> permission problems as errors, 2012-10-13) were intended to prevent
> important configuration (think "[transfer] fsckobjects") from being
> ignored when the configuration is unintentionally unreadable (for
> example with EIO on a flaky filesystem, or with ENOMEM due to a DoS
> attack).  Usually ~/.gitconfig and ~/.config/git are readable by the
> current user, and if they aren't then it would be easy to fix those
> permissions, so the damage from adding this check should have been
> minimal.
> 
> Unfortunately the access() check often trips when git is being run as
> a server.  A daemon (such as inetd or git-daemon) starts as "root",
> creates a listening socket, and then drops privileges, meaning that
> when git commands are invoked they cannot access $HOME and die with
> 
>  fatal: unable to access '/root/.config/git/config': Permission denied
> 
> Any patch to fix this would have one of three problems:
> 
>   1. We annoy sysadmins who need to take an extra step to handle HOME
>  when dropping privileges (the current behavior, or any other
>  proposal that they have to opt into).
> 
>   2. We annoy sysadmins who want to set HOME when dropping privileges,
>  either by making what they want to do impossible, or making them
>  set an extra variable or option to accomplish what used to work
>  (e.g., a patch to git-daemon to set HOME when --user is passed).
> 
>   3. We loosen the check, so some cases which might be noteworthy are
>  not caught.
> 
> This patch is of type (3).
> 
> Treat user and xdg configuration that are inaccessible due to
> permissions (EACCES) as though no user configuration was provided at
> all.
> 
> An alternative method would be to check if $HOME is readable, but that
> would not help in cases where the user who dropped privileges had a
> globally readable HOME with only .config or .gitconfig being private.
> 
> This does not change the behavior when /etc/gitconfig or .git/config
> is unreadable (since those are more serious configuration errors),
> nor when ~/.gitconfig or ~/.config/git is unreadable due to problems
> other than permissions.
> 
> Signed-off-by: Jonathan Nieder 
> Improved-by: Jeff King 
> ---
> Jonathan Nieder wrote:
> 
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)
> > warning(_("unable to access '%s': %s"), path, strerror(errno));
> >  }
> >  
> > +static int access_error_is_ok(int err, unsigned flag)
> > +{
> > +   return errno == ENOENT || errno == ENOTDIR ||
> 
> Sigh, I can't spell "err".  Here's a v2 incorporating that change and
> with commit message incorporating the latest discussion.
> 
>  builtin/config.c  |  4 ++--
>  config.c  | 10 +-
>  dir.c |  4 ++--
>  git-compat-util.h |  5 +++--
>  wrapper.c | 14 ++
>  5 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index 33c9bf9..19ffcaf 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char 
> *prefix)
>*/
>   die("$HOME not set");
>  
> - if (access_or_warn(user_config, R_OK) &&
> - xdg_config && !access_or_warn(xdg_config, R_OK))
> + if (access_or_warn(user_config, R_OK, 0) &&
> + xdg_config && !access_or_warn(xdg_config, R_OK, 0))
>   given_config_file = xdg_config;
>   else
>   given_config_file = user_config;
> diff --git a/config.c b/config.c
> index aefd80b..830ee14 100644
> --- a/config.c
> +++ b/config.c
> @@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct 
> config_include_data *inc
>   path = buf.buf;
>   }
>  
> - if (!access_or_die(path, R_OK)) {
> + if (!access_or_die(path, R_OK, 0)) {
>   if (++inc->depth > MAX_INCLUDE_DEPTH)
>   die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
>   cf && cf->name ? cf->name : "the command line");
> @@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const 
> char *repo_config)
>  
>   home_config_paths(&user_config, &xdg_config, "config");
>  
> - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
> + if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 
> 0)) {
>   ret += git_config_from_file(fn, git_etc_gitconfig(),
>   data);
>   found += 1;
>   }
>  
> - if (xdg_config && !access_or_die(xdg_config, R_OK)) {
> + if (xdg_config && !access_or_die(xdg_config, R_OK, ACC

Re: [PATCH] submodule foreach: Added in --post-order= and adjusted code per Jens Lehmann's suggestions

[Eric Cousineau]

Had accidentally sent this as HTML, resending as plain-text.

On Fri, Apr 12, 2013 at 11:09 PM, Eric Cousineau  wrote:
>
> Oops... I tried out using git-send-email adding in the Message-Id, but forgot 
> to change the title as well. My bad.
>
> This was in response to:
>
> [PATCH/RFC] Changing submodule foreach --recursive to be depth-first, 
> --parent option to execute command in supermodule as well
> Message-Id: <515b3c0e.9000...@web.de>
>
> - Eric
>
>
> On Fri, Apr 12, 2013 at 11:04 PM, eacousineau  wrote:
>>
>> Signed-off-by: eacousineau 
>> ---
>> I see what you meant by the extra variables, so I've fixed that so the
>> original flags aren't needed with recursion. Also updated it to not
>> print the entering command if there is only a post-order command.
>>
>> Examples:
>>
>> $ git submodule foreach --recursive --post-order 'echo Goodbye' "echo 
>> \"'ello\""
>> Entering 'b'
>> 'ello
>> Entering 'b/d'
>> 'ello
>> Leaving 'b/d'
>> Goodbye
>> Leaving 'b'
>> Goodbye
>> Entering 'c'
>> 'ello
>> Leaving 'c'
>> Goodbye
>>
>> $ git submodule foreach --recursive --post-order :
>> Leaving 'b/d'
>> Leaving 'b'
>> Leaving 'c'
>>
>>  git-submodule.sh | 31 ++-
>>  1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 79bfaac..e08a724 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -11,7 +11,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name 
>> ] [--reference > or: $dashless [--quiet] deinit [-f|--force] [--] ...
>> or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
>> [-f|--force] [--rebase] [--reference ] [--merge] [--recursive] 
>> [--] [...]
>> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
>> [commit] [--] [...]
>> -   or: $dashless [--quiet] foreach [--recursive] 
>> +   or: $dashless [--quiet] foreach [--recursive] [--post-order ] 
>> 
>> or: $dashless [--quiet] sync [--recursive] [--] [...]"
>>  OPTIONS_SPEC=
>>  . git-sh-setup
>> @@ -449,6 +449,15 @@ cmd_foreach()
>> --recursive)
>> recursive=1
>> ;;
>> +   --post-order)
>> +   test "$#" = "1" && usage
>> +   post_order="$2"
>> +   shift
>> +   ;;
>> +   --post-order=*)
>> +   # Will skip empty commands
>> +   post_order=${1#*=}
>> +   ;;
>> -*)
>> usage
>> ;;
>> @@ -471,7 +480,9 @@ cmd_foreach()
>> die_if_unmatched "$mode"
>> if test -e "$sm_path"/.git
>> then
>> -   say "$(eval_gettext "Entering '\$prefix\$sm_path'")"
>> +   enter_msg="$(eval_gettext "Entering 
>> '\$prefix\$sm_path'")"
>> +   leave_msg="$(eval_gettext "Leaving 
>> '\$prefix\$sm_path'")"
>> +   die_msg="$(eval_gettext "Stopping at '\$sm_path'; 
>> script returned non-zero status.")"
>> name=$(module_name "$sm_path")
>> (
>> prefix="$prefix$sm_path/"
>> @@ -479,13 +490,23 @@ cmd_foreach()
>> # we make $path available to scripts ...
>> path=$sm_path
>> cd "$sm_path" &&
>> -   eval "$@" &&
>> +   if test $# -gt 0 -o -z "$post_order"
>> +   then
>> +   say "$enter_msg" &&
>> +   eval "$@" || die "$die_msg"
>> +   fi &&
>> if test -n "$recursive"
>> then
>> -   cmd_foreach "--recursive" "$@"
>> +   # subshell will use parent-scoped 
>> values
>> +   cmd_foreach "$@"
>> +   fi &&
>> +   if test -n "$post_order"
>> +   then
>> +   say "$leave_msg" &&
>> +   eval "$post_order" || die "$die_msg"
>> fi
>> ) <&3 3<&- ||
>> -   die "$(eval_gettext "Stopping at '\$sm_path'; script 
>> returned non-zero status.")"
>> +   die "$die_msg"
>> fi
>> done
>>  }
>> --
>> 1.8.2.1.390.gd4ee029
>>
>
--
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] submodule foreach: Added in --post-order= and adjusted code per Jens Lehmann's suggestions

[eacousineau]

Signed-off-by: eacousineau 
---
I see what you meant by the extra variables, so I've fixed that so the
original flags aren't needed with recursion. Also updated it to not
print the entering command if there is only a post-order command.

Examples:

$ git submodule foreach --recursive --post-order 'echo Goodbye' "echo \"'ello\""
Entering 'b'
'ello
Entering 'b/d'
'ello
Leaving 'b/d'
Goodbye
Leaving 'b'
Goodbye
Entering 'c'
'ello
Leaving 'c'
Goodbye

$ git submodule foreach --recursive --post-order :
Leaving 'b/d'
Leaving 'b'
Leaving 'c'

 git-submodule.sh | 31 ++-
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..e08a724 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -11,7 +11,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name 
] [--reference ...
or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--rebase] [--reference ] [--merge] [--recursive] [--] 
[...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
-   or: $dashless [--quiet] foreach [--recursive] 
+   or: $dashless [--quiet] foreach [--recursive] [--post-order ] 

or: $dashless [--quiet] sync [--recursive] [--] [...]"
 OPTIONS_SPEC=
 . git-sh-setup
@@ -449,6 +449,15 @@ cmd_foreach()
--recursive)
recursive=1
;;
+   --post-order)
+   test "$#" = "1" && usage
+   post_order="$2"
+   shift
+   ;;
+   --post-order=*)
+   # Will skip empty commands
+   post_order=${1#*=}
+   ;;
-*)
usage
;;
@@ -471,7 +480,9 @@ cmd_foreach()
die_if_unmatched "$mode"
if test -e "$sm_path"/.git
then
-   say "$(eval_gettext "Entering '\$prefix\$sm_path'")"
+   enter_msg="$(eval_gettext "Entering 
'\$prefix\$sm_path'")"
+   leave_msg="$(eval_gettext "Leaving 
'\$prefix\$sm_path'")"
+   die_msg="$(eval_gettext "Stopping at '\$sm_path'; 
script returned non-zero status.")"
name=$(module_name "$sm_path")
(
prefix="$prefix$sm_path/"
@@ -479,13 +490,23 @@ cmd_foreach()
# we make $path available to scripts ...
path=$sm_path
cd "$sm_path" &&
-   eval "$@" &&
+   if test $# -gt 0 -o -z "$post_order"
+   then
+   say "$enter_msg" &&
+   eval "$@" || die "$die_msg"
+   fi &&
if test -n "$recursive"
then
-   cmd_foreach "--recursive" "$@"
+   # subshell will use parent-scoped values
+   cmd_foreach "$@"
+   fi &&
+   if test -n "$post_order"
+   then
+   say "$leave_msg" &&
+   eval "$post_order" || die "$die_msg"
fi
) <&3 3<&- ||
-   die "$(eval_gettext "Stopping at '\$sm_path'; script 
returned non-zero status.")"
+   die "$die_msg"
fi
done
 }
-- 
1.8.2.1.390.gd4ee029

--
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/2] doc/http-backend: match query-string in apache half-auth example

[Jeff King]

When setting up a "half-auth" repository in which reads can
be done anonymously but writes require authentication, it is
best if the server can require authentication for both the
ref advertisement and the actual receive-pack POSTs. This
alleviates the need for the admin to set http.receivepack in
the repositories, and means that the client is challenged
for credentials immediately, instead of partway through the
push process (and git clients older than v1.7.11.7 had
trouble handling these challenges).

Since detecting a push during the ref advertisement requires
matching the query string, and this is non-trivial to do in
Apache, we have traditionally punted and instructed users to
just protect "/git-receive-pack$".  This patch provides the
mod_rewrite recipe to actually match the ref advertisement,
which is preferred.

While we're at it, let's add the recipe to our test scripts
so that we can be sure that it works, and doesn't get broken
(either by our changes or by changes in Apache).

Signed-off-by: Jeff King 
---
This is on top of the jk/doc-http-backend topic.

Note that I have never used this in production, but it was pieced
together from various advice I found on the web, and I did confirm that
it works.

I kind of assume mod-rewrite is everywhere these days, so we could
potentially drop the fallback config completely, as it is likely to
confuse people.

 Documentation/git-http-backend.txt | 32 
 t/lib-httpd/apache.conf| 18 ++
 t/t5541-http-push.sh   | 30 ++
 3 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-http-backend.txt 
b/Documentation/git-http-backend.txt
index cad18ce..e3bcdb5 100644
--- a/Documentation/git-http-backend.txt
+++ b/Documentation/git-http-backend.txt
@@ -80,7 +80,30 @@ To enable anonymous read access but authenticated write 
access,
 
 +
 To enable anonymous read access but authenticated write access,
-require authorization with a LocationMatch directive:
+require authorization for both the initial ref advertisement (which we
+detect as a push via the service parameter in the query string), and the
+receive-pack invocation itself:
++
+
+RewriteCond %{QUERY_STRING} service=git-receive-pack [OR]
+RewriteCond %{REQUEST_URI} /git-receive-pack$
+RewriteRule ^/git/ - [E=AUTHREQUIRED:yes]
+
+
+   Order Deny,Allow
+   Deny from env=AUTHREQUIRED
+
+   AuthType Basic
+   AuthName "Git Access"
+   Require group committers
+   Satisfy Any
+   ...
+
+
++
+If you do not have `mod_rewrite` available to match against the query
+string, it is sufficient to just protect `git-receive-pack` itself,
+like:
 +
 
 
@@ -207,13 +230,6 @@ auth.require = (
 # ...and set up auth.backend here
 
 +
-Note that unlike the similar setup with Apache, we can easily match the
-query string for receive-pack, catching the initial request from the
-client. This means that the server administrator does not have to worry
-about configuring `http.receivepack` for the repositories (the default
-value, which enables it only in the case of authentication, is
-sufficient).
-+
 To require authentication for both reads and writes:
 +
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 938b4cf..542241b 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -40,6 +40,9 @@ ErrorLog error.log
 
LoadModule authz_user_module modules/mod_authz_user.so
 
+
+   LoadModule authz_host_module modules/mod_authz_host.so
+
 
 
 PassEnv GIT_VALGRIND
@@ -110,6 +113,21 @@ SSLEngine On
Require valid-user
 
 
+RewriteCond %{QUERY_STRING} service=git-receive-pack [OR]
+RewriteCond %{REQUEST_URI} /git-receive-pack$
+RewriteRule ^/half-auth-complete/ - [E=AUTHREQUIRED:yes]
+
+
+  Order Deny,Allow
+  Deny from env=AUTHREQUIRED
+
+  AuthType Basic
+  AuthName "Git Access"
+  AuthUserFile passwd
+  Require valid-user
+  Satisfy Any
+
+
 
LoadModule dav_module modules/mod_dav.so
LoadModule dav_fs_module modules/mod_dav_fs.so
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 4086f02..beb00be 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -294,5 +294,35 @@ test_expect_success 'push to auth-only-for-push repo' '
test_cmp expect actual
 '
 
+test_expect_success 'create repo without http.receivepack set' '
+   cd "$ROOT_PATH" &&
+   git init half-auth &&
+   (
+   cd half-auth &&
+   test_commit one
+   ) &&
+   git clone --bare half-auth "$HTTPD_DOCUMENT_ROOT_PATH/half-aut

[PATCH] i18n: branch: mark strings for translation

[Jiang Xin]

Signed-off-by: Jiang Xin 
---
 branch.c | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/branch.c b/branch.c
index 6ae6a..c8745 100644
--- a/branch.c
+++ b/branch.c
@@ -57,7 +57,7 @@ void install_branch_config(int flag, const char *local, const 
char *origin, cons
if (remote_is_branch
&& !strcmp(local, shortname)
&& !origin) {
-   warning("Not setting branch %s as its own upstream.",
+   warning(_("Not setting branch %s as its own upstream."),
local);
return;
}
@@ -79,26 +79,26 @@ void install_branch_config(int flag, const char *local, 
const char *origin, cons
if (flag & BRANCH_CONFIG_VERBOSE) {
if (remote_is_branch && origin)
printf(rebasing ?
-  "Branch %s set up to track remote branch %s from 
%s by rebasing.\n" :
-  "Branch %s set up to track remote branch %s from 
%s.\n",
+  _("Branch %s set up to track remote branch %s 
from %s by rebasing.\n") :
+  _("Branch %s set up to track remote branch %s 
from %s.\n"),
   local, shortname, origin);
else if (remote_is_branch && !origin)
printf(rebasing ?
-  "Branch %s set up to track local branch %s by 
rebasing.\n" :
-  "Branch %s set up to track local branch %s.\n",
+  _("Branch %s set up to track local branch %s by 
rebasing.\n") :
+  _("Branch %s set up to track local branch 
%s.\n"),
   local, shortname);
else if (!remote_is_branch && origin)
printf(rebasing ?
-  "Branch %s set up to track remote ref %s by 
rebasing.\n" :
-  "Branch %s set up to track remote ref %s.\n",
+  _("Branch %s set up to track remote ref %s by 
rebasing.\n") :
+  _("Branch %s set up to track remote ref %s.\n"),
   local, remote);
else if (!remote_is_branch && !origin)
printf(rebasing ?
-  "Branch %s set up to track local ref %s by 
rebasing.\n" :
-  "Branch %s set up to track local ref %s.\n",
+  _("Branch %s set up to track local ref %s by 
rebasing.\n") :
+  _("Branch %s set up to track local ref %s.\n"),
   local, remote);
else
-   die("BUG: impossible combination of %d and %p",
+   die(_("BUG: impossible combination of %d and %p"),
remote_is_branch, origin);
}
 }
@@ -115,7 +115,7 @@ static int setup_tracking(const char *new_ref, const char 
*orig_ref,
int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;
 
if (strlen(new_ref) > 1024 - 7 - 7 - 1)
-   return error("Tracking not set up: name too long: %s",
+   return error(_("Tracking not set up: name too long: %s"),
new_ref);
 
memset(&tracking, 0, sizeof(tracking));
@@ -134,7 +134,7 @@ static int setup_tracking(const char *new_ref, const char 
*orig_ref,
}
 
if (tracking.matches > 1)
-   return error("Not tracking: ambiguous information for ref %s",
+   return error(_("Not tracking: ambiguous information for ref 
%s"),
orig_ref);
 
install_branch_config(config_flags, new_ref, tracking.remote,
@@ -179,12 +179,12 @@ int validate_new_branchname(const char *name, struct 
strbuf *ref,
int force, int attr_only)
 {
if (strbuf_check_branch_ref(ref, name))
-   die("'%s' is not a valid branch name.", name);
+   die(_("'%s' is not a valid branch name."), name);
 
if (!ref_exists(ref->buf))
return 0;
else if (!force && !attr_only)
-   die("A branch named '%s' already exists.", ref->buf + 
strlen("refs/heads/"));
+   die(_("A branch named '%s' already exists."), ref->buf + 
strlen("refs/heads/"));
 
if (!attr_only) {
const char *head;
@@ -192,7 +192,7 @@ int validate_new_branchname(const char *name, struct strbuf 
*ref,
 
head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-   die("Cannot force update the current branch.");
+   die(_("Cannot force update the current branch."));
}
return 1;
 }
@@ -247,7 +247,7 @@ void create_br

Re: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

[Duy Nguyen]

On Fri, Apr 5, 2013 at 6:57 PM, Jakub Narębski  wrote:
>>> +void format_decoration(struct strbuf *sb,
>>> +   const struct commit *commit,
>>> +   int use_color);
>>
>> I think you can fit these on a single line, especially if you drop
>> the unused variable names (they help when there are more than one
>> parameter of the same type to document the order of the arguments,
>> but that does not apply here).  That would help people who run
>> "grep" on the header files without using CTAGS/ETAGS.
>
> Well, I think "int use_color" should be left with variable name,
> don't you think?

I don't care too much about this. If Junio does not respond, I'll
leave the names in place (in one long line).
--
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: [PATCH v2 02/12] pretty: share code between format_decoration and show_decorations

[Duy Nguyen]

Sorry for this late reply. I've been quite busy lately..

On Tue, Apr 2, 2013 at 4:53 AM, Junio C Hamano  wrote:
>> -void show_decorations(struct rev_info *opt, struct commit *commit)
>> +void format_decoration(struct strbuf *sb,
>> +const struct commit *commit,
>> +int use_color)
>>  {
>> - const char *prefix;
>> - struct name_decoration *decoration;
>> + const char *prefix = " (";
>> + struct name_decoration *d;
>
> This renaming of variable from decoration to d seems to make the
> patched result unnecessarily different from the original in
> show_decorations, making it harder to compare.  Intentional?

I think I just happened to reuse the style of the old
format_decoration(). Will reuse the name "decoration" instead.

>>   const char *color_commit =
>> - diff_get_color_opt(&opt->diffopt, DIFF_COMMIT);
>> + diff_get_color(use_color, DIFF_COMMIT);
>>   const char *color_reset =
>> - decorate_get_color_opt(&opt->diffopt, DECORATION_NONE);
>> + decorate_get_color(use_color, DECORATION_NONE);
>> +
>> + load_ref_decorations(DECORATE_SHORT_REFS);
>
> In cmd_log_init_finish(), we have loaded decorations with specified
> decoration_style already.  Why is this needed (and with a hardcoded
> style that may be different from what the user specified)?

legacy from pretty.c:format_decoration(). Will move it to the caller
format_commit_one.

>
>> + d = lookup_decoration(&name_decoration, &commit->object);
>> + if (!d)
>> + return;
>> + while (d) {
>> + strbuf_addstr(sb, color_commit);
>> + strbuf_addstr(sb, prefix);
>> + strbuf_addstr(sb, decorate_get_color(use_color, d->type));
>> + if (d->type == DECORATION_REF_TAG)
>> + strbuf_addstr(sb, "tag: ");
>> + strbuf_addstr(sb, d->name);
>> + strbuf_addstr(sb, color_reset);
>> + prefix = ", ";
>> + d = d->next;
>> + }
>> + if (prefix[0] == ',') {
>> + strbuf_addstr(sb, color_commit);
>> + strbuf_addch(sb, ')');
>> + strbuf_addstr(sb, color_reset);
>> + }
>
> Was this change to conditionally close ' (' mentioned in the log
> message?  It is in line with the version taken from pretty.c, and I
> think it may be an improvement, but I do not think the check is
> necessary in the context of this function.  You will never see
> prefix[0] != ',' after the loop, because "while (d)" above runs at
> least once; otherwise the "if (!d) return" would have returned from
> the function early, no?

Yes, your eyeballs have really good quality ;)

>> @@ -625,8 +639,8 @@ void show_log(struct rev_info *opt)
>>   printf(" (from %s)",
>>  find_unique_abbrev(parent->object.sha1,
>> abbrev_commit));
>> + fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
>>   show_decorations(opt, commit);
>> - printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
>
> We used to show and then reset.  I can see the updated
> show_decorations() to format_decoration() callchain always reset at
> the end, so the loss of the final reset here is very sane, but is
> there a need to reset beforehand?  What is the calling convention
> for the updated show_decorations()?  The caller should make sure
> there is no funny colors in effect before calling, and the caller
> can rest assured that there is no funny colors when the function
> returns?

I think it's a sane convention, unless we want a some background color
going through show_decorations.

>> +void format_decoration(struct strbuf *sb,
>> +const struct commit *commit,
>> +int use_color);
>
> I think you can fit these on a single line, especially if you drop
> the unused variable names (they help when there are more than one
> parameter of the same type to document the order of the arguments,
> but that does not apply here).  That would help people who run
> "grep" on the header files without using CTAGS/ETAGS.

No problem.

> Wouldn't it be "format_decorations()", or does it handle only one?

All in one, apparently. Will rename.
--
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

[PATCH v3] checkout: add --ignore-skip-worktree-bits in sparse checkout mode

[Nguyễn Thái Ngọc Duy]

"git checkout -- " is usually used to restore all modified
files in . In sparse checkout mode, this command is overloaded
with another meaning: to add back all files in  that are
excluded by sparse patterns.

As the former makes more sense for day-to-day use. Switch it to the
default and the latter enabled with --ignore-skip-worktree-bits.

While at there, add info/sparse-checkout to gitrepository-layout.txt

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-checkout.txt |  6 ++
 Documentation/gitrepository-layout.txt |  4 
 builtin/checkout.c |  5 +
 t/t1011-read-tree-sparse-checkout.sh   | 24 
 t/t3001-ls-files-others-exclude.sh |  2 +-
 5 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8edcdca..23a9413 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -180,6 +180,12 @@ branch by running "git rm -rf ." from the top level of the 
working tree.
 Afterwards you will be ready to prepare your new files, repopulating the
 working tree, by copying them from elsewhere, extracting a tarball, etc.
 
+--ignore-skip-worktree-bits::
+   In sparse checkout mode, `git checkout -- ` would
+   update only entries matched by  and sparse patterns
+   in $GIT_DIR/info/sparse-checkout. This option ignores
+   the sparse patterns and adds back any files in .
+
 -m::
 --merge::
When switching branches,
diff --git a/Documentation/gitrepository-layout.txt 
b/Documentation/gitrepository-layout.txt
index f0eef76..817337f 100644
--- a/Documentation/gitrepository-layout.txt
+++ b/Documentation/gitrepository-layout.txt
@@ -184,6 +184,10 @@ info/exclude::
'git clean' look at it but the core Git commands do not look
at it.  See also: linkgit:gitignore[5].
 
+info/sparse-checkout::
+   This file stores sparse checkout patterns.
+   See also: linkgit:git-read-tree[1].
+
 remotes::
Stores shorthands for URL and default refnames for use
when interacting with remote repositories via 'git fetch',
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f8033f4..4ed1ee7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -35,6 +35,7 @@ struct checkout_opts {
int force_detach;
int writeout_stage;
int overwrite_ignore;
+   int ignore_skipworktree;
 
const char *new_branch;
const char *new_branch_force;
@@ -278,6 +279,8 @@ static int checkout_paths(const struct checkout_opts *opts,
for (pos = 0; pos < active_nr; pos++) {
struct cache_entry *ce = active_cache[pos];
ce->ce_flags &= ~CE_MATCHED;
+   if (!opts->ignore_skipworktree && ce_skip_worktree(ce))
+   continue;
if (opts->source_tree && !(ce->ce_flags & CE_UPDATE))
/*
 * "git checkout tree-ish -- path", but this entry
@@ -1058,6 +1061,8 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
OPT_STRING(0, "conflict", &conflict_style, N_("style"),
   N_("conflict style (merge or diff3)")),
OPT_BOOLEAN('p', "patch", &opts.patch_mode, N_("select hunks 
interactively")),
+   OPT_BOOL(0, "ignore-skip-worktree-bits", 
&opts.ignore_skipworktree,
+N_("do not limit pathspecs to sparse entries only")),
{ OPTION_BOOLEAN, 0, "guess", &dwim_new_local_branch, NULL,
  N_("second guess 'git checkout no-such-branch'"),
  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
diff --git a/t/t1011-read-tree-sparse-checkout.sh 
b/t/t1011-read-tree-sparse-checkout.sh
index 5c0053a..0c74bee 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -250,4 +250,28 @@ EOF
test_cmp expected actual
 '
 
+test_expect_success 'checkout without --ignore-skip-worktree-bits' '
+   echo "*" >.git/info/sparse-checkout &&
+   git checkout -f top &&
+   test_path_is_file init.t &&
+   echo sub >.git/info/sparse-checkout &&
+   git checkout &&
+   echo modified >> sub/added &&
+   git checkout . &&
+   test_path_is_missing init.t &&
+   git diff --exit-code HEAD
+'
+
+test_expect_success 'checkout with --ignore-skip-worktree-bits' '
+   echo "*" >.git/info/sparse-checkout &&
+   git checkout -f top &&
+   test_path_is_file init.t &&
+   echo sub >.git/info/sparse-checkout &&
+   git checkout &&
+   echo modified >> sub/added &&
+   git checkout --ignore-skip-worktree-bits . &&
+   test_path_is_file init.t &&
+   git diff --exit-code HEAD
+'
+
 test_done
diff --git a/t/t3001-ls-files-others-exclude.sh 
b/t/t3001-ls-files-others-exclude.sh
index efb7ebc..2d274bf 100755
--- a/t/t3001-ls-files-others-exclude.sh
+++ b/t/t3001-ls-files-others-e

Re: [PATCH v1 23/45] check-ignore: convert to use parse_pathspec

[Duy Nguyen]

On Sat, Apr 13, 2013 at 1:03 AM, Adam Spiers  wrote:
>> -static int check_ignore(const char *prefix, const char **pathspec)
>> +static int check_ignore(int argc, const char **argv, const char *prefix)
>>  {
>>   struct dir_struct dir;
>> - const char *path, *full_path;
>>   char *seen;
>>   int num_ignored = 0, dtype = DT_UNKNOWN, i;
>>   struct path_exclude_check check;
>>   struct exclude *exclude;
>> + struct pathspec pathspec;
>>
>>   /* read_cache() is only necessary so we can watch out for submodules. 
>> */
>>   if (read_cache() < 0)
>> @@ -70,31 +70,39 @@ static int check_ignore(const char *prefix, const char 
>> **pathspec)
>>   dir.flags |= DIR_COLLECT_IGNORED;
>>   setup_standard_excludes(&dir);
>>
>> - if (!pathspec || !*pathspec) {
>> + if (!argc) {
>
> Is there a compelling reason for introducing argc as a new parameter
> to check_ignore(), other than simplifying the above line?  And why
> rename the pathspec parameter to argv?  Both these changes are
> misleading AFAICS, since paths provided to check_ignore() can come
> from sources other than CLI arguments (i.e. via --stdin).

Because I introduced "struct pathspec pathspec;" I need to rename the
argument "pathspec" to something else. Maybe we could rename the
argument to "paths"?

> The introduction of argc also makes it possible to invoke
> check_ignore() with arguments which are not self-consistent.

This is the same problem with main() and other places that follow this
convention. But I don't mind dropping argc either.
-- 
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: What's cooking in git.git (Apr 2013, #04; Fri, 12)

[Junio C Hamano]

Jakub Narębski  writes:

> So mc/count-objects-kibibytes is to be discarded, or merged to 'master'?

I asked for opinions, not questions ;-)

I am on the fence, but slightly in favor of dropping it.

I suspect some scripts will get unexpectedly hurt, just like we had
to patch cvsimport.  Among the human users who deeply care about the
output between "kilobytes" and "KiB", nobody would want "684322 KiB"
the "kibibytes" topic would give; people who care will likely to use
"-H" to obtain "668.28 MiB" instead.
--
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] test-bzr: portable shell and utf-8 strings for Mac OS

[Junio C Hamano]

Felipe Contreras  writes:

> On Fri, Apr 12, 2013 at 4:18 PM, Torsten Bögershausen  wrote:
>> Make the shell script more portable:
>> - Split export X=Y into 2 lines
>> - Use printf instead of echo -e
>>
>> Use UTF-8 code points which are not decomposed by the filesystem:
>>  Code points like "á" will be decomposed by Mac OS X.
>>  bzr is unable to find the file "á" on disk.
>>  Use code points from unicode which can not be decomposed.
>>  In other words, the precompsed form use the same bytes as decomposed.
>
> Looks good to me.

Thanks, both. Applied.

--
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 (Apr 2013, #04; Fri, 12)

[Jakub Narębski]

W dniu 12.04.2013 23:40, Junio C Hamano pisze:

> --
> [New Topics]
> * ap/strbuf-humanize (2013-04-10) 2 commits
>  - count-objects: add -H option to humanize sizes
>  - strbuf: create strbuf_humanise_bytes() to show byte sizes
> 
>  Teach "--human-readable" aka "-H" option to "git count-objects" to
>  show various large numbers in Ki/Mi/GiB scaled as necessary.
> 
>  Will merge to 'next'.
> 
>  It may not be a bad idea to discard mc/count-objects-kibibytes,
>  which can introduce regression to scripted users that expect the
>  output to say "N kilobytes".  Opinions?

> --
> [Cooking]
> * mc/count-objects-kibibytes (2013-04-03) 1 commit
>   (merged to 'next' on 2013-04-05 at f4e50e8)
>  + count-objects: output "KiB" instead of "kilobytes"
> 
>  The command reports the total diskspace used to store loose objects
>  in kibibytes, but it was labelled as "kilobytes".  The number now
>  is shown with "KiB", e.g. "6750 objects, 50928 KiB".
> 
>  If you have scripts that decide when to run "git repack" by parsing
>  the output from "git count-objects", this release may break them.
>  Sorry about that.  One of the scripts shipped by git-core itself
>  also had to be adjusted.  You may want to consider updating such
>  scripts to always call "git gc --auto" to let it decide when to
>  repack for you.
> 
>  Will merge to 'master'.

So mc/count-objects-kibibytes is to be discarded, or merged to 'master'?
-- 
Jakub Narębski
--
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: [RFC/PATCH] push: introduce implicit push

[Junio C Hamano]

Ramkumar Ramachandra  writes:

> Currently, there is no way to invoke 'git push' without explicitly
> specifying the destination to push to as the first argument.  When
> pushing several branches, this information is often available in
> branch..pushremote, falling back to branch..remote.  So,
> we can use this information to create a more pleasant push experience.
> You can now do:
>
> $ git push master next pu
>
> If the branches master, next, and pu have different remotes, do_push()
> will be executed three times on the three different remotes.

I am lukewarm on this one, slightly more close to negative than
positive, for a couple of reasons.

The primary reason is the confusion factor Jeff mentioned in the
thread that inspired this patch.  People would realize it is very
natural to decide where to push to based on what branch is being
pushed, but only after they think it long and hard enough [*1*].  I
suspect that it is an equally natural expectation for casual users
that the destination is chosen based on the current branch, if only
because that is what they are used to seeing when they say "git
push" without any argument.

Even though I personally am in favor of this "destination is tied to
what is pushed out", not "destination is chosen based on the current
branch", I can understand why some people would prefer the latter,
and why they find it simpler and easier to explain.

The second reason is purely on the differences between what the
above clean-nice explanation says and what the patch actually does.

I think "is-possible-refspec" and "pushremote-get-for-refspec" are
both way over-engineered, even for people who agree with me and the
above introduction for this change to favor "destination depends on
what branch is pushed out".  If is-possible-refspec is replaced with
a much simpler to understand logic, "Is this a local branch name?",
possibly combined with "There is no such path on the filesystem" and
"It's not a defined remote" (iow, reject "git push master:next" and
anything more complex) [*2*], I suspect it would be a bit more
sellable.


[Footnote]

*1* I've explained in a separate message why basing the destination
on what is being pushed is logical and internally consistent.

*2* This is in the same spirit (not implementation or design) as
revname vs pathspec disambiguation.
--
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] gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM

[Jakub Narębski]

On Fri, 12 April 2013, Junio C Hamano wrote:
> Jonathan Nieder  writes:
>
>>Note that if per-instance configuration file exists, then system-wide
>> -  configuration is _not used at all_.  This is quite untypical and suprising
>> +  configuration is _not used at all_.  This is quite untypical and 
>> surprising
>>behavior.  On the other hand changing current behavior would break 
>> backwards
>>compatibility and can lead to unexpected changes in gitweb behavior.
>>Therefore gitweb also looks for common system-wide configuration file,
>
> Hmm, "atypical", isn't it?
>
> The flow of the text is awkward.  "This is bad. Oh the other hand,
> better is broken. Therefore ..." forces readers to make multiple
> guesses while reading: "ok, bad, so you plan to change it and warn
> us about upcoming change?  oh, not that, changing it is bad, so we
> have to live with it?  oh, not that, there is another one that is
> common and that is what we can use".
>
> It may be a good idea to rewrite this paragraph to avoid such a
> mental roller-coaster in the first place.
>
> The GITWEB_CONFIG_SYSTEM system-wide configuration file is only
> used for instances that lack per-instance configuration file.
> You can use GITWEB_CONFIG_COMMON file to keep common default
> settings that apply to all instances.
>
> or something.
>
> Not asking for a re-roll, but it may be a potential follow-up candidate.

Perhaps something like this?

Note that this change avoids repetition of build / environmental
configuration variable (I think the paragraph above, not touched
in this patch, also might need rewrite).

-- >8 --
Subject: [PATCH] gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM

The flow of the text describing GITWEB_CONFIG_SYSTEM and
GITWEB_CONFIG_COMMON in gitweb/INSTALL is awkward.  "This is
bad. Oh the other hand, better is broken. Therefore ..." forces
readers to make multiple guesses while reading: "ok, bad, so you plan
to change it and warn us about upcoming change?  oh, not that,
changing it is bad, so we have to live with it?  oh, not that, there
is another one that is common and that is what we can use".

Better rewrite said paragraph to avoid such a mental roller-coaster in
the first place.

Signed-off-by: Junio Hamano 
Signed-off-by: Jakub Narebski 
---
 gitweb/INSTALL |   14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/gitweb/INSTALL b/gitweb/INSTALL
index 6d45406..7ad1050 100644
--- a/gitweb/INSTALL
+++ b/gitweb/INSTALL
@@ -243,14 +243,12 @@ for gitweb (in gitweb/README), and gitweb.conf(5) manpage.
   GITWEB_CONFIG_SYSTEM build configuration variable, and override it
   through the GITWEB_CONFIG_SYSTEM environment variable.
 
-  Note that if per-instance configuration file exists, then system-wide
-  configuration is _not used at all_.  This is quite untypical and suprising
-  behavior.  On the other hand changing current behavior would break backwards
-  compatibility and can lead to unexpected changes in gitweb behavior.
-  Therefore gitweb also looks for common system-wide configuration file,
-  normally /etc/gitweb-common.conf (set during build time using build time
-  configuration variable GITWEB_CONFIG_COMMON, set it at runtime using
-  environment variable with the same name).  Settings from per-instance or
+
+  Note that the GITWEB_CONFIG_SYSTEM system-wide configuration file is
+  only used for instances that lack per-instance configuration file.
+  You can use GITWEB_CONFIG_COMMON common system-wide configuration
+  file (normally /etc/gitweb-common.conf) to keep common default
+  settings that apply to all instances.  Settings from per-instance or
   system-wide configuration file override those from common system-wide
   configuration file.
 
-- 
1.7.10.4

--
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: [ITCH] Specify refspec without remote

[Junio C Hamano]

Jeff King  writes:

> Which is still kind of weird, because why should the branch you are on
> affect the default push location? But that is how default "matching" has
> always behaved, and we would remain consistent with that.

I agree that what makes us behave "kind of weird" is that the
current branch is used to look up branch.$name.{remote,pushremote}
when pushing. I do not think "matching" [*1*] has anything to do
with it.

The per-branch configuration, branch.$name.{remote,pushremote}, says
"this branch interacts with a remote that is different from what I
normally interact with".

It is excusable for branch.$name.remote to take the current branch
into account, when it is used to govern the fetch-integrate side
(i.e.  not used as a fall-back for branch.$name.pushremote).  In
order to affect that configured local branch, e.g. "git pull" to
merge other's work, you need to have that named branch checked out
in your working tree.  Triggering the effect of the configuration
based on which branch is checked out makes more sense because of
that reason when you are fetching.

It does not make much sense to use the current branch as the key to
look it up when you are pushing things out. If anything, what is
being pushed out should be what determines where it goes.

But that is a realization that comes after you think the issue long
and hard enough. To a casual end user, I think it is an equally or
even more natural expectation a "git push" would pick the destination
based on what branch you are currently on, as that is what happens
when he runs the command without any argument.


[Footnote]

*1* The "matching" semantics is to support the workflow for people
who batch things up. You perfect _all_ your branches that matter to
the public, and push all of them in one go. If you do not finish a
work on one branch and push out when other branches are not yet
ready, you do not want your push to be limited to the current
branch. And you do not have to "configure" what branches should be
visible to the public. Instead, you have _your_ remote remember it
for you: what are already there are the ones that are updated.

The "current", "upstream", etc. are to support folks who want to
push work done on a single branch out as soon as it is done, even
though the other branches are in no shape to be pushed out.
--
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 (Apr 2013, #04; Fri, 12)

[Junio C Hamano]

What's cooking in git.git (Apr 2013, #04; Fri, 12)
--

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

There are some topics posted on the list that do not appear here,
for various reasons, e.g. simply I ran out of time to pick them up,
it appeared we did not yet have consensus yet, the topic generated
enough comments and likely to be rerolled, or nobody seems to have
been interested in the topic yet. Also some of the new topics are
queued on 'pu' because I thought I'd have time to later review them,
but I actually haven't got around to review them carefully yet (I do
know that there are no malicious change in these topics).

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* po/help-guides (2013-04-03) 5 commits
  (merged to 'next' on 2013-04-04 at 3d99b28)
 + doc: include --guide option description for "git help"
 + help: mention -a and -g option, and 'git help ' usage.
 + builtin/help.c: add list_common_guides_help() function
 + builtin/help.c: add --guide option
 + builtin/help.c: split "-a" processing into two

 "git help" learned "-g" option to show the list of guides just like
 list of commands are given with "-a".


* ap/combine-diff-coalesce-lost (2013-03-25) 1 commit
  (merged to 'next' on 2013-03-29 at f6a05ca)
 + combine-diff: coalesce lost lines optimally

 Attempts to minimize "diff -c/--cc" output by coalescing the same
 lines removed from the parents better, but with an O(n^2)
 complexity.


* js/rerere-forget-protect-against-NUL (2013-04-04) 2 commits
  (merged to 'next' on 2013-04-05 at 426d4e2)
 + rerere forget: do not segfault if not all stages are present
 + rerere forget: grok files containing NUL

 A few bugfixes to "git rerere" working on corner case merge
 conflicts.


* sr/log-SG-no-textconv (2013-04-05) 6 commits
  (merged to 'next' on 2013-04-05 at 7f06945)
 + diffcore-pickaxe: unify code for log -S/-G
 + diffcore-pickaxe: fix leaks in "log -S" and "log -G"
 + diffcore-pickaxe: port optimization from has_changes() to diff_grep()
 + diffcore-pickaxe: respect --no-textconv
 + diffcore-pickaxe: remove fill_one()
 + diffcore-pickaxe: remove unnecessary call to get_textconv()

 "git log -S/-G" started paying attention to textconv filter, but
 there was no way to disable this.  Make it honor --no-textconv
 option.

--
[New Topics]

* po/help-guides (2013-04-12) 1 commit
 - help: mark common_guides[] as translatable

 Finishing touches.
 Will fast-track to 'master'.


* ap/strbuf-humanize (2013-04-10) 2 commits
 - count-objects: add -H option to humanize sizes
 - strbuf: create strbuf_humanise_bytes() to show byte sizes

 Teach "--human-readable" aka "-H" option to "git count-objects" to
 show various large numbers in Ki/Mi/GiB scaled as necessary.

 Will merge to 'next'.

 It may not be a bad idea to discard mc/count-objects-kibibytes,
 which can introduce regression to scripted users that expect the
 output to say "N kilobytes".  Opinions?


* as/clone-reference-with-gitfile (2013-04-09) 2 commits
 - clone: Allow repo using gitfile as a reference
 - clone: Fix error message for reference repository

 "git clone" did not work if a repository pointed at by the
 "--reference" option is a gitfile that points at another place.

 Waiting for comments.


* fc/transport-helper-error-reporting (2013-04-11) 3 commits
 - transport-helper: improve push messages
 - transport-helper: mention helper name when it dies
 - transport-helper: report errors properly

 Rerolled enough times.  In-code comments may want to be further
 extended to explain tricky parts, but seems to be ready otherwise.

 Will merge to 'next'.


* jc/decorate (2013-04-07) 2 commits
 - decorate: add "clear_decoration()"
 - decorate: document API
 (this branch is used by jc/gg.)

 Will discard.


* jc/gg (2013-04-08) 3 commits
 - commit: add get_commit_encoding()
 - commit: rename parse_commit_date()
 - commit: shrink "indegree" field
 (this branch uses jc/decorate.)

 Will discard.


* jk/doc-http-backend (2013-04-11) 2 commits
 - doc/http-backend: give some lighttpd config examples
 - doc/http-backend: clarify "half-auth" repo configuration

 Improve documentation to illustrate "push authenticated, fetch
 anonymous" configuration for smart HTTP servers.

 Will merge to 'next'.


* jk/gitweb-utf8 (2013-04-08) 4 commits
 - gitweb: Fix broken blob action parameters on blob/commitdiff pages
 - gitweb: Don't append ';js=(0|1)' to external links
 - gitweb: Make feed title valid utf8
 - gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, 
and patch

 Various fixes to gitweb.

 Waiting for a reroll after a review.


* jk/su

Re: [PATCH] test-bzr: portable shell and utf-8 strings for Mac OS

[Felipe Contreras]

On Fri, Apr 12, 2013 at 4:18 PM, Torsten Bögershausen  wrote:
> Make the shell script more portable:
> - Split export X=Y into 2 lines
> - Use printf instead of echo -e
>
> Use UTF-8 code points which are not decomposed by the filesystem:
>  Code points like "á" will be decomposed by Mac OS X.
>  bzr is unable to find the file "á" on disk.
>  Use code points from unicode which can not be decomposed.
>  In other words, the precompsed form use the same bytes as decomposed.

Looks good to me.

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

[PATCH] test-bzr: portable shell and utf-8 strings for Mac OS

[Torsten Bögershausen]

Make the shell script more portable:
- Split export X=Y into 2 lines
- Use printf instead of echo -e

Use UTF-8 code points which are not decomposed by the filesystem:
 Code points like "á" will be decomposed by Mac OS X.
 bzr is unable to find the file "á" on disk.
 Use code points from unicode which can not be decomposed.
 In other words, the precompsed form use the same bytes as decomposed.

Signed-off-by: Torsten Bögershausen 
---
 contrib/remote-helpers/test-bzr.sh | 38 +++---
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/contrib/remote-helpers/test-bzr.sh 
b/contrib/remote-helpers/test-bzr.sh
index e800c97..34666e1 100755
--- a/contrib/remote-helpers/test-bzr.sh
+++ b/contrib/remote-helpers/test-bzr.sh
@@ -169,31 +169,30 @@ test_expect_success 'fetch utf-8 filenames' '
   mkdir -p tmp && cd tmp &&
   test_when_finished "cd .. && rm -rf tmp && LC_ALL=C" &&
 
-  export LC_ALL=en_US.UTF-8
-
+  LC_ALL=en_US.UTF-8
+  export LC_ALL
   (
   bzr init bzrrepo &&
   cd bzrrepo &&
 
-  echo test >> "áéíóú" &&
-  bzr add "áéíóú" &&
-  echo test >> "îø∫∆" &&
-  bzr add "îø∫∆" &&
-  bzr commit -m utf-8 &&
-  echo test >> "áéíóú" &&
-  bzr commit -m utf-8 &&
-  bzr rm "îø∫∆" &&
-  bzr mv "áéíóú" "åß∂" &&
-  bzr commit -m utf-8
+  echo test >> "ærø" &&
+  bzr add "ærø" &&
+  echo test >> "ø~?" &&
+  bzr add "ø~?" &&
+  bzr commit -m add-utf-8 &&
+  echo test >> "ærø" &&
+  bzr commit -m test-utf-8 &&
+  bzr rm "ø~?" &&
+  bzr mv "ærø" "ø~?" &&
+  bzr commit -m bzr-mv-utf-8
   ) &&
 
   (
   git clone "bzr::$PWD/bzrrepo" gitrepo &&
   cd gitrepo &&
-  git ls-files > ../actual
+  git -c core.quotepath=false ls-files > ../actual
   ) &&
-
-  echo "\"\\303\\245\\303\\237\\342\\210\\202\"" > expected &&
+  echo "ø~?" > expected &&
   test_cmp expected actual
 '
 
@@ -201,7 +200,8 @@ test_expect_success 'push utf-8 filenames' '
   mkdir -p tmp && cd tmp &&
   test_when_finished "cd .. && rm -rf tmp && LC_ALL=C" &&
 
-  export LC_ALL=en_US.UTF-8
+  LC_ALL=en_US.UTF-8
+  export LC_ALL
 
   (
   bzr init bzrrepo &&
@@ -216,15 +216,15 @@ test_expect_success 'push utf-8 filenames' '
   git clone "bzr::$PWD/bzrrepo" gitrepo &&
   cd gitrepo &&
 
-  echo test >> "áéíóú" &&
-  git add "áéíóú" &&
+  echo test >> "ærø" &&
+  git add "ærø" &&
   git commit -m utf-8 &&
 
   git push
   ) &&
 
   (cd bzrrepo && bzr ls > ../actual) &&
-  echo -e "content\náéíóú" > expected &&
+  printf "content\nærø\n" > expected &&
   test_cmp expected actual
 '
 
-- 
1.8.2.341.g543621f

--
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 v2] config: allow inaccessible configuration under $HOME

[Jonathan Nieder]

The changes v1.7.12.1~2^2~4 (config: warn on inaccessible files,
2012-08-21) and v1.8.1.1~22^2~2 (config: treat user and xdg config
permission problems as errors, 2012-10-13) were intended to prevent
important configuration (think "[transfer] fsckobjects") from being
ignored when the configuration is unintentionally unreadable (for
example with EIO on a flaky filesystem, or with ENOMEM due to a DoS
attack).  Usually ~/.gitconfig and ~/.config/git are readable by the
current user, and if they aren't then it would be easy to fix those
permissions, so the damage from adding this check should have been
minimal.

Unfortunately the access() check often trips when git is being run as
a server.  A daemon (such as inetd or git-daemon) starts as "root",
creates a listening socket, and then drops privileges, meaning that
when git commands are invoked they cannot access $HOME and die with

 fatal: unable to access '/root/.config/git/config': Permission denied

Any patch to fix this would have one of three problems:

  1. We annoy sysadmins who need to take an extra step to handle HOME
 when dropping privileges (the current behavior, or any other
 proposal that they have to opt into).

  2. We annoy sysadmins who want to set HOME when dropping privileges,
 either by making what they want to do impossible, or making them
 set an extra variable or option to accomplish what used to work
 (e.g., a patch to git-daemon to set HOME when --user is passed).

  3. We loosen the check, so some cases which might be noteworthy are
 not caught.

This patch is of type (3).

Treat user and xdg configuration that are inaccessible due to
permissions (EACCES) as though no user configuration was provided at
all.

An alternative method would be to check if $HOME is readable, but that
would not help in cases where the user who dropped privileges had a
globally readable HOME with only .config or .gitconfig being private.

This does not change the behavior when /etc/gitconfig or .git/config
is unreadable (since those are more serious configuration errors),
nor when ~/.gitconfig or ~/.config/git is unreadable due to problems
other than permissions.

Signed-off-by: Jonathan Nieder 
Improved-by: Jeff King 
---
Jonathan Nieder wrote:

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)
>   warning(_("unable to access '%s': %s"), path, strerror(errno));
>  }
>  
> +static int access_error_is_ok(int err, unsigned flag)
> +{
> + return errno == ENOENT || errno == ENOTDIR ||

Sigh, I can't spell "err".  Here's a v2 incorporating that change and
with commit message incorporating the latest discussion.

 builtin/config.c  |  4 ++--
 config.c  | 10 +-
 dir.c |  4 ++--
 git-compat-util.h |  5 +++--
 wrapper.c | 14 ++
 5 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..19ffcaf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
 */
die("$HOME not set");
 
-   if (access_or_warn(user_config, R_OK) &&
-   xdg_config && !access_or_warn(xdg_config, R_OK))
+   if (access_or_warn(user_config, R_OK, 0) &&
+   xdg_config && !access_or_warn(xdg_config, R_OK, 0))
given_config_file = xdg_config;
else
given_config_file = user_config;
diff --git a/config.c b/config.c
index aefd80b..830ee14 100644
--- a/config.c
+++ b/config.c
@@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct 
config_include_data *inc
path = buf.buf;
}
 
-   if (!access_or_die(path, R_OK)) {
+   if (!access_or_die(path, R_OK, 0)) {
if (++inc->depth > MAX_INCLUDE_DEPTH)
die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
cf && cf->name ? cf->name : "the command line");
@@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const 
char *repo_config)
 
home_config_paths(&user_config, &xdg_config, "config");
 
-   if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
+   if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}
 
-   if (xdg_config && !access_or_die(xdg_config, R_OK)) {
+   if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
ret += git_config_from_file(fn, xdg_config, data);
found += 1;
}
 
-   if (user_config && !access_or_die(user_config, R_OK)) {
+   if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) 
{
   

Re: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King  writes:

> OK. I'll hold off for now while we stew on it. Jonathan's patch looks OK
> to me, but it has the same issue. But I think every path has to be one
> of:
>
>   1. We annoy sysadmins who need to take an extra step to handle the
>  HOME situation with --user (the current behavior, or any other
>  proposal that they have to opt into).
>
>   2. We annoy sysadmins who want to set HOME with --user, either by
>  making what they want to do impossible, or making them set an extra
>  variable or option to accomplish what used to work (my patch to set
>  HOME with --user).
>
>   3. We loosen the check, so some cases which might be noteworthy are
>  not caught (my patch, Jonathan's patch, etc).
>
> I think any solution will have to fall into one of those slots. So we
> need to pick the least evil one, and then hammer out its least evil
> form.

That summarizes our options nicely, I think.  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 10/10] Correct common spelling mistakes in comments and tests

[Junio C Hamano]

Jonathan Nieder  writes:

> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index d8b7f2ff..f8a08b7f 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -116,7 +116,7 @@ tree_pretty_content="100644 blob $hello_sha1  hello"
>  
>  run_tests 'tree' $tree_sha1 $tree_size "" "$tree_pretty_content"
>  
> -commit_message="Intial commit"
> +commit_message="Initial commit"
>  commit_sha1=$(echo_without_newline "$commit_message" | git commit-tree 
> $tree_sha1)
>  commit_size=176
>  commit_content="tree $tree_sha1

This breaks the test for rather obvious reasons.  I'll squash this in:

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index f8a08b7..9820f70 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -118,7 +118,7 @@ run_tests 'tree' $tree_sha1 $tree_size "" 
"$tree_pretty_content"
 
 commit_message="Initial commit"
 commit_sha1=$(echo_without_newline "$commit_message" | git commit-tree 
$tree_sha1)
-commit_size=176
+commit_size=177
 commit_content="tree $tree_sha1
 author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> 00 +
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 00 +

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

[PATCH] fixup! config: allow inaccessible configuration under $HOME

[Jonathan Nieder]

A cleanup from Jeff King.

Signed-off-by: Jonathan Nieder 
---
 wrapper.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

Jeff King wrote:

> I kind of wonder if we are doing anything with the check at this point.
> I suppose ENOMEM and EIO are the only interesting things left at this
> point. The resulting code would be much nicer if the patch were just:
>
>   -access_or_die(...);
>   +access(...);
>
> but I guess those conditions are still worth checking for, especially if
> we think an attacker could trigger ENOMEM intentionally. To be honest, I
> am not sure how possible that is, but it is not that hard to do so.

Yeah, I was tempted to go the plain access() route.  The case that
convinced me not to is that I wouldn't want to think about whether
there could have been a blip in $HOME producing EIO when wondering how
some malformed object had been accepted.  The ENOMEM case just seemed
simpler to explain.

I would also be surprised if it is easy to control kernel ENOMEM
carefully enough to exploit (a more likely DoS outcome is crashing
programs or a kernel assertion failure, which are more harmless in
their way).  I've given up on predicting what is too hard or easy
enough for security folks.  I couldn't think of an obviously easier
vulnerability that is already accepted to put my mind at ease.

[...]
> I know you are trying to be flexible with the flag,

I was mostly worried about damage to readability if we end up needing
to go further down this direction.  The opportunity to look at all
call sites was also nice.

[...]
>   static int access_error_is_ok(int err, int flag)
>   {
>   return err == ENOENT || errno == ENOTDIR ||
>   (flag & ACCESS_EACCES_OK && err == EACCES);
>   }

Nice.  Here's a patch for squashing in.

diff --git a/wrapper.c b/wrapper.c
index e860ad1..29a866b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -408,11 +408,16 @@ void warn_on_inaccessible(const char *path)
warning(_("unable to access '%s': %s"), path, strerror(errno));
 }
 
+static int access_error_is_ok(int err, unsigned flag)
+{
+   return errno == ENOENT || errno == ENOTDIR ||
+   ((flag & ACCESS_EACCES_OK) && errno == EACCES);
+}
+
 int access_or_warn(const char *path, int mode, unsigned flag)
 {
int ret = access(path, mode);
-   if (ret && errno != ENOENT && errno != ENOTDIR &&
-   (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
+   if (ret && !access_error_is_ok(errno, flag))
warn_on_inaccessible(path);
return ret;
 }
@@ -420,8 +425,7 @@ int access_or_warn(const char *path, int mode, unsigned 
flag)
 int access_or_die(const char *path, int mode, unsigned flag)
 {
int ret = access(path, mode);
-   if (ret && errno != ENOENT && errno != ENOTDIR &&
-   (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
+   if (ret && !access_error_is_ok(errno, flag))
die_errno(_("unable to access '%s'"), path);
return ret;
 }
-- 
1.8.2.1

--
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: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 12:51:19PM -0700, Junio C Hamano wrote:

> >> If the access() failed due to ENOENT, the caller will get a negative
> >> return from this function and will treat it as "ok, it does not
> >> exist", with the original or the updated code.  This new case is
> >> treated the same way by the existing callers, i.e. pretending as if
> >> there is _no_ file in that unreadable $HOME directory.
> >
> > Exactly.
> 
> The explanation you are replying to was meant to illustrate how this
> is not "inaccessible is OK", but is "treat inaccessible as missing",
> by the way.

Ah, I see the distinction you were making. Yes, that is what I was
thinking (and what the patch does); I just used the word "OK" instead.

> Well, at least to me, the documentation update was never about
> "oops, we broke it", but was about "be careful where the HOME you
> are using actually is" from the beginning of the suggestion.  I was
> actually planning to apply it to maint-1.8.1 that predates the xdg
> stuff, and that is why the text only suggests to set HOME for the
> config.

Yes; I think the only change needed would be to the commit message I
proposed (if you even picked that up; I didn't look).

> > Do you have an opinion on just dropping the environment variable
> > completely and behaving this way all the time? It would "just fix" the
> > cases people running into using su/sudo, too.
> 
> With the tightening, people who used --user=daemon, expecting that
> they can later tweak the behaviour by touching ~daemon/.gitconfig,
> got an early warning that they need to set HOME themselves, but with
> any variant of the patch under discussion, as long as loosening is
> on by default, will no longer get that benefit.
> 
> I am not yet convinced if that is a real "fix/cure".
> 
> So, no, I have not even reached the point where I can form an
> opinion if this behaviour should be the default.

OK. I'll hold off for now while we stew on it. Jonathan's patch looks OK
to me, but it has the same issue. But I think every path has to be one
of:

  1. We annoy sysadmins who need to take an extra step to handle the
 HOME situation with --user (the current behavior, or any other
 proposal that they have to opt into).

  2. We annoy sysadmins who want to set HOME with --user, either by
 making what they want to do impossible, or making them set an extra
 variable or option to accomplish what used to work (my patch to set
 HOME with --user).

  3. We loosen the check, so some cases which might be noteworthy are
 not caught (my patch, Jonathan's patch, etc).

I think any solution will have to fall into one of those slots. So we
need to pick the least evil one, and then hammer out its least evil
form.

-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: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King  writes:

>> If the access() failed due to ENOENT, the caller will get a negative
>> return from this function and will treat it as "ok, it does not
>> exist", with the original or the updated code.  This new case is
>> treated the same way by the existing callers, i.e. pretending as if
>> there is _no_ file in that unreadable $HOME directory.
>
> Exactly.

The explanation you are replying to was meant to illustrate how this
is not "inaccessible is OK", but is "treat inaccessible as missing",
by the way.

>> That semantics sounds sane and safe to me.
>
> Thanks. I'll re-roll with a proper commit message and the fixups I
> mentioned above. I think we should still do the documentation for
> git-daemon. But it is no longer about "oops, we broke git-daemon", but
> "you may want know that we do not set HOME in case you are doing
> something tricky with config". I'll submit that with the re-roll, too.

Well, at least to me, the documentation update was never about
"oops, we broke it", but was about "be careful where the HOME you
are using actually is" from the beginning of the suggestion.  I was
actually planning to apply it to maint-1.8.1 that predates the xdg
stuff, and that is why the text only suggests to set HOME for the
config.

> Do you have an opinion on just dropping the environment variable
> completely and behaving this way all the time? It would "just fix" the
> cases people running into using su/sudo, too.

With the tightening, people who used --user=daemon, expecting that
they can later tweak the behaviour by touching ~daemon/.gitconfig,
got an early warning that they need to set HOME themselves, but with
any variant of the patch under discussion, as long as loosening is
on by default, will no longer get that benefit.

I am not yet convinced if that is a real "fix/cure".

So, no, I have not even reached the point where I can form an
opinion if this behaviour should be the default.
--
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 1/3] Make read_index_data() public

[Jeff King]

On Fri, Apr 12, 2013 at 07:26:11PM +0200, Lukas Fleischer wrote:

> This allows for reusing the function in convert.c later.
> 
> Also, move it from attr.c to read-cache.c and add a use_index parameter
> to specify a custom index_state since we are no longer enable to access
> the static use_index variable from attr.c.

I'm all for removing duplicated code, but, but I think the name
"read_index_data" is a bit misleading for a global function. I would
expect it to read data from the index (and the argument "path" does not
help clarify that at all).

Can we rename it read_blob_data_from_index_path() or something?

-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] config: allow inaccessible configuration under $HOME

[Jeff King]

On Fri, Apr 12, 2013 at 12:14:33PM -0700, Jonathan Nieder wrote:

> One unexpected downside to the changes v1.7.12.1~2^2~4 (config: warn
> on inaccessible files, 2012-08-21) and v1.8.1.1~22^2~2 (config: treat
> user and xdg config permission problems as errors, 2012-10-13) is that
> they often trip when git is being run as a server.  The appropriate
> daemon (sshd, inetd, git-daemon, etc) starts as "root", creates a
> listening socket, and then drops privileges, meaning that when git
> commands are invoked they cannot access $HOME and die with
> 
>  fatal: unable to access '/root/.config/git/config': Permission denied
> 
> The intent was always to prevent important configuration (think
> "[transfer] fsckobjects") from being ignored when the configuration is
> unintentionally unreadable (for example with ENOMEM due to a DoS
> attack).  But that is not worth breaking these cases of
> drop-privileges-without-resetting-HOME that were working fine before.
> 
> Treat user and xdg configuration that is inaccessible due to
> permissions (EACCES) as though no user configuration was provided at
> all.

I kind of wonder if we are doing anything with the check at this point.
I suppose ENOMEM and EIO are the only interesting things left at this
point. The resulting code would be much nicer if the patch were just:

  -access_or_die(...);
  +access(...);

but I guess those conditions are still worth checking for, especially if
we think an attacker could trigger ENOMEM intentionally. To be honest, I
am not sure how possible that is, but it is not that hard to do so.

> An alternative approach would be to check if $HOME is readable, but
> that would not solve the problem in cases where the user who dropped
> privileges had a globally readable HOME with only .config or
> .gitconfig being private.

Yeah, although I wonder if those are signs of a misconfiguration that
should be brought to the user's attention (~/.gitconfig I feel more
confident about; less so about $HOME/.config, which might be locked down
for reasons unrelated to git).

> > Given how tight the exception is, I almost wonder if we should drop the
> > environment variable completely, and just never complain about this case
> > (in other words, just make it a loosening of 96b9e0e3).
> 
> Yeah, I think that would be better.
> 
> How about this?  (Still needs tests.)

I think it's probably OK. Like all of the proposed solutions, it is a
bit of compromise about what is worth mentioning to the user and what is
not. But we cannot have our cake and eat it, too, so I'd be fine with
this.

I agree a test would be nice for whatever solution we choose (both to
check that we fail when we should, and do not when we should not).

> - if (xdg_config && !access_or_die(xdg_config, R_OK)) {
> + if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {

I know you are trying to be flexible with the flag, but I wonder if the
resulting code would read better if we just added a new
"access_or_die_lenient" helper, which would embody the wisdom of
ignoring EACCES, or anything else that comes up later. It seems like all
callers would want either the vanilla form or the lenient form.

I do not feel too strongly about it, though.

> -int access_or_warn(const char *path, int mode)
> +int access_or_warn(const char *path, int mode, unsigned flag)
>  {
>   int ret = access(path, mode);
> - if (ret && errno != ENOENT && errno != ENOTDIR)
> + if (ret && errno != ENOENT && errno != ENOTDIR &&
> + (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
>   warn_on_inaccessible(path);
>   return ret;
>  }
>  
> -int access_or_die(const char *path, int mode)
> +int access_or_die(const char *path, int mode, unsigned flag)
>  {
>   int ret = access(path, mode);
> - if (ret && errno != ENOENT && errno != ENOTDIR)
> + if (ret && errno != ENOENT && errno != ENOTDIR &&
> + (!(flag & ACCESS_EACCES_OK) || errno != EACCES))
>   die_errno(_("unable to access '%s'"), path);
>   return ret;
>  }

Now that these conditions are getting more complex, we should perhaps
combine them, like:

  static int access_error_is_ok(int err, int flag)
  {
  return err == ENOENT || errno == ENOTDIR ||
  (flag & ACCESS_EACCES_OK && err == EACCES);
  }

-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: git push crashing

[Pali Rohár]

On Friday 12 April 2013 21:06:31 John Keeping wrote:
> On Fri, Apr 12, 2013 at 08:50:25PM +0200, Pali Rohár wrote:
> > when I'm trying to push one specific branch from my git
> > repository to server, git push crashing. Pushing branch is
> > rejected by server (because non fast forward), but local
> > git app should not crash.
> > 
> > I'm using git from ubuntu apt repository (compiled myself
> > for debug symbols), version git_1.7.10.4-1ubuntu1 on amd64
> > ubuntu system:
> > http://packages.ubuntu.com/source/quantal/git
> > 
> > Here are gdb backtrace and valgrind outputs. I changed
> > server, repo and branch strings from output. It looks like
> > that git crashing in strcmp function because one of
> > arguments is NULL.
> > 
> > If you need more info, I can send it. This crash occur
> > always. I can reproduce it again and again...
> 
> This looks like the same issue that was fixed by commit
> 1d2c14d (push: fix segfault when HEAD points nowhere -
> 2013-01-31).
> 
> Can you try again with Git 1.8.2 and see if the crash still
> happens?

Hello, now I tried version 1.8.2.1 and git push not crashing :-) 
I also tried version 1.8.1 and as expected it crashed.

So thanks for quick reply, seems that this problem is already 
fixed in 1.8.2.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

[PATCH] config: allow inaccessible configuration under $HOME

[Jonathan Nieder]

One unexpected downside to the changes v1.7.12.1~2^2~4 (config: warn
on inaccessible files, 2012-08-21) and v1.8.1.1~22^2~2 (config: treat
user and xdg config permission problems as errors, 2012-10-13) is that
they often trip when git is being run as a server.  The appropriate
daemon (sshd, inetd, git-daemon, etc) starts as "root", creates a
listening socket, and then drops privileges, meaning that when git
commands are invoked they cannot access $HOME and die with

 fatal: unable to access '/root/.config/git/config': Permission denied

The intent was always to prevent important configuration (think
"[transfer] fsckobjects") from being ignored when the configuration is
unintentionally unreadable (for example with ENOMEM due to a DoS
attack).  But that is not worth breaking these cases of
drop-privileges-without-resetting-HOME that were working fine before.

Treat user and xdg configuration that is inaccessible due to
permissions (EACCES) as though no user configuration was provided at
all.

An alternative approach would be to check if $HOME is readable, but
that would not solve the problem in cases where the user who dropped
privileges had a globally readable HOME with only .config or
.gitconfig being private.

This does not change the behavior when "git config" is being used to
write to a user's config file, when /etc/gitconfig or .git/config is
unreadable (since those are more serious configuration errors), or
when ~/.gitconfig or ~/.config/git is unreadable due to problems other
than permissions.

Thanks to Mike Galbraith, W. Trevor King, and Jeff King for their
patient explanations.

Signed-off-by: Jonathan Nieder 
---
Jeff King wrote:

> Given how tight the exception is, I almost wonder if we should drop the
> environment variable completely, and just never complain about this case
> (in other words, just make it a loosening of 96b9e0e3).

Yeah, I think that would be better.

How about this?  (Still needs tests.)

 builtin/config.c  |  4 ++--
 config.c  | 10 +-
 dir.c |  4 ++--
 git-compat-util.h |  5 +++--
 wrapper.c | 10 ++
 5 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 33c9bf9..19ffcaf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -379,8 +379,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
 */
die("$HOME not set");
 
-   if (access_or_warn(user_config, R_OK) &&
-   xdg_config && !access_or_warn(xdg_config, R_OK))
+   if (access_or_warn(user_config, R_OK, 0) &&
+   xdg_config && !access_or_warn(xdg_config, R_OK, 0))
given_config_file = xdg_config;
else
given_config_file = user_config;
diff --git a/config.c b/config.c
index aefd80b..830ee14 100644
--- a/config.c
+++ b/config.c
@@ -58,7 +58,7 @@ static int handle_path_include(const char *path, struct 
config_include_data *inc
path = buf.buf;
}
 
-   if (!access_or_die(path, R_OK)) {
+   if (!access_or_die(path, R_OK, 0)) {
if (++inc->depth > MAX_INCLUDE_DEPTH)
die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
cf && cf->name ? cf->name : "the command line");
@@ -954,23 +954,23 @@ int git_config_early(config_fn_t fn, void *data, const 
char *repo_config)
 
home_config_paths(&user_config, &xdg_config, "config");
 
-   if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
+   if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}
 
-   if (xdg_config && !access_or_die(xdg_config, R_OK)) {
+   if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
ret += git_config_from_file(fn, xdg_config, data);
found += 1;
}
 
-   if (user_config && !access_or_die(user_config, R_OK)) {
+   if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) 
{
ret += git_config_from_file(fn, user_config, data);
found += 1;
}
 
-   if (repo_config && !access_or_die(repo_config, R_OK)) {
+   if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
ret += git_config_from_file(fn, repo_config, data);
found += 1;
}
diff --git a/dir.c b/dir.c
index 91cfd99..9cb2f3d 100644
--- a/dir.c
+++ b/dir.c
@@ -1637,9 +1637,9 @@ void setup_standard_excludes(struct dir_struct *dir)
home_config_paths(NULL, &xdg_path, "ignore");
excludes_file = xdg_path;
}
-   if (!access_or_warn(path, R_OK))
+   if (!access_or_warn(path, R_OK, 0))
add_excludes_from_file(dir

Re: [PATCH 01/10] doc: various spelling fixes

[Junio C Hamano]

Jonathan Nieder  writes:

>Note that if per-instance configuration file exists, then system-wide
> -  configuration is _not used at all_.  This is quite untypical and suprising
> +  configuration is _not used at all_.  This is quite untypical and surprising
>behavior.  On the other hand changing current behavior would break 
> backwards
>compatibility and can lead to unexpected changes in gitweb behavior.
>Therefore gitweb also looks for common system-wide configuration file,

Hmm, "atypical", isn't it?

The flow of the text is awkward.  "This is bad. Oh the other hand,
better is broken. Therefore ..." forces readers to make multiple
guesses while reading: "ok, bad, so you plan to change it and warn
us about upcoming change?  oh, not that, changing it is bad, so we
have to live with it?  oh, not that, there is another one that is
common and that is what we can use".

It may be a good idea to rewrite this paragraph to avoid such a
mental roller-coaster in the first place.

The GITWEB_CONFIG_SYSTEM system-wide configuration file is only
used for instances that lack per-instance configuration file.
You can use GITWEB_CONFIG_COMMON file to keep common default
settings that apply to all instances.

or something.

Not asking for a re-roll, but it may be a potential follow-up candidate.

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: git push crashing

[John Keeping]

On Fri, Apr 12, 2013 at 08:50:25PM +0200, Pali Rohár wrote:
> when I'm trying to push one specific branch from my git repository
> to server, git push crashing. Pushing branch is rejected by server
> (because non fast forward), but local git app should not crash.
> 
> I'm using git from ubuntu apt repository (compiled myself for debug
> symbols), version git_1.7.10.4-1ubuntu1 on amd64 ubuntu system:
> http://packages.ubuntu.com/source/quantal/git
> 
> Here are gdb backtrace and valgrind outputs. I changed server, repo
> and branch strings from output. It looks like that git crashing in
> strcmp function because one of arguments is NULL.
> 
> If you need more info, I can send it. This crash occur always. I can
> reproduce it again and again...

This looks like the same issue that was fixed by commit 1d2c14d (push:
fix segfault when HEAD points nowhere - 2013-01-31).

Can you try again with Git 1.8.2 and see if the crash still happens?
--
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: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 11:23:46AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > So here's what I came up with. I tried to make the exception as tight as
> > possible by checking that $HOME was actually the problem, as that is the
> > common problem (you switch users, but HOME is pointing to the old user).
> > ...
> > diff --git a/daemon.c b/daemon.c
> > index 131b049..6c56cc0 100644
> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
> > if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
> > setgid (cred->gid) || setuid(cred->pass->pw_uid)))
> > die("cannot drop privileges");
> > -   setenv("GIT_CONFIG_INACCESSIBLE_HOME_OK", "1", 0);
> > +   setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, "1", 0);
> >  }
> 
> Compared against an unpublished diffbase???

Oops. Forgot I had made a WIP commit before running the diff.

> OK, so the idea is
> 
>  - The environment can tell us to ignore permission errors for paths
>under $HOME if (and only if) $HOME itself is not readable;
> 
>  - We got a permission error here.  inaccessible_home_ok() will tell
>us if the path is under $HOME and the above condition holds (in
>which case it will say "ok, ignore that error").

Exactly.

> which sounds good, but it relies on the caller of this function not
> to try actually reading from the path.

Yes, but that is the only sane thing for the caller to do, since it gets
the same exit code from ENOENT and ENOTDIR already. Probably a comment
describing the return value is in order.

> If the access() failed due to ENOENT, the caller will get a negative
> return from this function and will treat it as "ok, it does not
> exist", with the original or the updated code.  This new case is
> treated the same way by the existing callers, i.e. pretending as if
> there is _no_ file in that unreadable $HOME directory.

Exactly.

> That semantics sounds sane and safe to me.

Thanks. I'll re-roll with a proper commit message and the fixups I
mentioned above. I think we should still do the documentation for
git-daemon. But it is no longer about "oops, we broke git-daemon", but
"you may want know that we do not set HOME in case you are doing
something tricky with config". I'll submit that with the re-roll, too.

Do you have an opinion on just dropping the environment variable
completely and behaving this way all the time? It would "just fix" the
cases people running into using su/sudo, too.

-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 v3 0/7] completion: reorg and performance improvements

[Junio C Hamano]

All patches in this series and the other cherry-pick one looked
sensible, with a fix to [3/7] I suggested in the other thread about
protecting against "set -u".

Queued.


--
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 3/7] completion: add new __gitcompadd helper

[Junio C Hamano]

Felipe Contreras  writes:

> On Fri, Apr 12, 2013 at 12:55 PM, Junio C Hamano  wrote:
>> Felipe Contreras  writes:
>
>>>  }
>>>
>>>  # Generates completion reply with compgen from newline-separated possible
>>> @@ -1820,7 +1823,7 @@ _git_config ()
>>>   local remote="${prev#remote.}"
>>>   remote="${remote%.fetch}"
>>>   if [ -z "$cur" ]; then
>>> - COMPREPLY=("refs/heads/")
>>> + __gitcompadd "refs/heads/"
>>
>> I am not sure about this one, though.
>>
>> Other callers took pains to protet against triggering unset variable
>> references by using ${1-} instead of ${1}.  Shouldn't this caller be
>> passing three empty strings?
>
> Perhaps, or perhaps we were being too careful before: 'compgen -W foo'
> is the same as 'compgen -W foo -S "" -P "" -- ""'.

Yes, they are the same (otherwise this patch would not be valid),
but that is not what i was wondering about.


--
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 push crashing

[Pali Rohár]

Hello,

when I'm trying to push one specific branch from my git repository
to server, git push crashing. Pushing branch is rejected by server
(because non fast forward), but local git app should not crash.

I'm using git from ubuntu apt repository (compiled myself for debug
symbols), version git_1.7.10.4-1ubuntu1 on amd64 ubuntu system:
http://packages.ubuntu.com/source/quantal/git

Here are gdb backtrace and valgrind outputs. I changed server, repo
and branch strings from output. It looks like that git crashing in
strcmp function because one of arguments is NULL.

If you need more info, I can send it. This crash occur always. I can
reproduce it again and again...

$ valgrind --leak-check=full git push server:~/repo branch 
==19381== Memcheck, a memory error detector
==19381== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==19381== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==19381== Command: git push server:~/repo branch
==19381== 
X11 forwarding request failed on channel 0
To server:~/repo
 ! [rejected]branch -> branch (non-fast-forward)
==19381== Invalid read of size 1
==19381==at 0x4C2C831: strcmp (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19381==by 0x4DA2DD: transport_print_push_status (transport.c:747)
==19381==by 0x4DAC19: transport_push (transport.c:1069)
==19381==by 0x4515FC: push_with_options (push.c:191)
==19381==by 0x451EF5: cmd_push (push.c:270)
==19381==by 0x405787: handle_internal_command (git.c:308)
==19381==by 0x404B91: main (git.c:513)
==19381==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==19381== 
==19381== 
==19381== Process terminating with default action of signal 11 (SIGSEGV)
==19381==  Access not within mapped region at address 0x0
==19381==at 0x4C2C831: strcmp (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19381==by 0x4DA2DD: transport_print_push_status (transport.c:747)
==19381==by 0x4DAC19: transport_push (transport.c:1069)
==19381==by 0x4515FC: push_with_options (push.c:191)
==19381==by 0x451EF5: cmd_push (push.c:270)
==19381==by 0x405787: handle_internal_command (git.c:308)
==19381==by 0x404B91: main (git.c:513)
==19381==  If you believe this happened as a result of a stack
==19381==  overflow in your program's main thread (unlikely but
==19381==  possible), you can try to increase the size of the
==19381==  main thread stack using the --main-stacksize= flag.
==19381==  The main thread stack size used in this run was 8388608.
==19381== 
==19381== HEAP SUMMARY:
==19381== in use at exit: 7,672,224 bytes in 28,274 blocks
==19381==   total heap usage: 70,136 allocs, 41,862 frees, 108,300,058 bytes 
allocated
==19381== 
==19381== 43 (24 direct, 19 indirect) bytes in 1 blocks are definitely lost in 
loss record 23 of 59
==19381==at 0x4C29E46: calloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19381==by 0x4E4B28: xcalloc (wrapper.c:98)
==19381==by 0x4BBCE9: parse_refspec_internal (remote.c:520)
==19381==by 0x4BCE3E: match_push_refs (remote.c:653)
==19381==by 0x4DAB3D: transport_push (transport.c:1047)
==19381==by 0x4515FC: push_with_options (push.c:191)
==19381==by 0x451EF5: cmd_push (push.c:270)
==19381==by 0x405787: handle_internal_command (git.c:308)
==19381==by 0x404B91: main (git.c:513)
==19381== 
==19381== 2,404 (110 direct, 2,294 indirect) bytes in 1 blocks are definitely 
lost in loss record 46 of 59
==19381==at 0x4C29E46: calloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19381==by 0x4E4B28: xcalloc (wrapper.c:98)
==19381==by 0x4BC241: one_local_ref (remote.c:1643)
==19381==by 0x4B7B8C: do_for_each_ref (refs.c:750)
==19381==by 0x4BDEF3: get_local_heads (remote.c:1654)
==19381==by 0x4DAAF0: transport_push (transport.c:1032)
==19381==by 0x4515FC: push_with_options (push.c:191)
==19381==by 0x451EF5: cmd_push (push.c:270)
==19381==by 0x405787: handle_internal_command (git.c:308)
==19381==by 0x404B91: main (git.c:513)
==19381== 
==19381== LEAK SUMMARY:
==19381==definitely lost: 134 bytes in 2 blocks
==19381==indirectly lost: 2,313 bytes in 24 blocks
==19381==  possibly lost: 0 bytes in 0 blocks
==19381==still reachable: 7,669,777 bytes in 28,248 blocks
==19381== suppressed: 0 bytes in 0 blocks
==19381== Reachable blocks (those to which a pointer was found) are not shown.
==19381== To see them, rerun with: --leak-check=full --show-reachable=yes
==19381== 
==19381== For counts of detected and suppressed errors, rerun with: -v
==19381== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 2 from 2)


$ gdb --args git push server:~/repo branch 
GNU gdb (GDB) 7.5-ubuntu
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitt

Re: [PATCH v3 3/7] completion: add new __gitcompadd helper

[Felipe Contreras]

On Fri, Apr 12, 2013 at 12:55 PM, Junio C Hamano  wrote:
> Felipe Contreras  writes:

>>  }
>>
>>  # Generates completion reply with compgen from newline-separated possible
>> @@ -1820,7 +1823,7 @@ _git_config ()
>>   local remote="${prev#remote.}"
>>   remote="${remote%.fetch}"
>>   if [ -z "$cur" ]; then
>> - COMPREPLY=("refs/heads/")
>> + __gitcompadd "refs/heads/"
>
> I am not sure about this one, though.
>
> Other callers took pains to protet against triggering unset variable
> references by using ${1-} instead of ${1}.  Shouldn't this caller be
> passing three empty strings?

Perhaps, or perhaps we were being too careful before: 'compgen -W foo'
is the same as 'compgen -W foo -S "" -P "" -- ""'.

--
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: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King  writes:

> So here's what I came up with. I tried to make the exception as tight as
> possible by checking that $HOME was actually the problem, as that is the
> common problem (you switch users, but HOME is pointing to the old user).
> ...
> diff --git a/daemon.c b/daemon.c
> index 131b049..6c56cc0 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
>   if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
>   setgid (cred->gid) || setuid(cred->pass->pw_uid)))
>   die("cannot drop privileges");
> - setenv("GIT_CONFIG_INACCESSIBLE_HOME_OK", "1", 0);
> + setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, "1", 0);
>  }

Compared against an unpublished diffbase???

>  
>  static struct credentials *prepare_credentials(const char *user_name,
> diff --git a/wrapper.c b/wrapper.c
> index bac59d2..644f867 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)
>   return ret;
>  }
>  
> +/*
> + * Returns true iff the path is under $HOME, that directory is inaccessible,
> + * and the user has told us through the environment that an inaccessible HOME
> + * is OK. The results are cached so that repeated calls will not make 
> multiple
> + * syscalls.
> + */
> +static int inaccessible_home_ok(const char *path)
> +{
> + static int gentle = -1;
> + static int home_inaccessible = -1;
> + const char *home;
> +
> + if (gentle < 0)
> + gentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);
> + if (!gentle)
> + return 0;
> +
> + home = getenv("HOME");
> + if (!home)
> + return 0;
> + /*
> +  * We do not bother with normalizing PATHs to avoid symlinks
> +  * here, since the point is to catch paths that are
> +  * constructed as "$HOME/...".
> +  */
> + if (prefixcmp(path, home) && path[strlen(home)] == '/')
> + return 0;
> +
> + if (home_inaccessible < 0)
> + home_inaccessible = access(home, R_OK|X_OK) < 0 && errno == 
> EACCES;
> + return home_inaccessible;
> +}
> +
>  int access_or_die(const char *path, int mode)
>  {
>   int ret = access(path, mode);
> - if (ret && errno != ENOENT && errno != ENOTDIR)
> + if (ret && errno != ENOENT && errno != ENOTDIR) {
> + /*
> +  * We do not have to worry about clobbering errno
> +  * in inaccessible_home_ok, because we get either EACCES
> +  * again, or we die.
> +  */
> + if (errno == EACCES && inaccessible_home_ok(path))
> + return ret;

OK, so the idea is

 - The environment can tell us to ignore permission errors for paths
   under $HOME if (and only if) $HOME itself is not readable;

 - We got a permission error here.  inaccessible_home_ok() will tell
   us if the path is under $HOME and the above condition holds (in
   which case it will say "ok, ignore that error").

which sounds good, but it relies on the caller of this function not
to try actually reading from the path.

If the access() failed due to ENOENT, the caller will get a negative
return from this function and will treat it as "ok, it does not
exist", with the original or the updated code.  This new case is
treated the same way by the existing callers, i.e. pretending as if
there is _no_ file in that unreadable $HOME directory.

That semantics sounds sane and safe to me.
--
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 5/7] completion: get rid of compgen

[Junio C Hamano]

Felipe Contreras  writes:

> Here are some numbers filtering N amount of words:

Nice table.  "N amount of words" sounded somewhat funny to me, but I
am not a native.

> ...
>   == 1000 ==
>   original: 0.012s
>   new: 0.011s
>   == 1 ==
>   original: 0.056s
>   new: 0.066s
>   == 10 ==
>   original: 2.669s
>   new: 0.622s
>
> If the results are not narrowed:
> ...
>   == 1000 ==
>   original: 0.020s
>   new: 0.015s
>   == 1 ==
>   original: 0.101s
>   new: 0.355s
>   == 10 ==
>   original: 2.850s
>   new: 31.941s
>
> So, unless 'git checkout ' usually gives you more than 10
> results, you'll get an improvement :)

Nice numbers.  I think you meant 1 not 10 here.

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 90b54ab..d8009f5 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -197,11 +197,16 @@ fi
>  
>  __gitcompadd ()
>  {
> - COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
> + local i=0
> + for x in $1; do
> + if [[ "$x" == "$3"* ]]; then
> + COMPREPLY[i++]="$2$x$4"
> + fi
> + done
>  }

Nice; can't be simpler than that ;-)
--
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 3/7] completion: add new __gitcompadd helper

[Junio C Hamano]

Felipe Contreras  writes:

> The idea is to never touch the COMPREPLY variable directly.
>
> This allows other completion systems (i.e. zsh) to override
> __gitcompadd, and do something different instead.
>
> Also, this allows further optimizations down the line.
>
> There should be no functional changes.
>
> Signed-off-by: Felipe Contreras 
> ---
>  contrib/completion/git-completion.bash | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 2c87fd8..90b54ab 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -195,6 +195,11 @@ _get_comp_words_by_ref ()
>  }
>  fi
>  
> +__gitcompadd ()
> +{
> + COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))

Making a mental note that this takes prefix ($2), suffix ($4) and
the actual word ($3) are given in addition to the list of expansions
($1)...

> +}
> +
>  # Generates completion reply with compgen, appending a space to possible
>  # completion words, if necessary.
>  # It accepts 1 to 4 arguments:
> @@ -211,9 +216,7 @@ __gitcomp ()
>   ;;
>   *)
>   local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" \
> - -W "$(__gitcomp_1 "${1-}" "${4-}")" \
> - -- "$cur_"))
> + __gitcompadd "$(__gitcomp_1 "${1-}" "${4-}")" "${2-}" "$cur_" ""

This did not use to use suffix, but we pass an empty string as $4,
so it is an equivalent rewrite.

>   ;;
>   esac
>  }
> @@ -230,7 +233,7 @@ __gitcomp ()
>  __gitcomp_nl ()
>  {
>   local IFS=$'\n'
> - COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
> + __gitcompadd "$1" "${2-}" "${3-$cur}" "${4- }"

This also is a straight-forward rewrite.

>  }
>  
>  # Generates completion reply with compgen from newline-separated possible
> @@ -1820,7 +1823,7 @@ _git_config ()
>   local remote="${prev#remote.}"
>   remote="${remote%.fetch}"
>   if [ -z "$cur" ]; then
> - COMPREPLY=("refs/heads/")
> + __gitcompadd "refs/heads/"

I am not sure about this one, though.

Other callers took pains to protet against triggering unset variable
references by using ${1-} instead of ${1}.  Shouldn't this caller be
passing three empty strings?

>   return
>   fi
>   __gitcomp_nl "$(__git_refs_remotes "$remote")"
--
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: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King  writes:

> On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:
>
>> OK, then...
>
>> -- >8 --
>> Subject: [PATCH] doc: clarify that "git daemon --user=" option does 
>> not export HOME=~user
>
> I'd add this motiviation to the body of the commit message:
>
>   The fact that we don't set $HOME may confuse admins who
>   expect $HOME/.gitconfig to be respected. And worse, since
>   96b9e0e3, a git-daemon started by root is likely to fail
>   to run at all, as the user we switch to generally cannot
>   read ~root.
>
> This still feels ugly, like we are documenting some gotcha
> that is going to hit most admins, when we could be helping
> them in the code.

I agree that it feels a bit wrong to sound as if we are blaming the
messanger (the one that notices a possible misconfiguration), but
you are correct that we should make a note on why we think it is a
good idea to add this piece of extra documentation in the history.

Will add the above before queuing.

> One option we have not explored is an environment variable
> to loosen git's requirement. I'm thinking something like
> GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
> when git-daemon uses --user.
>
> That would leave all existing setups working, but would
> still enable the extra protections for people not running
> git-daemon (and people who use git via sudo could choose to
> set it, too, if they would prefer that to setting up HOME).

Perhaps.

Right now, the only case people noticed was that we complain when
the effective user cannot even tell if config file(s) exists or not.
Labelling this option as "Treat unreable as missing" is fine, but
"an inaccessible homedir is OK" is vastly different.  Imagine a new
version where we start _requiring_ something to exist (and we read
from it) and imagine further that the expected place of that thing
is somewhere inside $HOME. We cannot keep the promise to those who
set "an inaccessible homedir is OK" option when that happens, as we
may need that piece of information we wanted to read from there in
order to properly operate.

In any case, I think the loosening is an independent issue.


--
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/3] convert.c: Remove duplicate code

[Lukas Fleischer]

has_cr_in_index() is an almost 1:1 copy of read_index_data() with some
additions. Invoke read_index_data() instead of using copy-pasted code.

Signed-off-by: Lukas Fleischer 
---
 convert.c | 27 ++-
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/convert.c b/convert.c
index 3520252..0d9f8a9 100644
--- a/convert.c
+++ b/convert.c
@@ -153,36 +153,13 @@ static void check_safe_crlf(const char *path, enum 
crlf_action crlf_action,
 
 static int has_cr_in_index(const char *path)
 {
-   int pos, len;
unsigned long sz;
-   enum object_type type;
void *data;
int has_cr;
-   struct index_state *istate = &the_index;
 
-   len = strlen(path);
-   pos = index_name_pos(istate, path, len);
-   if (pos < 0) {
-   /*
-* We might be in the middle of a merge, in which
-* case we would read stage #2 (ours).
-*/
-   int i;
-   for (i = -pos - 1;
-(pos < 0 && i < istate->cache_nr &&
- !strcmp(istate->cache[i]->name, path));
-i++)
-   if (ce_stage(istate->cache[i]) == 2)
-   pos = i;
-   }
-   if (pos < 0)
+   data = read_index_data(path, NULL, &sz);
+   if (!data)
return 0;
-   data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz);
-   if (!data || type != OBJ_BLOB) {
-   free(data);
-   return 0;
-   }
-
has_cr = memchr(data, '\r', sz) != NULL;
free(data);
return has_cr;
-- 
1.8.2.675.gda3bb24.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 2/3] Add size parameter to read_index_data()

[Lukas Fleischer]

This allows for optionally getting the size of the returned data and
will be used in a follow-up patch.

Signed-off-by: Lukas Fleischer 
---
 attr.c   | 2 +-
 cache.h  | 2 +-
 read-cache.c | 5 -
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/attr.c b/attr.c
index 2e1ce7b..e5af3c6 100644
--- a/attr.c
+++ b/attr.c
@@ -387,7 +387,7 @@ static struct attr_stack *read_attr_from_index(const char 
*path, int macro_ok)
char *buf, *sp;
int lineno = 0;
 
-   buf = read_index_data(path, use_index);
+   buf = read_index_data(path, use_index, NULL);
if (!buf)
return NULL;
 
diff --git a/cache.h b/cache.h
index a71e443..b281bbf 100644
--- a/cache.h
+++ b/cache.h
@@ -471,7 +471,7 @@ extern int add_file_to_index(struct index_state *, const 
char *path, int flags);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, int refresh);
 extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
-extern void *read_index_data(const char *path, struct index_state *use_index);
+extern void *read_index_data(const char *path, struct index_state *use_index, 
unsigned long *size);
 
 /* do stat comparison even if CE_VALID is true */
 #define CE_MATCH_IGNORE_VALID  01
diff --git a/read-cache.c b/read-cache.c
index 39e3424..32dc471 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1900,7 +1900,8 @@ int index_name_is_other(const struct index_state *istate, 
const char *name,
return 1;
 }
 
-void *read_index_data(const char *path, struct index_state *use_index)
+void *read_index_data(const char *path, struct index_state *use_index,
+ unsigned long *size)
 {
int pos, len;
unsigned long sz;
@@ -1930,5 +1931,7 @@ void *read_index_data(const char *path, struct 
index_state *use_index)
free(data);
return NULL;
}
+   if (size)
+   *size = sz;
return data;
 }
-- 
1.8.2.675.gda3bb24.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 1/3] Make read_index_data() public

[Lukas Fleischer]

This allows for reusing the function in convert.c later.

Also, move it from attr.c to read-cache.c and add a use_index parameter
to specify a custom index_state since we are no longer enable to access
the static use_index variable from attr.c.

Signed-off-by: Lukas Fleischer 
---
I am not totally sure whether read-cache.c is the best place for this...

 attr.c   | 35 +--
 cache.h  |  1 +
 read-cache.c | 33 +
 3 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/attr.c b/attr.c
index 689bc2a..2e1ce7b 100644
--- a/attr.c
+++ b/attr.c
@@ -381,46 +381,13 @@ static struct attr_stack *read_attr_from_file(const char 
*path, int macro_ok)
return res;
 }
 
-static void *read_index_data(const char *path)
-{
-   int pos, len;
-   unsigned long sz;
-   enum object_type type;
-   void *data;
-   struct index_state *istate = use_index ? use_index : &the_index;
-
-   len = strlen(path);
-   pos = index_name_pos(istate, path, len);
-   if (pos < 0) {
-   /*
-* We might be in the middle of a merge, in which
-* case we would read stage #2 (ours).
-*/
-   int i;
-   for (i = -pos - 1;
-(pos < 0 && i < istate->cache_nr &&
- !strcmp(istate->cache[i]->name, path));
-i++)
-   if (ce_stage(istate->cache[i]) == 2)
-   pos = i;
-   }
-   if (pos < 0)
-   return NULL;
-   data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz);
-   if (!data || type != OBJ_BLOB) {
-   free(data);
-   return NULL;
-   }
-   return data;
-}
-
 static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
 {
struct attr_stack *res;
char *buf, *sp;
int lineno = 0;
 
-   buf = read_index_data(path);
+   buf = read_index_data(path, use_index);
if (!buf)
return NULL;
 
diff --git a/cache.h b/cache.h
index ba5a47c..a71e443 100644
--- a/cache.h
+++ b/cache.h
@@ -471,6 +471,7 @@ extern int add_file_to_index(struct index_state *, const 
char *path, int flags);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, int refresh);
 extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);
 extern int index_name_is_other(const struct index_state *, const char *, int);
+extern void *read_index_data(const char *path, struct index_state *use_index);
 
 /* do stat comparison even if CE_VALID is true */
 #define CE_MATCH_IGNORE_VALID  01
diff --git a/read-cache.c b/read-cache.c
index 5a9704f..39e3424 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1899,3 +1899,36 @@ int index_name_is_other(const struct index_state 
*istate, const char *name,
}
return 1;
 }
+
+void *read_index_data(const char *path, struct index_state *use_index)
+{
+   int pos, len;
+   unsigned long sz;
+   enum object_type type;
+   void *data;
+   struct index_state *istate = use_index ? use_index : &the_index;
+
+   len = strlen(path);
+   pos = index_name_pos(istate, path, len);
+   if (pos < 0) {
+   /*
+* We might be in the middle of a merge, in which
+* case we would read stage #2 (ours).
+*/
+   int i;
+   for (i = -pos - 1;
+(pos < 0 && i < istate->cache_nr &&
+ !strcmp(istate->cache[i]->name, path));
+i++)
+   if (ce_stage(istate->cache[i]) == 2)
+   pos = i;
+   }
+   if (pos < 0)
+   return NULL;
+   data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz);
+   if (!data || type != OBJ_BLOB) {
+   free(data);
+   return NULL;
+   }
+   return data;
+}
-- 
1.8.2.675.gda3bb24.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 0/3] Remove ~25 lines of duplicate code

[Lukas Fleischer]

Merge read_index_data() and has_cr_in_index() which are almost 1:1
copies of each other.

Lukas Fleischer (3):
  Make read_index_data() public
  Add size parameter to read_index_data()
  convert.c: Remove duplicate code

 attr.c   | 35 +--
 cache.h  |  1 +
 convert.c| 27 ++-
 read-cache.c | 36 
 4 files changed, 40 insertions(+), 59 deletions(-)

-- 
1.8.2.675.gda3bb24.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: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 12:16:00PM -0400, Jeff King wrote:

> One option we have not explored is an environment variable
> to loosen git's requirement. I'm thinking something like
> GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
> when git-daemon uses --user.
> 
> That would leave all existing setups working, but would
> still enable the extra protections for people not running
> git-daemon (and people who use git via sudo could choose to
> set it, too, if they would prefer that to setting up HOME).

So here's what I came up with. I tried to make the exception as tight as
possible by checking that $HOME was actually the problem, as that is the
common problem (you switch users, but HOME is pointing to the old user).

It means that we would still die if ~root is readable, but
~root/.gitconfig is not (or ~root/.config is not). I think that is
probably a good thing, though, as it is an indication of something more
interesting going on that the user should investigate.

Given how tight the exception is, I almost wonder if we should drop the
environment variable completely, and just never complain about this case
(in other words, just make it a loosening of 96b9e0e3).

-Peff

---
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6a875f2..e004ee8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -806,6 +806,15 @@ for further details.
temporarily to avoid using a buggy `/etc/gitconfig` file while
waiting for someone with sufficient permissions to fix it.
 
+'GIT_INACCESSIBLE_HOME_OK'::
+   Normally git will complain and die if it cannot access
+   `$HOME/.gitconfig`. If this variable is set to "1", then
+   a `$HOME` directory which cannot be accessed will not be
+   considered an error. This can be useful if you are switching
+   user ids without resetting `$HOME`, and are willing to take the
+   risk that some configuration options might not be used (because
+   git could not read the config file that contained them).
+
 'GIT_FLUSH'::
If this environment variable is set to "1", then commands such
as 'git blame' (in incremental mode), 'git rev-list', 'git log',
diff --git a/cache.h b/cache.h
index e1e8ce8..01f300f 100644
--- a/cache.h
+++ b/cache.h
@@ -358,6 +358,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define GIT_NOTES_REWRITE_REF_ENVIRONMENT "GIT_NOTES_REWRITE_REF"
 #define GIT_NOTES_REWRITE_MODE_ENVIRONMENT "GIT_NOTES_REWRITE_MODE"
 #define GIT_LITERAL_PATHSPECS_ENVIRONMENT "GIT_LITERAL_PATHSPECS"
+#define GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT "GIT_INACCESSIBLE_HOME_OK"
 
 /*
  * This environment variable is expected to contain a boolean indicating
diff --git a/daemon.c b/daemon.c
index 131b049..6c56cc0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1091,7 +1091,7 @@ static void drop_privileges(struct credentials *cred)
if (cred && (initgroups(cred->pass->pw_name, cred->gid) ||
setgid (cred->gid) || setuid(cred->pass->pw_uid)))
die("cannot drop privileges");
-   setenv("GIT_CONFIG_INACCESSIBLE_HOME_OK", "1", 0);
+   setenv(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, "1", 0);
 }
 
 static struct credentials *prepare_credentials(const char *user_name,
diff --git a/wrapper.c b/wrapper.c
index bac59d2..644f867 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -416,11 +416,52 @@ int access_or_die(const char *path, int mode)
return ret;
 }
 
+/*
+ * Returns true iff the path is under $HOME, that directory is inaccessible,
+ * and the user has told us through the environment that an inaccessible HOME
+ * is OK. The results are cached so that repeated calls will not make multiple
+ * syscalls.
+ */
+static int inaccessible_home_ok(const char *path)
+{
+   static int gentle = -1;
+   static int home_inaccessible = -1;
+   const char *home;
+
+   if (gentle < 0)
+   gentle = git_env_bool(GIT_INACCESSIBLE_HOME_OK_ENVIRONMENT, 0);
+   if (!gentle)
+   return 0;
+
+   home = getenv("HOME");
+   if (!home)
+   return 0;
+   /*
+* We do not bother with normalizing PATHs to avoid symlinks
+* here, since the point is to catch paths that are
+* constructed as "$HOME/...".
+*/
+   if (prefixcmp(path, home) && path[strlen(home)] == '/')
+   return 0;
+
+   if (home_inaccessible < 0)
+   home_inaccessible = access(home, R_OK|X_OK) < 0 && errno == 
EACCES;
+   return home_inaccessible;
+}
+
 int access_or_die(const char *path, int mode)
 {
int ret = access(path, mode);
-   if (ret && errno != ENOENT && errno != ENOTDIR)
+   if (ret && errno != ENOENT && errno != ENOTDIR) {
+   /*
+* We do not have to worry about clobbering errno
+* in inaccessible_home_ok, because we get either EACCES
+* again, or we die.
+*/
+   if (errno == EACCES && 

Re: [PATCH] fast-export: fix argument name in error messages

[Junio C Hamano]

Paul Price  writes:

> The --signed-tags argument is plural, while error messages referred
> to --signed-tag (singular).  Tweak error messages to correspond to the
> argument.
>
> Signed-off-by: Paul Price 
> ---
> First submission; please report any formatting or style errors privately.

The patch is trivially whitespace-damaged but was easy enough to
correct locally here.

Applied; thanks.

>
> builtin/fast-export.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 77dffd1..ad9d0c4 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -43,7 +43,7 @@ static int parse_opt_signed_tag_mode(const struct option 
> *opt,
>   else if (!strcmp(arg, "strip"))
>   signed_tag_mode = STRIP;
>   else
> - return error("Unknown signed-tag mode: %s", arg);
> + return error("Unknown signed-tags mode: %s", arg);
>   return 0;
> }
>
> @@ -416,7 +416,7 @@ static void handle_tag(const char *name, struct tag *tag)
>   switch(signed_tag_mode) {
>   case ABORT:
>   die ("Encountered signed tag %s; use "
> -  "--signed-tag= to handle it.",
> +  "--signed-tags= to handle it.",
>sha1_to_hex(tag->object.sha1));
>   case WARN:
>   warning ("Exporting signed tag %s",
--
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] pull: fail early if we know we can't merge from upstream

[Junio C Hamano]

Carlos Martín Nieto  writes:

> On Thu, 2013-04-11 at 10:37 -0700, Junio C Hamano wrote:
>
>> > +  fetch=$(git config --get-all "remote.$use_remote.fetch")
>> > +  if [ -z "$fetch" ]; then
>> > +  return
>> > +  fi
>> 
>> Hmm, it is probably correct to punt on this case, but it defeats
>> large part of the effect of your effort, doesn't it? We fetch what
>> is covered by remote.$name.fetch _and_ what need to complete the
>> merge operation (otherwise branch.$name.merge that is not covered by
>> remote.$there.fetch will not work).  So
>> 
>> [remote "origin"]
>> url = $over_there
>> [branch "master"]
>> remote = origin
>> merge = refs/heads/master
>> 
>> would still fetch refs/heads/master from there and merge it.
>
> If you run 'git pull' in this situation, then everything's fine and the
> right thing gets merged.

My mistake.  You are trying to reject an obviously bad case early,
and because this is an obviously good case, you just let it be
handled in the original codeflow (which should not find any issues
in this set-up).
--
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: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Mike Galbraith]

On Fri, 2013-04-12 at 09:08 -0700, Junio C Hamano wrote: 
> Jeff King  writes:
> 
> >> How about "and make sure any Git configuration files", since there
> >> might not be any Git configuration files.
> >
> > Yeah, that is better. Thanks.
> 
> OK, then...
> 
> -- >8 --
> Subject: [PATCH] doc: clarify that "git daemon --user=" option does not 
> export HOME=~user
> 
> Signed-off-by: Jeff King 
> Helped-by: W. Trevor King 
> Signed-off-by: Junio C Hamano 
> ---
>  Documentation/git-daemon.txt | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index 7e5098a..2ac07ba 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -147,6 +147,13 @@ OPTIONS
>  Giving these options is an error when used with `--inetd`; use
>  the facility of inet daemon to achieve the same before spawning
>  'git daemon' if needed.
> ++
> +Like many programs that switch user id, the daemon does not reset
> +environment variables such as `$HOME` when it runs git programs,
> +e.g. `upload-pack` and `receive-pack`. When using this option, you
> +may also want to set and export `HOME` to point at the home
> +directory of `` before starting the daemon, and make sure any
> +Git configuration files in that directory are readable by ``.

The "you may want to.." is perhaps a little understated given it will
fail -EGOAWAY if git-daemon is started via init scripts if you don't.
(but otoh, that's enough of a hint to anyone setting the thing up, no
need to write paragraphs of legal-beagle boiler-plate for dinky bug;)

-Mike 

--
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] help: mark common_guides[] as translatable

[Philip Oakley]

From: "Simon Ruderich" 
Sent: Friday, April 12, 2013 2:51 PM

Signed-off-by: Simon Ruderich 
---
On Tue, Apr 02, 2013 at 11:39:51PM +0100, Philip Oakley wrote:

--- a/help.c
+++ b/help.c
@@ -240,6 +241,23 @@ void list_common_cmds_help(void)
 }
 }

+void list_common_guides_help(void)
+{
+ int i, longest = 0;
+
+ for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
+ if (longest < strlen(common_guides[i].name))
+ longest = strlen(common_guides[i].name);
+ }
+
+ puts(_("The common Git guides are:\n"));
+ for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
+ printf("   %s   ", common_guides[i].name);
+ mput_char(' ', longest - strlen(common_guides[i].name));
+ puts(_(common_guides[i].help));


common_guides[] is used here, but without N_() not picked up by
xgettext when creating the pot file.


Yes. I mucked that up when I hacked the generate-cmdlist.sh to create 
this list.


Acked-by: Philip Oakley 

At some point it is on my TODO list to extend the guide list mechanism 
to all the community generated guides (option -gg) by extending the 
command-list.txt file and the shell script.




Regards
Simon

builtin/help.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 034c36c..062957f 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -419,13 +419,13 @@ static struct {
 const char *name;
 const char *help;
} common_guides[] = {
- { "attributes", "Defining attributes per path" },
- { "glossary", "A Git glossary" },
- { "ignore", "Specifies intentionally untracked files to ignore" },
- { "modules", "Defining submodule properties" },
- { "revisions", "Specifying revisions and ranges for Git" },
- { "tutorial", "A tutorial introduction to Git (for version 1.5.1 or 
newer)" },

- { "workflows", "An overview of recommended workflows with Git"},
+ { "attributes", N_("Defining attributes per path") },
+ { "glossary", N_("A Git glossary") },
+ { "ignore", N_("Specifies intentionally untracked files to 
ignore") },

+ { "modules", N_("Defining submodule properties") },
+ { "revisions", N_("Specifying revisions and ranges for Git") },
+ { "tutorial", N_("A tutorial introduction to Git (for version 1.5.1 
or newer)") },
+ { "workflows", N_("An overview of recommended workflows with 
Git") },

};

static void list_common_guides_help(void)
--
1.8.2.481.g0d034d4

--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9


--
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: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 09:08:31AM -0700, Junio C Hamano wrote:

> OK, then...

> -- >8 --
> Subject: [PATCH] doc: clarify that "git daemon --user=" option does not 
> export HOME=~user

I'd add this motiviation to the body of the commit message:

  The fact that we don't set $HOME may confuse admins who
  expect $HOME/.gitconfig to be respected. And worse, since
  96b9e0e3, a git-daemon started by root is likely to fail
  to run at all, as the user we switch to generally cannot
  read ~root.

This still feels ugly, like we are documenting some gotcha
that is going to hit most admins, when we could be helping
them in the code.

One option we have not explored is an environment variable
to loosen git's requirement. I'm thinking something like
GIT_INACCESSIBLE_HOMEDIR_OK, which could be set by default
when git-daemon uses --user.

That would leave all existing setups working, but would
still enable the extra protections for people not running
git-daemon (and people who use git via sudo could choose to
set it, too, if they would prefer that to setting up HOME).

-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] Documentation: distinguish between ref and offset deltas in pack-format

[Junio C Hamano]

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 v2] Fix various typos and grammaros

[Junio C Hamano]

Stefano Lattarini  writes:

> Hi Junio.
>
> On 04/12/2013 02:45 AM, Junio C Hamano wrote:
>> Stefano Lattarini  writes:
>> 
>> How much of this stuff have interact with real changes that are in
>> flight, with various doneness cooking in different integration
>> branches?
>> 
> I don't know, since I only follow the master branch of Git.  And
> honestly, I don't have time right now to go checking for possible
> conflicts, or to resubmit ...   But I see Jonathan has taken up
> the ball on this (thanks Jonathan!), so I'll leave the matter to
> him.

Yeah, that's OK.  Thanks.

> Next time I'll try to prepare a patch broken up in more digestible
> pieces, so that it will be easier for you to deal with conflicts,
> and/or to selectively decide which fixes are worth applying.
>
> Thanks, and sorry for the confusion,
>   Stefano
--
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: [RFC/PATCH maint 0/10] Re: [PATCH v2] Fix various typos and grammaros

[Junio C Hamano]

Jonathan Nieder  writes:

> Hi,
>
> Junio C Hamano wrote:
>
>> How much of this stuff have interact with real changes that are in
>> flight, with various doneness cooking in different integration
>> branches?
>
> All except the t3511-cherry-pick-x.sh change apply cleanly to
> "maint" and merge without trouble with master and pu.
>
> Here is a split-up version.

Thanks.

> I haven't looked closely at the patch,
> even to sanity check it --- one of the main points of splitting it
> this way is to make it easier to review with reference to code
> borrowed from other projects.

Sure.

> Some of these patches need more work if they are to be applied.  For
> example, git-gui is maintained separately and should not be patched
> from the toplevel.

True.

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: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Junio C Hamano]

Jeff King  writes:

>> How about "and make sure any Git configuration files", since there
>> might not be any Git configuration files.
>
> Yeah, that is better. Thanks.

OK, then...

-- >8 --
Subject: [PATCH] doc: clarify that "git daemon --user=" option does not 
export HOME=~user

Signed-off-by: Jeff King 
Helped-by: W. Trevor King 
Signed-off-by: Junio C Hamano 
---
 Documentation/git-daemon.txt | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 7e5098a..2ac07ba 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -147,6 +147,13 @@ OPTIONS
 Giving these options is an error when used with `--inetd`; use
 the facility of inet daemon to achieve the same before spawning
 'git daemon' if needed.
++
+Like many programs that switch user id, the daemon does not reset
+environment variables such as `$HOME` when it runs git programs,
+e.g. `upload-pack` and `receive-pack`. When using this option, you
+may also want to set and export `HOME` to point at the home
+directory of `` before starting the daemon, and make sure any
+Git configuration files in that directory are readable by ``.
 
 --enable=::
 --disable=::
-- 
1.8.2.1-472-g6c5785c

--
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/4] log -L: improve comments in process_all_files()

[Thomas Rast]

The funny range assignment in process_all_files() had me sidetracked
while investigating what led to the previous commit.  Let's improve
the comments.

Signed-off-by: Thomas Rast 
---
 line-log.c | 19 ---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/line-log.c b/line-log.c
index 44d1cd5..4bbb09b 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1095,11 +1095,24 @@ static int process_all_files(struct line_log_data 
**range_out,
 
for (i = 0; i < queue->nr; i++) {
struct diff_ranges *pairdiff = NULL;
-   if (process_diff_filepair(rev, queue->queue[i], *range_out, 
&pairdiff)) {
+   struct diff_filepair *pair = queue->queue[i];
+   if (process_diff_filepair(rev, pair, *range_out, &pairdiff)) {
+   /*
+* Store away the diff for later output.  We
+* tuck it in the ranges we got as _input_,
+* since that's the commit that caused the
+* diff.
+*
+* NEEDSWORK not enough when we get around to
+* doing something interesting with merges;
+* currently each invocation on a merge parent
+* trashes the previous one's diff.
+*
+* NEEDSWORK tramples over data structures not owned 
here
+*/
struct line_log_data *rg = range;
changed++;
-   /* NEEDSWORK tramples over data structures not owned 
here */
-   while (rg && strcmp(rg->path, 
queue->queue[i]->two->path))
+   while (rg && strcmp(rg->path, pair->two->path))
rg = rg->next;
assert(rg);
rg->pair = diff_filepair_dup(queue->queue[i]);
-- 
1.8.2.1.567.g8ad0f43

--
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/4] log -L: store the path instead of a diff_filespec

[Thomas Rast]

line_log_data has held a diff_filespec* since the very early versions
of the code.  However, the only place in the code where we actually
need the full filespec is parse_range_arg(); in all other cases, we
are only interested in the path, so there is hardly a reason to store
a filespec.  Even worse, it causes a lot of redundant ->spec->path
pointer dereferencing.

And *even* worse, it caused the following bug.  If you merge a rename
with a modification to the old filename, like so:

  * Merge
  | \
  |  * Modify foo
  |  |
  *  | Rename foo->bar
  | /
  * Create foo

we internally -- in process_ranges_merge_commit() -- scan all parents.
We are mainly looking for one that doesn't have any modifications, so
that we can assign all the blame to it and simplify away the merge.
In doing so, we run the normal machinery on all parents in a loop.
For each parent, we prepare a "working set" line_log_data by making a
copy with line_log_data_copy(), which does *not* make a copy of the
spec.

Now suppose the rename is the first parent.  The diff machinery tells
us that the filepair is ('foo', 'bar').  We duly update the path we
are interested in:

  rg->spec->path = xstrdup(pair->one->path);

But that 'struct spec' is shared between the output line_log_data and
the original input line_log_data.  So we just wrecked the state of
process_ranges_merge_commit().  When we get around to the second
parent, the ranges tell us we are interested in a file 'foo' while the
commits touch 'bar'.

So most of this patch is just s/->spec->path/->path/ and associated
management changes.  This implicitly fixes the bug because we removed
the shared parts between input and output of line_log_data_copy(); it
is now safe to overwrite the path in the copy.

There's one only somewhat related change: the comment in
process_all_files() explains the reasoning behind using 'range' there.
That bit of half-correct code had me sidetracked for a while.

Signed-off-by: Thomas Rast 
---
 line-log.c  | 45 -
 line-log.h  |  8 ++--
 t/t4211-line-log.sh |  2 +-
 3 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/line-log.c b/line-log.c
index 85c7c24..44d1cd5 100644
--- a/line-log.c
+++ b/line-log.c
@@ -265,7 +265,7 @@ static void free_line_log_data(struct line_log_data *r)
if (insertion_point)
*insertion_point = NULL;
while (p) {
-   int cmp = strcmp(p->spec->path, path);
+   int cmp = strcmp(p->path, path);
if (!cmp)
return p;
if (insertion_point && cmp < 0)
@@ -275,22 +275,26 @@ static void free_line_log_data(struct line_log_data *r)
return NULL;
 }
 
+/*
+ * Note: takes ownership of 'path', which happens to be what the only
+ * caller needs.
+ */
 static void line_log_data_insert(struct line_log_data **list,
-struct diff_filespec *spec,
+char *path,
 long begin, long end)
 {
struct line_log_data *ip;
-   struct line_log_data *p = search_line_log_data(*list, spec->path, &ip);
+   struct line_log_data *p = search_line_log_data(*list, path, &ip);
 
if (p) {
range_set_append_unsafe(&p->ranges, begin, end);
sort_and_merge_range_set(&p->ranges);
-   free_filespec(spec);
+   free(path);
return;
}
 
p = xcalloc(1, sizeof(struct line_log_data));
-   p->spec = spec;
+   p->path = path;
range_set_append(&p->ranges, begin, end);
if (ip) {
p->next = ip->next;
@@ -354,7 +358,7 @@ static void dump_line_log_data(struct line_log_data *r)
 {
char buf[4096];
while (r) {
-   snprintf(buf, 4096, "file %s\n", r->spec->path);
+   snprintf(buf, 4096, "file %s\n", r->path);
dump_range_set(&r->ranges, buf);
r = r->next;
}
@@ -561,7 +565,7 @@ static const char *nth_line(void *data, long line)
 
for_each_string_list_item(item, args) {
const char *name_part, *range_part;
-   const char *full_name;
+   char *full_name;
struct diff_filespec *spec;
long begin = 0, end = 0;
 
@@ -584,7 +588,7 @@ static const char *nth_line(void *data, long line)
 
if (parse_range_arg(range_part, nth_line, &cb_data,
lines, &begin, &end,
-   spec->path))
+   full_name))
die("malformed -L argument '%s'", range_part);
if (begin < 1)
begin = 1;
@@ -593,8 +597,9 @@ static const char *nth_line(void *data, long line)
begin--;
if (lines < end || lines < begin)
die("file %s has only %ld l

[PATCH 1/4] t4211: pass -M to 'git log -M -L...' test

[Thomas Rast]

Embarrassingly, the -M test did not actually invoke -M, and thus not
really test the feature.
---
 t/t4211-line-log.sh   |  2 +-
 t/t4211/expect.move-support-f | 56 ---
 2 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 2341351..2a67e31 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -37,7 +37,7 @@ canned_test "-L 20:a.c simple" end-of-file
 canned_test "-L '/long f/',/^}/:a.c -L /main/,/^}/:a.c simple" two-ranges
 canned_test "-L 24,+1:a.c simple" vanishes-early
 
-canned_test "-L '/long f/,/^}/:b.c' move-support" move-support-f
+canned_test "-M -L '/long f/,/^}/:b.c' move-support" move-support-f
 
 canned_test "-L 4,12:a.c -L :main:a.c simple" multiple
 canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping
diff --git a/t/t4211/expect.move-support-f b/t/t4211/expect.move-support-f
index 78a8cf1..c905e01 100644
--- a/t/t4211/expect.move-support-f
+++ b/t/t4211/expect.move-support-f
@@ -19,22 +19,62 @@ diff --git a/b.c b/b.c
return s;
  }
 
-commit e6da343666244ea9e67cbe3f3bd26da860f9fe0e
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
 Author: Thomas Rast 
-Date:   Thu Feb 28 10:49:28 2013 +0100
+Date:   Thu Feb 28 10:45:16 2013 +0100
 
-move file
+touch both functions
 
-diff --git a/b.c b/b.c
 /dev/null
-+++ b/b.c
-@@ -0,0 +4,9 @@
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -3,9 +3,9 @@
+-int f(int x)
 +long f(long x)
+ {
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
+   return s;
+ }
+
+commit f04fb20f2c77850996cba739709acc6faecc58f7
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:44:55 2013 +0100
+
+change f()
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -3,8 +3,9 @@
+ int f(int x)
+ {
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
++  return s;
+ }
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:44:48 2013 +0100
+
+initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
 b/a.c
+@@ -0,0 +3,8 @@
++int f(int x)
 +{
 +  int s = 0;
 +  while (x) {
 +  x >>= 1;
 +  s++;
 +  }
-+  return s;
 +}
-- 
1.8.2.1.567.g8ad0f43

--
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/4] log -L: test merge of parallel modify/rename

[Thomas Rast]

This tests a toy example of a history like

  * Merge
  | \
  |  * Modify foo
  |  |
  *  | Rename foo->bar
  | /
  * Create foo

Current log -L fails on this; we'll fix it in the next commit.

Signed-off-by: Thomas Rast 
---
 t/t4211-line-log.sh  |  16 +++-
 t/t4211/expect.parallel-change-f-to-main | 160 +++
 t/t4211/history.export   |  80 +++-
 3 files changed, 250 insertions(+), 6 deletions(-)
 create mode 100644 t/t4211/expect.parallel-change-f-to-main

diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 2a67e31..bba0b09 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -8,13 +8,20 @@ test_expect_success 'setup (import history)' '
git reset --hard
 '
 
-canned_test () {
-   test_expect_success "$1" "
-   git log $1 >actual &&
-   test_cmp \"\$TEST_DIRECTORY\"/t4211/expect.$2 actual
+canned_test_1 () {
+   test_expect_$1 "$2" "
+   git log $2 >actual &&
+   test_cmp \"\$TEST_DIRECTORY\"/t4211/expect.$3 actual
"
 }
 
+canned_test () {
+   canned_test_1 success "$@"
+}
+canned_test_failure () {
+   canned_test_1 failure "$@"
+}
+
 test_bad_opts () {
test_expect_success "invalid args: $1" "
test_must_fail git log $1 2>errors &&
@@ -38,6 +45,7 @@ canned_test "-L '/long f/',/^}/:a.c -L /main/,/^}/:a.c 
simple" two-ranges
 canned_test "-L 24,+1:a.c simple" vanishes-early
 
 canned_test "-M -L '/long f/,/^}/:b.c' move-support" move-support-f
+canned_test_failure "-M -L ':f:b.c' parallel-change" parallel-change-f-to-main
 
 canned_test "-L 4,12:a.c -L :main:a.c simple" multiple
 canned_test "-L 4,18:a.c -L :main:a.c simple" multiple-overlapping
diff --git a/t/t4211/expect.parallel-change-f-to-main 
b/t/t4211/expect.parallel-change-f-to-main
new file mode 100644
index 000..052def8
--- /dev/null
+++ b/t/t4211/expect.parallel-change-f-to-main
@@ -0,0 +1,160 @@
+commit 0469c60bc4837d52d97b1f081dec5f98dea20fed
+Merge: ba227c6 6ce3c4f
+Author: Thomas Rast 
+Date:   Fri Apr 12 16:16:24 2013 +0200
+
+Merge across the rename
+
+
+commit 6ce3c4ff690136099bb17e1a8766b75764726ea7
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:49:50 2013 +0100
+
+another simple change
+
+diff --git a/b.c b/b.c
+--- a/b.c
 b/b.c
+@@ -4,14 +4,14 @@
+ long f(long x)
+ {
+   int s = 0;
+   while (x) {
+-  x >>= 1;
++  x /= 2;
+   s++;
+   }
+   return s;
+ }
+ 
+ /*
+  * This is only an example!
+  */
+ 
+
+commit ba227c6632349700fbb957dec2b50f5e2358be3f
+Author: Thomas Rast 
+Date:   Fri Apr 12 16:15:57 2013 +0200
+
+change on another line of history while rename happens
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -4,14 +4,14 @@
+ long f(long x)
+ {
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
+   return s;
+ }
+ 
+ /*
+- * This is only an example!
++ * This is only a short example!
+  */
+ 
+
+commit 39b6eb2d5b706d3322184a169f666f25ed3fbd00
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:45:41 2013 +0100
+
+touch comment
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -3,14 +3,14 @@
+ long f(long x)
+ {
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
+   return s;
+ }
+ 
+ /*
+- * A comment.
++ * This is only an example!
+  */
+ 
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:45:16 2013 +0100
+
+touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -3,14 +3,14 @@
+-int f(int x)
++long f(long x)
+ {
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
+   return s;
+ }
+ 
+ /*
+  * A comment.
+  */
+ 
+
+commit f04fb20f2c77850996cba739709acc6faecc58f7
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:44:55 2013 +0100
+
+change f()
+
+diff --git a/a.c b/a.c
+--- a/a.c
 b/a.c
+@@ -3,13 +3,14 @@
+ int f(int x)
+ {
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
++  return s;
+ }
+ 
+ /*
+  * A comment.
+  */
+ 
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast 
+Date:   Thu Feb 28 10:44:48 2013 +0100
+
+initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
 b/a.c
+@@ -0,0 +3,13 @@
++int f(int x)
++{
++  int s = 0;
++  while (x) {
++  x >>= 1;
++  s++;
++  }
++}
++
++/*
++ * A comment.
++ */
++
diff --git a/t/t4211/history.export b/t/t4211/history.export
index c159794..f9f41e2 100644
--- a/t/t4211/history.export
+++ b/t/t4211/history.export
@@ -325,6 +325,82 @@ move within the file
 from :17
 M 100644 :18 b.c
 
-reset refs/heads/master
-from :19
+blob
+mark :20
+data 243
+#include 
+#include 
+
+long f(long x)
+{
+   int s = 0;
+   while (x) {
+   x >>= 1;
+   s++;
+   }
+   return s;
+}
+
+/*
+ * 

[PATCH 0/4] fix log -L -M

[Thomas Rast]

Ok, so this was not quite as bad as I feared.  The move support as far
as I had thought to test it previously actually worked ok, it just
wasn't tested (which is embarrassing enough).

The bug fixed in patch 3 is a bit more involved and only triggered by
history that merges a rename with a modification to the original
filename.  Luckily -- I guess -- there are several cases of this
happening in git.git around the big builtin/ move in 81b50f3 (Move
'builtin-*' into a 'builtin/' subdirectory, 2010-02-22).  So my fuzz
tests found that problem.  You can reproduce with e.g.

  git log -M -L:cmd_format_patch:builtin/log.c

in any git.git.


Thomas Rast (4):
  t4211: pass -M to 'git log -M -L...' test
  log -L: test merge of parallel modify/rename
  log -L: store the path instead of a diff_filespec
  log -L: improve comments in process_all_files()

 line-log.c   |  62 +++-
 line-log.h   |   8 +-
 t/t4211-line-log.sh  |  18 +++-
 t/t4211/expect.move-support-f|  56 +--
 t/t4211/expect.parallel-change-f-to-main | 160 +++
 t/t4211/history.export   |  80 +++-
 6 files changed, 344 insertions(+), 40 deletions(-)
 create mode 100644 t/t4211/expect.parallel-change-f-to-main

-- 
1.8.2.1.567.g8ad0f43

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

[RFC/PATCH] push: introduce implicit push

[Ramkumar Ramachandra]

Currently, there is no way to invoke 'git push' without explicitly
specifying the destination to push to as the first argument.  When
pushing several branches, this information is often available in
branch..pushremote, falling back to branch..remote.  So,
we can use this information to create a more pleasant push experience.
You can now do:

$ git push master next pu

If the branches master, next, and pu have different remotes, do_push()
will be executed three times on the three different remotes.

NOTE:

Pushing a non-branch ref still uses the current branch's
configuration, and this is wrong.  We need to find a way to fix
remote_get_1() without breaking existing features.

Signed-off-by: Ramkumar Ramachandra 
---
 This is a very rough patch, meant to illustrate how to build our
 implicit push feature.  I think it's a really good idea, and will
 polish the patch after I get feedback.

 builtin/push.c | 56 ++--
 remote.c   | 90 ++
 remote.h   |  9 ++
 3 files changed, 152 insertions(+), 3 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 909c34d..e8b667c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -26,6 +26,12 @@ static int refspec_nr;
 static int refspec_alloc;
 static int default_matching_used;
 
+static void reset_refspecs()
+{
+   refspec_nr = 0;
+   refspec_alloc = 0;
+}
+
 static void add_refspec(const char *ref)
 {
refspec_nr++;
@@ -277,6 +283,11 @@ static void advise_ref_needs_force(void)
advise(_(message_advice_ref_needs_force));
 }
 
+static int is_possible_refspec(const char *name) {
+   /* TODO: check that name looks like a refspec */
+   return !is_configured_remote(name) && !strstr(name, "://");
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
int err;
@@ -319,10 +330,9 @@ static int push_with_options(struct transport *transport, 
int flags)
return 1;
 }
 
-static int do_push(const char *repo, int flags)
+static int do_push(const char *repo, struct remote *remote, int flags)
 {
int i, errs;
-   struct remote *remote = pushremote_get(repo);
const char **url;
int url_nr;
 
@@ -414,6 +424,8 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
int tags = 0;
int rc;
const char *repo = NULL;/* default repository */
+   struct remote *remote;
+
struct option options[] = {
OPT__VERBOSITY(&verbosity),
OPT_STRING( 0 , "repo", &repo, N_("repository"), 
N_("repository")),
@@ -455,11 +467,49 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
add_refspec("refs/tags/*");
 
if (argc > 0) {
+   if (!strcmp(argv[0], "--") || is_possible_refspec(argv[0])) {
+   struct refspec_with_remote *refspec_remotes;
+   int i, pushtimes;
+
+   /* Attempt to fetch remotes for each refspec */
+   if (!strcmp(argv[0], "--"))
+   set_refspecs(argv + 1, argc - 1);
+   else
+   set_refspecs(argv, argc);
+   refspec_remotes = pushremote_get_for_refspec(refspec_nr,
+   refspec);
+   if (!refspec_remotes)
+   die(_("internal error: refspec_remotes() 
returned NULL"));
+
+   /* TODO: collect the refspecs for each
+* remote and batch the do_push() for
+* each remote
+*/
+   for (i = 0, pushtimes = refspec_nr; i < pushtimes; i++) 
{
+   struct strbuf sb = STRBUF_INIT;
+
+   /* TODO: clean up this nonsense */
+   if (refspec_remotes[i].refspec->dst)
+   strbuf_addf(&sb, "%s:%s",
+   refspec_remotes[i].refspec->src,
+   
refspec_remotes[i].refspec->dst);
+   else
+   strbuf_addf(&sb, "%s",
+   
refspec_remotes[i].refspec->src);
+   reset_refspecs();
+   set_refspecs((const char **) &sb.buf, 1);
+   rc = do_push(NULL, refspec_remotes[i].remote, 
flags);
+   if (rc == -1)
+   usage_with_options(push_usage, options);
+   }
+   return rc;
+   }
repo = argv[0];
+   remote = pushremote_get(repo);
set_refspecs(argv + 1, argc - 1);
}

Re: [PATCH v1 23/45] check-ignore: convert to use parse_pathspec

[Adam Spiers]

On Fri, Mar 15, 2013 at 01:06:38PM +0700, Nguyễn Thái Ngọc Duy wrote:
> check-ignore (at least the test suite) seems to rely on the pattern
> order. PATHSPEC_KEEP_ORDER is introduced to explictly express this.
> The lack of PATHSPEC_MAXDEPTH_VALID is sufficient because it's the
> only flag that reorders pathspecs, but it's less obvious that way.

Sorry for the slow response - I only just noticed this today.  (It
would be useful if any future patches to check-ignore Cc: me
explicitly, to catch my mail filters.)

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/check-ignore.c | 34 +-
>  pathspec.c |  6 +-
>  pathspec.h |  1 +
>  t/t0008-ignores.sh |  8 
>  4 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> index 0240f99..6e55f06 100644
> --- a/builtin/check-ignore.c
> +++ b/builtin/check-ignore.c
> @@ -53,14 +53,14 @@ static void output_exclude(const char *path, struct 
> exclude *exclude)
>   }
>  }
>  
> -static int check_ignore(const char *prefix, const char **pathspec)
> +static int check_ignore(int argc, const char **argv, const char *prefix)
>  {
>   struct dir_struct dir;
> - const char *path, *full_path;
>   char *seen;
>   int num_ignored = 0, dtype = DT_UNKNOWN, i;
>   struct path_exclude_check check;
>   struct exclude *exclude;
> + struct pathspec pathspec;
>  
>   /* read_cache() is only necessary so we can watch out for submodules. */
>   if (read_cache() < 0)
> @@ -70,31 +70,39 @@ static int check_ignore(const char *prefix, const char 
> **pathspec)
>   dir.flags |= DIR_COLLECT_IGNORED;
>   setup_standard_excludes(&dir);
>  
> - if (!pathspec || !*pathspec) {
> + if (!argc) {

Is there a compelling reason for introducing argc as a new parameter
to check_ignore(), other than simplifying the above line?  And why
rename the pathspec parameter to argv?  Both these changes are
misleading AFAICS, since paths provided to check_ignore() can come
from sources other than CLI arguments (i.e. via --stdin).  

The introduction of argc also makes it possible to invoke
check_ignore() with arguments which are not self-consistent.

I haven't been following your pathspec work, but FWIW the other
changes in this patch look reasonable at a glance.

Thanks,
Adam
--
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: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Jeff King]

On Fri, Apr 12, 2013 at 07:26:36AM -0400, W. Trevor King wrote:

> On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:
> >   Like many programs that switch user id, the daemon does not reset
> >   environment variables such a `$HOME` when it runs git programs like
> >   `upload-pack` and `receive-pack`. When using this option, you may also
> >   want to set and export `HOME` to point at the home directory of
> >   `` before starting the daemon, and make sure the Git
> >   configuration file in that directory are readable by ``.
> 
> How about "and make sure any Git configuration files", since there
> might not be any Git configuration files.

Yeah, that is better. Thanks.

-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: regression: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[Evan Priestley]

Jonathan Nieder  wrote:

> I'm not sure whether to keep 96b9e0e (config: treat user and xdg
> config permission problem as errors) in the long run, BTW.
> 
> Insights welcome.

For what it's worth, here's an anecdote about this:

I work on some open source software which includes a web-based repository 
browser for Git, somewhat similar to gitweb. We implement this partially by 
executing `git` commands from the webserver (usually Apache). For example, we 
run `git cat-file …` to retrieve file content to show to the user.

After this change, a number of users who manage installs of the software are 
hitting "fatal: unable to access '/root/.config/git/config': Permission denied" 
while browsing repositories, because their Apache runs as some unprivileged 
user (like "apache" or "www-data") but with HOME=/root. We've seen about half a 
dozen reports of this now, so I believe this sort of setup is at least somewhat 
common and not just a bizarre one-off user with a broken environment. Users 
generally have difficulty resolving this error on their own, as it's not 
obvious that this boils down to an Apache environmental issue.

We'll likely resolve this by running `HOME=/ git ...` instead of `git ...` when 
we execute commands (or some more finessed version of that, but basically 
pointing HOME at some dummy readable directory). From cursory investigation, it 
appears we can't avoid this fatal with more surgical settings like GIT_CONFIG 
or XDG_CONFIG_HOME, since git still ends up looking in HOME and fataling 
anyway. This fix is a bit clunky, but not really a big deal.

I imagine our use case is fairly unusual, but I wanted to relate it in case 
it's helpful in balancing concerns here. If I've missed a more reasonable 
approach to solving this than redirecting HOME, please let me know, but it 
looks like that's more or less what the git-daemon patch is doing too.

Thanks,
Evan Priestley

--
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] fast-export: fix argument name in error messages

[Paul Price]

The --signed-tags argument is plural, while error messages referred
to --signed-tag (singular).  Tweak error messages to correspond to the
argument.

Signed-off-by: Paul Price 
---
First submission; please report any formatting or style errors privately.

builtin/fast-export.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 77dffd1..ad9d0c4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -43,7 +43,7 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
else if (!strcmp(arg, "strip"))
signed_tag_mode = STRIP;
else
-   return error("Unknown signed-tag mode: %s", arg);
+   return error("Unknown signed-tags mode: %s", arg);
return 0;
}

@@ -416,7 +416,7 @@ static void handle_tag(const char *name, struct tag *tag)
switch(signed_tag_mode) {
case ABORT:
die ("Encountered signed tag %s; use "
-"--signed-tag= to handle it.",
+"--signed-tags= to handle it.",
 sha1_to_hex(tag->object.sha1));
case WARN:
warning ("Exporting signed tag %s",
-- 
1.8.1.1

--
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: Git crash in Ubuntu 12.04

[Konstantin Khomoutov]

On Fri, 12 Apr 2013 18:58:24 +0530
Sivaram Kannan  wrote:

> > ^^^ Try to issue the
> >
> > $ ulimit -c unlimited
> 
> Have set the git user's crash limit to 1GB in
> /etc/security/limits.conf and still getting the same error when
> issuing gdb to the crash file.

Yep, suppsedly in Ubuntu it's not that easy to just get a plain old
coredump file -- see below.

> > command in your shell before attempting the cloning -- this should
> > remove the upper limit on the core file size.  And try look for the
> > core file in the current directory after the crash occurs.  I'm not
> > sure Ubuntu's "crash interceptor" won't kick in, but just in case...
> 
> You mean, /usr/bin/git? crash file for git is getting created each
> time it crashes in /var/crash.
> 
> Can you please tell me what else I could try?

Googling for "ubuntu+disable+crash" turns up that your Git crashes are
handled by a system-wide tool called "apport" [1].

Considering this, I would try to explore two routes:

* [1] Tells that apport has a special tool, apport-retrace, which is
  said to be able to download available matching debug packages, if
  any, and generate the stack traces.  Basically this would do what
  Thomas advised you to attempt to do using GDB.

* Try to disable apprort permanently and then crash Git normally,
  so that apport does not interfere with the crash and the kernel is
  able to generate a regular core file in your current directory.
  Be sure to verify the core-file-size limit has a sensibly large
  value in your shell before attempting to do that.

> Would upgrading to the 1.8.2.1 - latest in Ubuntu PPA would help?

Yes, this is a viable way to try solving the problem.
*But* there's a downside: the crash you're experiencing might affect
later Git versions as well as yours.  And if you just throw your hands
there, the bug will continue to be unfixed.  Hence I urge you to be a
good F/OSS user and help the Git devs investigate the case.

1. https://wiki.ubuntu.com/Apport
--
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] help: mark common_guides[] as translatable

[Simon Ruderich]

Signed-off-by: Simon Ruderich 
---
On Tue, Apr 02, 2013 at 11:39:51PM +0100, Philip Oakley wrote:
> --- a/help.c
> +++ b/help.c
> @@ -240,6 +241,23 @@ void list_common_cmds_help(void)
>   }
>  }
>
> +void list_common_guides_help(void)
> +{
> + int i, longest = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
> + if (longest < strlen(common_guides[i].name))
> + longest = strlen(common_guides[i].name);
> + }
> +
> + puts(_("The common Git guides are:\n"));
> + for (i = 0; i < ARRAY_SIZE(common_guides); i++) {
> + printf("   %s   ", common_guides[i].name);
> + mput_char(' ', longest - strlen(common_guides[i].name));
> + puts(_(common_guides[i].help));

common_guides[] is used here, but without N_() not picked up by
xgettext when creating the pot file.

Regards
Simon

 builtin/help.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 034c36c..062957f 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -419,13 +419,13 @@ static struct {
const char *name;
const char *help;
 } common_guides[] = {
-   { "attributes", "Defining attributes per path" },
-   { "glossary", "A Git glossary" },
-   { "ignore", "Specifies intentionally untracked files to ignore" },
-   { "modules", "Defining submodule properties" },
-   { "revisions", "Specifying revisions and ranges for Git" },
-   { "tutorial", "A tutorial introduction to Git (for version 1.5.1 or 
newer)" },
-   { "workflows", "An overview of recommended workflows with Git"},
+   { "attributes", N_("Defining attributes per path") },
+   { "glossary", N_("A Git glossary") },
+   { "ignore", N_("Specifies intentionally untracked files to ignore") },
+   { "modules", N_("Defining submodule properties") },
+   { "revisions", N_("Specifying revisions and ranges for Git") },
+   { "tutorial", N_("A tutorial introduction to Git (for version 1.5.1 or 
newer)") },
+   { "workflows", N_("An overview of recommended workflows with Git") },
 };
 
 static void list_common_guides_help(void)
-- 
1.8.2.481.g0d034d4

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
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: Git crash in Ubuntu 12.04

[Sivaram Kannan]

Hi,

>
> ^^^ Try to issue the
>
> $ ulimit -c unlimited

Have set the git user's crash limit to 1GB in
/etc/security/limits.conf and still getting the same error when
issuing gdb to the crash file.

>
> command in your shell before attempting the cloning -- this should
> remove the upper limit on the core file size.  And try look for the core
> file in the current directory after the crash occurs.  I'm not sure
> Ubuntu's "crash interceptor" won't kick in, but just in case...

You mean, /usr/bin/git? crash file for git is getting created each
time it crashes in /var/crash.

Can you please tell me what else I could try? Would upgrading to the
1.8.2.1 - latest in Ubuntu PPA would help?

./Siva.
--
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: "96b9e0e3 config: treat user and xdg config permission problems as errors" busted git-daemon

[W. Trevor King]

On Fri, Apr 12, 2013 at 01:05:50AM -0400, Jeff King wrote:
>   Like many programs that switch user id, the daemon does not reset
>   environment variables such a `$HOME` when it runs git programs like
>   `upload-pack` and `receive-pack`. When using this option, you may also
>   want to set and export `HOME` to point at the home directory of
>   `` before starting the daemon, and make sure the Git
>   configuration file in that directory are readable by ``.

How about "and make sure any Git configuration files", since there
might not be any Git configuration files.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy


signature.asc
Description: OpenPGP digital signature

Re: [PATCH 02/10] git-remote-mediawiki: spelling fixes

[Matthieu Moy]

Jonathan Nieder  writes:

> From: Stefano Lattarini 
> Date: Fri, 12 Apr 2013 00:36:10 +0200
>
> Most of these were found using Lucas De Marchi's codespell tool.
> Others were pointed out by Eric Sunshine.
>
> Helped-by: Eric Sunshine 
> Signed-off-by: Stefano Lattarini 
> Signed-off-by: Jonathan Nieder 

And obviously Ack-ed-by: Matthieu Moy 

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 5/5] Documentation: add caveats about I/O buffering for check-{attr,ignore}

[Adam Spiers]

On Thu, Apr 11, 2013 at 07:12:22PM -0700, Junio C Hamano wrote:
> It is usually OK to re-flow the text in the paragraph you are
> touching. After all, for the purpose of reviewing, people can just
> blindly apply and then ask "diff --color-words".  In this case,
> however, there was some changes that conflict in the vicinity, and
> reflowing made the resolution unnecessarily more cumbersome.

I see.  Thanks for the tip; I was only dimly aware of --color-words.

> I have briefly looked at this series, but it severely conflicts with
> a few topics in flight that touch the infrastructure you are using,
> so I haven't merged it to 'pu'. Perhaps after things calm down, we
> may want to ask you to reroll on top of updated codebase.

Sure, no problem.  I'll try a quick rebase now to see how ugly it is.

By the way, I've replaced my test for streaming --stdin which was
based on stdbuf(1) and sleep(1) with Peff's clever hack based on
mkfifo.  I'll hold off from sending a reroll until pathspec activity
cools down, but in the meantime it's available here:

https://github.com/aspiers/git/compare/master...git-annex-streaming

It requires my "t: make PIPE a standard test prerequisite" patch, but
I notice that's already in master which will make things easier later
on.

Thanks!
Adam
--
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] pull: fail early if we know we can't merge from upstream

[Carlos Martín Nieto]

On Thu, 2013-04-11 at 10:37 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto  writes:
> 
> > I can't quite decide whether the behaviour of 'git pull' with no
> > upstream configured but a default remote with no fetch refspecs
> > merging the remote's HEAD is a feature, a bug or something in between,
> > but it's used by t7409 so maybe someone else is using it and we
> > shouldn't break it.
> 
> Isn't it the simplest "works without any configuration" from the
> original days? 

I don't recall remotes not having refspecs when they're int he config,
though I guess it's equivalent to running 'git pull
git://example.org/myrepo.git'.

> 
> > There's another check that could be made earlier ('git pull
> > someremote' when that's not the branch's upstream remote), but then
> > you have to start figuring out what the flags to fetch are.
> 
> When the user gave us explicitly the name of the remote, it does not
> sound too bad to fetch from there.  "git pull someremote thatbranch"
> can be given after seeing a failure and succeed without retransfer,
> no?

It's not too bad, though you're paying for connection and ref
advertisement twice which breaks the otherwise quick pace of git
commands.

What I find bad from a UI point of view is that after fetching (which
could even be from the wrong remote for 'git pull' w/o upstream info)
git turns around and says "I was never going to merge/rebase that" for
things that we can know before fetching because they depend solely on
the configuration.

> 
> I am not sure if it is worth the added complexity and potential to
> introduce new bugs in general by trying to outsmart the for-merge
> logic that kicks in only after we learn what the other side offers
> and fetch from it, but anyway, let's see what we got here...
> 
> > diff --git a/git-pull.sh b/git-pull.sh
> > index 266e682..b62f5d3 100755
> > --- a/git-pull.sh
> > +++ b/git-pull.sh
> > @@ -43,6 +43,8 @@ log_arg= verbosity= progress= recurse_submodules=
> >  merge_args= edit=
> >  curr_branch=$(git symbolic-ref -q HEAD)
> >  curr_branch_short="${curr_branch#refs/heads/}"
> > +upstream=$(git config "branch.$curr_branch_short.merge")
> > +remote=$(git config "branch.$curr_branch_short.remote")
> >  rebase=$(git config --bool branch.$curr_branch_short.rebase)
> 
> Learning these upfront sounds sensible.
> 
> >  if test -z "$rebase"
> >  then
> > @@ -138,6 +140,47 @@ do
> > esac
> > shift
> >  done
> > +if test true = "$rebase"
> > +then
> > +op_type=rebase
> > +op_prep=against
> > +else
> > +op_type=merge
> > +op_prep=with
> > +fi
> > +
> > +check_args_against_config () {
> > +   # If fetch gets user-provided arguments, the user is
> > +   # overriding the upstream configuration, so we have to wait
> > +   # for fetch to do its work to know if we can merge.
> > +   if [ $# -gt 0 ]; then
> > +   return
> > +   fi
> 
> > +   # Figure out what remote we're going to be fetching from
> > +   use_remote=origin
> > +   if [ -n "$remote" ]; then
> > +   use_remote="$remote"
> > +   fi
> > +
> > +   # If the remote doesn't have a fetch refspec, then we'll merge
> > +   # whatever fetch marks for-merge, same as above.
> 
> The "above" in this sentence refers to...?
> 
> I guess "we have to wait", but it wasn't very clear.
> 

Yes, it refers to having to wait for fetch to complete before we can
know if we'll be able to merge.

> > +   fetch=$(git config --get-all "remote.$use_remote.fetch")
> > +   if [ -z "$fetch" ]; then
> > +   return
> > +   fi
> 
> Hmm, it is probably correct to punt on this case, but it defeats
> large part of the effect of your effort, doesn't it? We fetch what
> is covered by remote.$name.fetch _and_ what need to complete the
> merge operation (otherwise branch.$name.merge that is not covered by
> remote.$there.fetch will not work).  So
> 
> [remote "origin"]
> url = $over_there
> [branch "master"]
> remote = origin
> merge = refs/heads/master
> 
> would still fetch refs/heads/master from there and merge it.

If you run 'git pull' in this situation, then everything's fine and the
right thing gets merged.

> 
> > +   # The typical 'git pull' case where it should merge from the
> > +   # current branch's upstream. We can already check whether we
> > +   # we can do it. If HEAD is detached or there is no upstream
> > +   # branch, complain now.
> 
> Drop "typical", and rephrase "merge from" to also cover "rebase" (I
> often say "integrate with").

Sounds good.

> 
> To return to your original description:
> 
> A 'git pull' without specifying a remote is asked to take the
> current branch's upstream as the branch to merge from. This
> cannot work without an upstream configuration nor with HEAD
> detached, but we only check for this after fetching.
> 
> Wouldn't it be sufficient to add something like this before fetch
> happens:
> 
>   if test $# != 0 || # args explicitly specified
>test -n "$cu

Re: What's cooking in git.git (Apr 2013, #02; Fri, 5)

[Thomas Rast]

Junio C Hamano  writes:

> * tr/line-log (2013-04-05) 7 commits
>   (merged to 'next' on 2013-04-05 at 5afb00c)
>  + log -L: fix overlapping input ranges
>  + log -L: check range set invariants when we look it up
>   (merged to 'next' on 2013-04-01 at 5be920c)
>  + Speed up log -L... -M
>  + log -L: :pattern:file syntax to find by funcname
>  + Implement line-history search (git log -L)
>  + Export rewrite_parents() for 'log -L'
>  + Refactor parse_loc
>
>  Will merge down to 'master'

I did some fuzz-testing, choosing random commits and ranges from files
and running log -L on them.

While the good news is that I couldn't break ordinary log -L, there's a
rather embarassing pair of bugs: -M is completely broken (at least when
it would do any good) and the tests that claim to look at move support
actually don't pass -M.

So please hold off merging, I'll try to get this fixed this weekend.

Sorry for the trouble!

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 v2] Fix various typos and grammaros

[Stefano Lattarini]

Hi Junio.

On 04/12/2013 02:45 AM, Junio C Hamano wrote:
> Stefano Lattarini  writes:
> 
> How much of this stuff have interact with real changes that are in
> flight, with various doneness cooking in different integration
> branches?
> 
I don't know, since I only follow the master branch of Git.  And
honestly, I don't have time right now to go checking for possible
conflicts, or to resubmit ...   But I see Jonathan has taken up
the ball on this (thanks Jonathan!), so I'll leave the matter to
him.

Next time I'll try to prepare a patch broken up in more digestible
pieces, so that it will be easier for you to deal with conflicts,
and/or to selectively decide which fixes are worth applying.

Thanks, and sorry for the confusion,
  Stefano
--
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: git-http-backend: anonymous read, authenticated write

[Magnus Therning]

On Thu, Apr 11, 2013 at 9:34 PM, Jeff King  wrote:
> On Thu, Apr 11, 2013 at 08:52:56AM +0200, Magnus Therning wrote:
>
>> > The documentation should probably make the use of http.receivepack more
>> > clear in this situation.
>>
>> I think that'd be good.  The fact that it wasn't until several mails
>> into the thread that anyone thought of the http.receivepack setting
>> also suggests that its use is a bit un-intuitive (even though it
>> probably makes perfect sense and is a good solution).
>
> Yeah, I did not even think of http.receivepack because I have never had
> to set it before (it was turned on in the original tests that I built
> top of). I have the impression that the anonymous-read/authenticated-write
> setup you are using has not been all that commonly used. The example in
> the manpage dates back to 2009, but it was only in 2012 that we got a
> bug report that the client-side authentication handler has problems with
> it.

Really?  I certainly think it deserves a bit more attention than that.
 It may be that gitosis and other SSH-based solutions have been around
longer than git-http-backend, but from what I've understood from
reading, it fits very nicely in between git-daemon and the rather
heavy SSH-based stuff.

>> > But your fix under lighttpd is much better, as it asks for the
>> > credentials up front (which means the client does not go to any work
>> > creating a packfile just to find out that it does not have access).
>>
>> Yes, I think it also helps with my particular scenario where new repos
>> will be added from time to time.  This way there is no second step,
>> after `git init`, that must be remembered.
>
> Yeah, avoiding setting http.receivepack at all is helpful. Though note
> that you can also set it in /etc/gitconfig for the whole system at once.

Good point.

/M

-- 
Magnus Therning  OpenPGP: 0xAB4DFBA4
email: mag...@therning.org   jabber: mag...@therning.org
twitter: magthe   http://therning.org/magnus
--
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 06/10] compat/nedmalloc: fix spelling in comments

[Sebastian Schuberth]

On Fri, Apr 12, 2013 at 9:06 AM, Jonathan Nieder  wrote:

> These don't seem to be fixed yet in https://github.com/ned14/nedmalloc
> (pointed to from http://www.nedprod.com/programs/portable/nedmalloc/).
> Would it make sense to write a note to Ned and then import a fixed
> version from there?

Not necessarily. Last time I checked upstream nedmalloc has diverged
quite a lot from our version (which is why I only cherry-picked a
small change from upstream instead of taking all of it), so we should
make sure not to import any unwanted stuff when going the route of
importing upstream.

-- 
Sebastian Schuberth
--
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 10/10] Correct common spelling mistakes in comments and tests

[Jonathan Nieder]

From: Stefano Lattarini 
Date: Fri, 12 Apr 2013 00:36:10 +0200

Most of these were found using Lucas De Marchi's codespell tool.

Signed-off-by: Stefano Lattarini 
Signed-off-by: Jonathan Nieder 
---
Thanks for reading.

 builtin/apply.c  | 6 +++---
 commit.c | 2 +-
 commit.h | 2 +-
 diff.c   | 2 +-
 git-add--interactive.perl| 2 +-
 git-cvsserver.perl   | 4 ++--
 git-quiltimport.sh   | 2 +-
 gitweb/gitweb.perl   | 6 +++---
 perl/Git.pm  | 2 +-
 perl/Git/I18N.pm | 2 +-
 perl/private-Error.pm| 2 +-
 sequencer.c  | 2 +-
 t/t1006-cat-file.sh  | 2 +-
 t/t3701-add-interactive.sh   | 2 +-
 t/t4014-format-patch.sh  | 6 +++---
 t/t4124-apply-ws-rule.sh | 2 +-
 t/t6030-bisect-porcelain.sh  | 2 +-
 t/t7601-merge-pull-config.sh | 2 +-
 t/t7610-mergetool.sh | 2 +-
 t/t9001-send-email.sh| 4 ++--
 transport-helper.c   | 2 +-
 transport.h  | 2 +-
 xdiff/xdiffi.c   | 2 +-
 xdiff/xhistogram.c   | 2 +-
 24 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 06f5320b..f6a3c97d 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1921,7 +1921,7 @@ static int parse_binary(char *buffer, unsigned long size, 
struct patch *patch)
 }
 
 /*
- * Read the patch text in "buffer" taht extends for "size" bytes; stop
+ * Read the patch text in "buffer" that extends for "size" bytes; stop
  * reading after seeing a single patch (i.e. changes to a single file).
  * Create fragments (i.e. patch hunks) and hang them to the given patch.
  * Return the number of bytes consumed, so that the caller can call us
@@ -3025,7 +3025,7 @@ static struct patch *in_fn_table(const char *name)
  *
  * The latter is needed to deal with a case where two paths A and B
  * are swapped by first renaming A to B and then renaming B to A;
- * moving A to B should not be prevented due to presense of B as we
+ * moving A to B should not be prevented due to presence of B as we
  * will remove it in a later patch.
  */
 #define PATH_TO_BE_DELETED ((struct patch *) -2)
@@ -3509,7 +3509,7 @@ static int check_patch(struct patch *patch)
 *
 * A patch to swap-rename between A and B would first rename A
 * to B and then rename B to A.  While applying the first one,
-* the presense of B should not stop A from getting renamed to
+* the presence of B should not stop A from getting renamed to
 * B; ask to_be_deleted() about the later rename.  Removal of
 * B and rename from A to B is handled the same way by asking
 * was_deleted().
diff --git a/commit.c b/commit.c
index e8eb0aec..1a41757e 100644
--- a/commit.c
+++ b/commit.c
@@ -834,7 +834,7 @@ struct commit_list *get_merge_bases(struct commit *one, 
struct commit *two,
 }
 
 /*
- * Is "commit" a decendant of one of the elements on the "with_commit" list?
+ * Is "commit" a descendant of one of the elements on the "with_commit" list?
  */
 int is_descendant_of(struct commit *commit, struct commit_list *with_commit)
 {
diff --git a/commit.h b/commit.h
index 4138bb4c..252c7f87 100644
--- a/commit.h
+++ b/commit.h
@@ -164,7 +164,7 @@ extern struct commit_list *get_merge_bases(struct commit 
*rev1, struct commit *r
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, 
struct commit **twos, int cleanup);
 extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
 
-/* largest postive number a signed 32-bit integer can contain */
+/* largest positive number a signed 32-bit integer can contain */
 #define INFINITE_DEPTH 0x7fff
 
 extern int register_shallow(const unsigned char *sha1);
diff --git a/diff.c b/diff.c
index db952a5b..0eb26535 100644
--- a/diff.c
+++ b/diff.c
@@ -1565,7 +1565,7 @@ static void show_stats(struct diffstat_t *data, struct 
diff_options *options)
 * Binary files are displayed with "Bin XXX -> YYY bytes"
 * instead of the change count and graph. This part is treated
 * similarly to the graph part, except that it is not
-* "scaled". If total width is too small to accomodate the
+* "scaled". If total width is too small to accommodate the
 * guaranteed minimum width of the filename part and the
 * separators and this message, this message will "overflow"
 * making the line longer than the maximum width.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 710764ab..d2c4ce6e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1247,7 +1247,7 @@ sub summarize_hunk {
 
 
 # Print a one-line summary of each hunk in the array ref in
-# the first argument, starting wih the index in the 2nd.
+# the first argument, starting with the index in the 2nd.
 sub display_hunks {
my ($hunks, $i) = @_;
my $ctr = 0;
diff --git a/git-cv

[PATCH 09/10] git-gui: fix spelling in comments

[Jonathan Nieder]

From: Stefano Lattarini 
Date: Fri, 12 Apr 2013 00:36:10 +0200

Correct common spelling mistakes noticed by Lucas De Marchi's
codespell tool.

Signed-off-by: Stefano Lattarini 
Signed-off-by: Jonathan Nieder 
---
Split from a larger patch.  Could be applied with "patch -p2"
if it seems like a good change for git-gui.

 git-gui/lib/blame.tcl  | 2 +-
 git-gui/lib/index.tcl  | 2 +-
 git-gui/lib/spellcheck.tcl | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-gui/lib/blame.tcl b/git-gui/lib/blame.tcl
index 324f7744..b1d15f46 100644
--- a/git-gui/lib/blame.tcl
+++ b/git-gui/lib/blame.tcl
@@ -5,7 +5,7 @@ class blame {
 
 image create photo ::blame::img_back_arrow -data 
{R0lGODlhGAAYAIUAAPwCBEzKXFTSZIz+nGzmhGzqfGTidIT+nEzGXHTqhGzmfGzifFzadETCVES+VARWDFzWbHzyjAReDGTadFTOZDSyRDyyTCymPARaFGTedFzSbDy2TCyqRCyqPARaDAyCHES6VDy6VCyiPAR6HCSeNByWLARyFARiDARqFGTifARiFCH5BAEALAAYABgAAAajQIBwSCwaj8ikcsk0BppJwRPqHEypQwHBis0WDAdEFyBIKBaMAKLBdjQeSkFBYTBAIvgEoS6JmhUTEwIUDQ4VFhcMGEhyCgoZExoUaxsWHB0THkgfAXUGAhoBDSAVFR0XBnCbDRmgog0hpSIiDJpJIyEQhBUcJCIlwA22SSYVogknEg8eD82qSigdDSknY0IqJQXPYxIl1dZCGNvWw+Dm510GQQAh/mhDcmVhdGVkIGJ5IEJNUFRvR0lGIFBybyB2ZXJzaW9uIDIuNQ0KqSBEZXZlbENvciAxOTk3LDE5OTguIEFsbCByaWdodHMgcmVzZXJ2ZWQuDQpodHRwOi8vd3d3LmRldmVsY29yLmNvbQA7}
 
-# Persistant data (survives loads)
+# Persistent data (survives loads)
 #
 field history {}; # viewer history: {commit path}
 field header; # array commit,key -> header field
diff --git a/git-gui/lib/index.tcl b/git-gui/lib/index.tcl
index 8efbbdde..74a81a7b 100644
--- a/git-gui/lib/index.tcl
+++ b/git-gui/lib/index.tcl
@@ -414,7 +414,7 @@ proc revert_helper {txt paths} {
# such distinction is needed in some languages. Previously, the
# code used "Revert changes in" for both, but that can't work
# in languages where 'in' must be combined with word from
-   # rest of string (in diffrent way for both cases of course).
+   # rest of string (in different way for both cases of course).
#
# FIXME: Unfortunately, even that isn't enough in some languages
# as they have quite complex plural-form rules. Unfortunately,
diff --git a/git-gui/lib/spellcheck.tcl b/git-gui/lib/spellcheck.tcl
index e6120303..538d61c7 100644
--- a/git-gui/lib/spellcheck.tcl
+++ b/git-gui/lib/spellcheck.tcl
@@ -14,7 +14,7 @@ field w_menu  ; # context menu for the widget
 field s_menuidx 0 ; # last index of insertion into $w_menu
 
 field s_i   {} ; # timer registration for _run callbacks
-field s_clear0 ; # did we erase mispelled tags yet?
+field s_clear0 ; # did we erase misspelled tags yet?
 field s_seen[list] ; # lines last seen from $w_text in _run
 field s_checked [list] ; # lines already checked
 field s_pending [list] ; # [$line $data] sent to ispell/aspell
@@ -259,7 +259,7 @@ method _run {} {
if {$n == $cur_line
 && ![regexp {^\W$} [$w_text get $cur_pos insert]]} {
 
-   # If the current word is mispelled remove the tag
+   # If the current word is misspelled remove the tag
# but force a spellcheck later.
#
set tags [$w_text tag names $cur_pos]
-- 
1.8.2.1

--
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 08/10] kwset: fix spelling in comments

[Jonathan Nieder]

From: Stefano Lattarini 
Date: Fri, 12 Apr 2013 00:36:10 +0200

Correct spelling mistakes noticed using Lucas De Marchi's codespell
tool.

Signed-off-by: Stefano Lattarini 
Signed-off-by: Jonathan Nieder 
---
Git might be the canonical upstream for the GPL-2+ version of this
code.

 kwset.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kwset.c b/kwset.c
index 51b2ab6c..5800999b 100644
--- a/kwset.c
+++ b/kwset.c
@@ -26,7 +26,7 @@
The author may be reached (Email) at the address m...@ai.mit.edu,
or (US mail) as Mike Haertel c/o Free Software Foundation. */
 
-/* The algorithm implemented by these routines bears a startling resemblence
+/* The algorithm implemented by these routines bears a startling resemblance
to one discovered by Beate Commentz-Walter, although it is not identical.
See "A String Matching Algorithm Fast on the Average," Technical Report,
IBM-Germany, Scientific Center Heidelberg, Tiergartenstrasse 15, D-6900
@@ -435,7 +435,7 @@ kwsprep (kwset_t kws)
  /* Update the delta table for the descendents of this node. */
  treedelta(curr->links, curr->depth, delta);
 
- /* Compute the failure function for the decendents of this node. */
+ /* Compute the failure function for the descendants of this node. */
  treefails(curr->links, curr->fail, kwset->trie);
 
  /* Update the shifts at each node in the current node's chain
-- 
1.8.2.1

--
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 06/10] compat/nedmalloc: fix spelling in comments

[Jonathan Nieder]

From: Stefano Lattarini 
Date: Fri, 12 Apr 2013 00:36:10 +0200

Correct some typos found using Lucas De Marchi's codespell tool.

Signed-off-by: Stefano Lattarini 
Signed-off-by: Jonathan Nieder 
---
These don't seem to be fixed yet in https://github.com/ned14/nedmalloc
(pointed to from http://www.nedprod.com/programs/portable/nedmalloc/).
Would it make sense to write a note to Ned and then import a fixed
version from there?

 compat/nedmalloc/malloc.c.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h
index ff7c2c4f..1401a672 100644
--- a/compat/nedmalloc/malloc.c.h
+++ b/compat/nedmalloc/malloc.c.h
@@ -4778,7 +4778,7 @@ void* dlmalloc(size_t bytes) {
 
 void dlfree(void* mem) {
   /*
- Consolidate freed chunks with preceeding or succeeding bordering
+ Consolidate freed chunks with preceding or succeeding bordering
  free chunks, if they exist, and then place in a bin.  Intermixed
  with special cases for top, dv, mmapped chunks, and usage errors.
   */
@@ -5680,10 +5680,10 @@ History:
Wolfram Gloger (glo...@lrz.uni-muenchen.de).
   * Use last_remainder in more cases.
   * Pack bins using idea from  co...@nyx10.cs.du.edu
-  * Use ordered bins instead of best-fit threshhold
+  * Use ordered bins instead of best-fit threshold
   * Eliminate block-local decls to simplify tracing and debugging.
   * Support another case of realloc via move into top
-  * Fix error occuring when initial sbrk_base not word-aligned.
+  * Fix error occurring when initial sbrk_base not word-aligned.
   * Rely on page size for units instead of SBRK_UNIT to
avoid surprises about sbrk alignment conventions.
   * Add mallinfo, mallopt. Thanks to Raymond Nijssen
-- 
1.8.2.1

--
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 07/10] precompose-utf8: fix spelling of "want" in error message

[Jonathan Nieder]

From: Stefano Lattarini 
Date: Fri, 12 Apr 2013 00:36:10 +0200

Noticed using Lucas De Marchi's codespell tool.

Signed-off-by: Stefano Lattarini 
Signed-off-by: Jonathan Nieder 
---
 compat/precompose_utf8.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 8cf59558..030174db 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -134,7 +134,7 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR 
*prec_dir)
if (prec_dir->ic_precompose == (iconv_t)-1) {
die("iconv_open(%s,%s) failed, but needed:\n"
"precomposed unicode is not 
supported.\n"
-   "If you wnat to use 
decomposed unicode, run\n"
+   "If you want to use 
decomposed unicode, run\n"
"\"git config 
core.precomposeunicode false\"\n",
repo_encoding, path_encoding);
} else {
-- 
1.8.2.1

--
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 05/10] compat/regex: fix spelling and grammar in comments

[Jonathan Nieder]

From: Stefano Lattarini 
Date: Fri, 12 Apr 2013 00:36:10 +0200

Some of these were found using Lucas De Marchi's codespell tool.
Others noticed by Eric Sunshine.

Helped-by: Eric Sunshine 
Signed-off-by: Stefano Lattarini 
Signed-off-by: Jonathan Nieder 
---
Is gawk the appropriate upstream to send this sort of readability
improvement to?

 compat/regex/regcomp.c| 4 ++--
 compat/regex/regex.c  | 2 +-
 compat/regex/regex_internal.c | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 8c96ed94..d0025bd5 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -2095,7 +2095,7 @@ peek_token_bracket (re_token_t *token, re_string_t 
*input, reg_syntax_t syntax)
 
 /* Entry point of the parser.
Parse the regular expression REGEXP and return the structure tree.
-   If an error is occured, ERR is set by error code, and return NULL.
+   If an error has occurred, ERR is set by error code, and return NULL.
This function build the following tree, from regular expression :
   CAT
   / \
@@ -3715,7 +3715,7 @@ build_charclass_op (re_dfa_t *dfa, RE_TRANSLATE_TYPE 
trans,
 /* This is intended for the expressions like "a{1,3}".
Fetch a number from `input', and return the number.
Return -1, if the number field is empty like "{,1}".
-   Return -2, If an error is occured.  */
+   Return -2, if an error has occurred.  */
 
 static int
 fetch_number (re_string_t *input, re_token_t *token, reg_syntax_t syntax)
diff --git a/compat/regex/regex.c b/compat/regex/regex.c
index 3dd8dfa0..6aaae003 100644
--- a/compat/regex/regex.c
+++ b/compat/regex/regex.c
@@ -22,7 +22,7 @@
 #include "config.h"
 #endif
 
-/* Make sure noone compiles this code with a C++ compiler.  */
+/* Make sure no one compiles this code with a C++ compiler.  */
 #ifdef __cplusplus
 # error "This is C code, use a C compiler"
 #endif
diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
index 193854cf..d4121f2f 100644
--- a/compat/regex/regex_internal.c
+++ b/compat/regex/regex_internal.c
@@ -1284,7 +1284,7 @@ re_node_set_merge (re_node_set *dest, const re_node_set 
*src)
 
 /* Insert the new element ELEM to the re_node_set* SET.
SET should not already have ELEM.
-   return -1 if an error is occured, return 1 otherwise.  */
+   return -1 if an error has occurred, return 1 otherwise.  */
 
 static int
 internal_function
@@ -1341,7 +1341,7 @@ re_node_set_insert (re_node_set *set, int elem)
 
 /* Insert the new element ELEM to the re_node_set* SET.
SET should not already have any element greater than or equal to ELEM.
-   Return -1 if an error is occured, return 1 otherwise.  */
+   Return -1 if an error has occurred, return 1 otherwise.  */
 
 static int
 internal_function
@@ -1416,7 +1416,7 @@ re_node_set_remove_at (re_node_set *set, int idx)
 
 
 /* Add the token TOKEN to dfa->nodes, and return the index of the token.
-   Or return -1, if an error will be occured.  */
+   Or return -1, if an error has occurred.  */
 
 static int
 internal_function
-- 
1.8.2.1

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