'make test' fails in pu

2015-02-17 Thread Dennis Kaarsemaker
Make test has been failing for 'pu' yesterday for and today at
t4016-diff-quote.sh. Full log:
http://ci.kaarsemaker.net/git/refs/heads/pu/1df29c71a731c679de9055ae5e407f3a4e18740a/artefact/test/log

I noticed this a few times before and it tends to get fixed again
relatively quickly. So I'm wondering:

- Should I even mention that it's failing, or is that just useless
  noise?
- If I should report this, I could also make my testing thing send 
  mails. Would that be useful?

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

--
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: odb_mkstemp's 0444 permission broke write/delete access on AFP

2015-02-17 Thread Matthieu Moy
Fairuzan Roslan  writes:

> $ git clone https://github.com/robbyrussell/oh-my-zsh.git
> Cloning into 'oh-my-zsh'...
> remote: Counting objects: 11830, done.
> remote: Total 11830 (delta 0), reused 0 (delta 0)
> Receiving objects: 100% (11830/11830), 2.12 MiB | 481.00 KiB/s, done.
> Resolving deltas: 100% (6510/6510), done.
> warning: unable to unlink 
> /Volumes/installer/oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not 
> permitted

This should be fixable from Git itself, by replacing the calls to
"unlink" with something like

int unlink_or_chmod(...) {
if (unlink(...)) {
chmod(...); // give user write permission
return unlink(...);
}
}

This does not add extra cost in the normal case, and would fix this
particular issue for afp shares. So, I think that would fix the biggest
problem for afp-share users without disturbing others. It seems
reasonable to me to do that unconditionnally.

> $ rm -rf oh-my-zsh/.git/objects/pack/tmp_*
> rm: oh-my-zsh/.git/objects/pack/tmp_idx_oUN1sb: Operation not permitted
> rm: oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not permitted

What happens if you do "rm -fr oh-my-zsh/.git/objects/pack/" (i.e.
remove the directory, not the files)?

If you can still remove the directory, then I'd say the solution above
could be sufficient: the user isn't supposed to interfer with the
content of .git/objects other than by using Git, and if he or she does,
then asking a chmod prior to an rm seems reasonable.

If you can't, then it's another problematic use-case (basically, you
can't just "rm -fr" a whole clone), and then it deserves at least an
opt-in configuration to get writable pack files.

(Unfortunately, I suspect we're in the later case)

> If you insist on setting the tmp idx & pack file permission to 0444 at
> least give it a u+w permission whenever you try to unlink and rename
> it so it won’t fail.

Yes. In case you hadn't guessed, this is precisely what I had in mind
when I asked "Is it a problem when using Git [...] or when trying to
remove files outside Git?".

-- 
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: 'make test' fails in pu

2015-02-17 Thread Jeff King
On Tue, Feb 17, 2015 at 09:39:17AM +0100, Dennis Kaarsemaker wrote:

> Make test has been failing for 'pu' yesterday for and today at
> t4016-diff-quote.sh. Full log:
> http://ci.kaarsemaker.net/git/refs/heads/pu/1df29c71a731c679de9055ae5e407f3a4e18740a/artefact/test/log
> 
> I noticed this a few times before and it tends to get fixed again
> relatively quickly. So I'm wondering:
> 
> - Should I even mention that it's failing, or is that just useless
>   noise?
> - If I should report this, I could also make my testing thing send 
>   mails. Would that be useful?

If you bisect this, it turns up commit 30cd8f94f, which says:

WIP: diff-b-m

[...]

This update is still broken and breaks a handful of tests:

 4016 4023 4047 4130 6022 6031 6032 9300 9200 9300 9350

Sometimes a breakage in pu is surprising (e.g., it breaks only on a
platform that the maintainer does not run "make test" on) and we would
want to know about it. But sometimes it is merely that there is a
work-in-progress. And it probably requires a human to tell the
difference.

So no, I do not think automatically mailing on test failures in pu is a
good idea. Manually peeking at them and sending fixes before the series
is merged to next _is_ very much encouraged, though. :)

Unlike "pu", "next" and "master" should never fail tests (I think that
Junio will not push them out if the tests have failed on his system). So
failures there are much more likely to be interesting platform bugs (but
of course, testing "pu" is still encouraged, as we may catch problems).

But even for "next", I would say blind automated emails are not nearly
as useful as a human who has looked at the problem (and especially
bisected).

-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


Lift --stdout restriction for using reachability bitmap in pack-objects?

2015-02-17 Thread Duy Nguyen
Commit 6b8fda2 (pack-objects: use bitmaps when packing objects -
2013-12-21) turns off reachability bitmap optimization if --stdout is
not specified. I wonder if we could lift this restriction?

My use case is debugging pack-objects in gdb (repeatedly) where this
optimization could really save time. But specifying --stdout would
clutter gdb interface.. I don't know this code well enough to judge,
but I think the worst thing is we don't write .idx file out (maybe
because it would involve a lot more work).
-- 
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 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Paolo Bonzini


On 16/02/2015 20:47, Junio C Hamano wrote:
> Paolo Bonzini  writes:
> 
>> From: Paolo Bonzini 
>>
>> After updating to git 2.3.0, "git request-pull" is stubbornly complaining
>> that I lack a matching tag on the remote side unless I pass the third
>> argument.  But I did prepare and push a signed tag.
> 
> A few questions.
> 
>  - what old version did you update from?  I think the "correct
>over-eager dwimming" change was from v2.0 days.

I upgraded from 1.9.  My workflow is to make a signed tag, push it and
do "git request-pull origin/master ".

My branches have a different name locally vs. remotely (e.g.
"kvm-master" and "kvm-next" locally vs. refs/heads/master and
refs/heads/next remotely) exactly to avoid overeager matching in
git-request-pull.  I only ever want to request pulls based on signed tags.

>  - what exactly do you mean by "stubbornly complain"?  I think we
>say something about HEAD not matching the HEAD over there, which
>I think is bogus (we should instead say things about the branch
>you are on and the branch over there with the same name) and is
>worth fixing.

I tried both "git checkout kvm-next" and "git checkout tags/for-linus",
and it still complains.

What you refer to is, I think, fixed by patch 3.  The find_matching_ref
script does not work if its first argument is HEAD.  So patch 3 is
probably an improvement anyway for the "matching branch name" case, even
if my usecase involves tags rather than branches.

> An earlier 024d34cb (request-pull: more strictly match local/remote
> branches, 2014-01-22) deliberately disabled over-eager DWIMming when
> the $3-rd argument _is_ given.

I agree with that change.

> One part of me feel that not giving the $3-rd argument should behave
> the same way as if you gave the name of the current branch as the
> $3-rd argument.

This works well for workflows where you do pull requests based on
branches.  However Linus strongly encourages using signed tags.

I certainly can adjust my workflow for this.  For example I can add
something like this to my .gitconfig

[request-pull]
dwim = tags/for-linus

and add an alias that uses "git config request-pull.dwim" as the third
argument (other projects I work on obviously use different tag names :).

While similar, the two patches are different:

1) The usage of "git show-ref --heads --tags" looked like a feeble
attempt at DWIMming tags.  But I can see how that is supposed to work
only if $3 is specified.  Adding a usage of "git tag --points-at" would
go against the intentions of 024d34cb.  Perhaps restrict DWIMming to
signed and annotated tags only, through a new option to "git tag"?

2) Patch 3 makes sense independent of patch 2 and, as mentioned above,
it is probably a bugfix anyway.

> On the other hand, I can also understand (not necessarily agree
> with) a view that not giving the $3-rd argument is an explicit
> user's wish to us to DWIM as much as we want.  But again, that
> directly contradicts with the desire of 024d34cb.
> 
> So,... I dunno.

I don't know either.  Based on your answer, it seems like you are
focusing mostly on a branch-based workflow; the two definitely have
different requirements for DWIMming (since you cannot get a tag name via
"git symbolic-ref" for example).  On the other hand most of the
un-DWIMming changes were done by Linus who works a lot with (other
people's) signed tags...

Paolo
--
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: Lift --stdout restriction for using reachability bitmap in pack-objects?

2015-02-17 Thread Jeff King
On Tue, Feb 17, 2015 at 05:07:36PM +0700, Duy Nguyen wrote:

> Commit 6b8fda2 (pack-objects: use bitmaps when packing objects -
> 2013-12-21) turns off reachability bitmap optimization if --stdout is
> not specified. I wonder if we could lift this restriction?

I'm not sure what else would break if we did. For instance, would
bitmaps kick in when doing the "Counting objects" phase of "git repack
-ad". And if so, what would the resulting pack look like?

So I'm not opposed to it in principle, but I suspect there's some
cleanup work to be done. But...

> My use case is debugging pack-objects in gdb (repeatedly) where this
> optimization could really save time. But specifying --stdout would
> clutter gdb interface.. I don't know this code well enough to judge,
> but I think the worst thing is we don't write .idx file out (maybe
> because it would involve a lot more work).

If the only reason is for gdb, then perhaps:

  set args pack-objects --stdout /dev/null

in gdb would help?

-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: Lift --stdout restriction for using reachability bitmap in pack-objects?

2015-02-17 Thread Duy Nguyen
On Tue, Feb 17, 2015 at 5:13 PM, Jeff King  wrote:
> If the only reason is for gdb, then perhaps:
>
>   set args pack-objects --stdout /dev/null
>
> in gdb would help?

Right. I used "gdb --args command >/dev/null" instead. Stupid
question. Sorry for the noise.
-- 
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: Lift --stdout restriction for using reachability bitmap in pack-objects?

2015-02-17 Thread Jeff King
On Tue, Feb 17, 2015 at 05:36:30PM +0700, Duy Nguyen wrote:

> On Tue, Feb 17, 2015 at 5:13 PM, Jeff King  wrote:
> > If the only reason is for gdb, then perhaps:
> >
> >   set args pack-objects --stdout /dev/null
> >
> > in gdb would help?
> 
> Right. I used "gdb --args command >/dev/null" instead. Stupid
> question. Sorry for the noise.

I've made the same mistake myself many times. I really wish gdb would
interact over /dev/tty by default. The perl debugger does this, and I
find it quite handy. But I've never managed to make gdb do it. Maybe
there is an option I've missed[1].

The downside of what approach, though, is that you cannot restart the
program reliably from within the debugger (it cannot know how to
re-set-up the descriptors). So you have to quit and restart, which loses
any breakpoints, etc.

-Peff

[1] Having written that, I'm not sure I ever tried a script that does
something like:

  program=$1; shift
  exec 3<&0
  exec 4>&1
  gdb /dev/tty -ex "set args <&3 >&4 gdb $*" "$program"

I think that would work, but it does screw up quoting/whitespace in
your arguments.
--
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] builtin/push.c: make push_default a static variable

2015-02-17 Thread Jeff King
On Mon, Feb 16, 2015 at 12:47:54AM -0500, Jeff King wrote:

> When the "push_default" flag was originally added, it was
> made globally visible to all code. This might have been
> useful if other commands or library calls ended up depending
> on it, but as it turns out, only builtin/push.c cares.
> 
> Let's make it a static variable in builtin/push.c.
>
> [...]
> 
> ---
> We know this is safe because no other callers needed tweaked when the
> variable went out of scope. :) It would only be a bad idea if we
> were planning on having other code in the future depend on push_default
> (e.g., the code in remote.c to find the push destination). But it does
> not seem to have needed that in the intervening years, so it's probably
> fine to do this cleanup now.

I had a nagging feeling that there was some code which wanted to use
this elsewhere, and I did finally find it, when I merged this topic with
my other personal topics.

If we wanted to implement "@{push}" (or "@{publish}") to mean "the
tracking ref of the remote ref you would push to if you ran git-push",
then this is a step in the wrong direction.

The patches I posted last January (and which you carried as
jk/branch-at-publish for a while) do work, and I've used the feature
once or twice since then. From the discussion, it looks like they were
meant to be a building block for more triangular-flow work, but I don't
remember what else was needed. I'm tempted to resurrect them, but it's
not a high priority for me.

Anyway, food for thought on whether we want to do this cleanup or not,
then. We can always leave this here as part of git_default_config, and
still move Dave's new option into git_push_config.

-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 v2 03/12] struct ref_update: move "have_old" into "flags"

2015-02-17 Thread Michael Haggerty
On 02/12/2015 08:15 PM, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
>> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty  
>> wrote:
>>> -   int flags; /* REF_NODEREF? */
>>> -   int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
>>> +   /*
>>> +* One or more of REF_HAVE_OLD, REF_NODEREF,
>>> +* REF_DELETING, and REF_IS_PRUNING:
>>> +*/
>>> +   int flags;
>>
>> Nit:
>> I'd find it more readable if it would be:
>> /*
>>  * One or more of
>>  * REF_HAVE_OLD,
>>  * REF_NODEREF,
>>  * REF_DELETING,
>>  * REF_IS_PRUNING:
>>  * whose definition is found at the top of this file.
>>  */
> 
> I wouldn't do either, though, as you would have to keep repeating
> yourself here and over there.  Wouldn't it be sufficient to:
> 
>  - Have a header that says "these are bits meant to go to struct
>ref_update.flags" at the beginning of series of #define's;
> 
>  - Say "ref_update.flags bits are defined above" here.  The phrasing
>can be "One or more of REF_HAVE_OLD, etc. defined above." as long
>as it is clear that this is not meant to be an exhausitive list.

This would be an easy change but for the fact that REF_NODEREF is
defined in refs.h whereas the other constants are internal to refs.c. If
it's OK with you guys, I'd rather leave it the way it is for now and
come back to it later.

For example I want to rename the constants to REF_NODEREF ->
REF_NO_DEREF and REF_ISPRUNING -> REF_IS_PRUNING [1], but am leaving
that for when the refs code is not in so much flux. I can reorganize the
constants and docs then.

Michael

[1] As I type this I realize that the comment misspells the name of
REF_ISPRUNING. I'll fix that, too.

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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 06/12] commit: add tests of commit races

2015-02-17 Thread Michael Haggerty
On 02/12/2015 07:13 PM, Stefan Beller wrote:
> On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty  
> wrote:
>> Committing involves the following steps:
>> [...]
>> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
>> new file mode 100755
>> index 000..08e6a6c
>> --- /dev/null
>> +++ b/t/t7516-commit-races.sh
>> @@ -0,0 +1,33 @@
>> +#!/bin/sh
>> +
>> +test_description='git commit races'
>> +. ./test-lib.sh
>> +
>> +test_tick
> 
> So I am wondering why we need to have a test_tick here.
> In case we need to pass simulation time after loading the
> test-lib, we should rather have it inside the test-lib.

You're right; I don't think it's necessary. I will remove it.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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 06/12] commit: add tests of commit races

2015-02-17 Thread Michael Haggerty
On 02/12/2015 08:36 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
>> new file mode 100755
>> index 000..08e6a6c
>> --- /dev/null
>> +++ b/t/t7516-commit-races.sh
>> @@ -0,0 +1,33 @@
>> +#!/bin/sh
>> +
>> +test_description='git commit races'
>> +. ./test-lib.sh
>> +
>> +test_tick
>> +
>> +test_expect_success 'set up editor' '
>> +write_script editor <<-\EOF
>> +git commit --allow-empty -m hare
>> +echo tortoise >"$1"
>> +EOF
>> +'
>> +
>> +test_expect_failure 'race to create orphan commit' '
>> +test_must_fail env EDITOR=./editor git commit --allow-empty &&
>> +git show -s --pretty=format:%s >subject &&
>> +grep -q hare subject &&
> 
> Why "grep -q" in the test?  Normal invocation of the tests will hide
> the output anyway, no?
> 
> Wouldn't letting "sh t-name-of-test.sh -v" show the output
> better for those who are hunting for breakages to see at which step
> of the &&-chain things break?

Good point. I will remove the "-q" from the two grep invocations in this
file.

>> +test -z "$(git show -s --pretty=format:%P)"
>> +'
> 
>> +test_expect_success 'race to create non-orphan commit' '
>> +git checkout --orphan branch &&
>> +git commit --allow-empty -m base &&
>> +git rev-parse HEAD >base &&
>> +test_must_fail env EDITOR=./editor git commit --allow-empty &&
>> +git show -s --pretty=format:%s >subject &&
>> +grep -q hare subject &&
> 
> Can we use a token different from hare and tortoise here?  If the
> previous one worked correctly, the main "commit" process would have
> failed to add 'tortoise' on top of 'hare' that raced from sideways
> (which is simulated by making 'hare' from the editor), so the tip of
> the history would be 'hare' when this test starts.  Expecting 'hare'
> here makes it unclear if you are expecting _both_ of the competing
> processes to fail (i.e. the main 'commit' fails to add 'tortoise'
> and the racing 'commit' fails to do 'hare'), leaving the 'hare' the
> previous test left at the tip of the history, or if you are expecting
> that the competing one that tries to create the second 'hare' on top
> of the existing 'hare' to win.

Yes, you're right. I will change the second test to use different tokens.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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 03/12] struct ref_update: move "have_old" into "flags"

2015-02-17 Thread Junio C Hamano
Michael Haggerty  writes:

> For example I want to rename the constants to REF_NODEREF ->
> REF_NO_DEREF and REF_ISPRUNING -> REF_IS_PRUNING [1], but am leaving
> that for when the refs code is not in so much flux. I can reorganize the
> constants and docs then.

These renames are very sensible.  Thanks for your attention to the
details.



>
> Michael
>
> [1] As I type this I realize that the comment misspells the name of
> REF_ISPRUNING. I'll fix that, too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2015-02-17 Thread Michael Haggerty
On 02/13/2015 12:08 AM, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
>> Jeff King  writes:
>>
>>> On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:
>>>
 I am more worried about variable length part pushing the information
 that is given later out to the right, e.g. "error: missing file '%s'
 prevents us from doing X".  Chomping to [1024] is not a good
 strategy for that kind of message; abbreviating %s to /path/name/...
 (again, with literally "...") would be.
> 
> I have this one in my pile of Undecided topics:
> 
> * jn/doc-api-errors (2014-12-04) 1 commit
>  - doc: document error handling functions and conventions
> 
>  For discussion.
>  What's the status of this one
> 
> I think we all agree that the early part of the new documentation
> text is good, but the last section that proposes to store more
> detailed errors in caller supplied strbuf in textual form was
> controversial (and I have not convinced myself it is a good idea
> yet).
> 
> I could chuck the last section and then start merging the remainder
> to 'next' to salvage the "obviously good bits".  Or do people want
> to hash its last section a bit more?

Whether or not we decide on a different error-handling convention in the
future, it is a fact of life that a good bit of code already uses the
"strbuf" convention documented by Jonathan's patch. So I think it is OK
to merge it as is. If we change the preferred convention in the future,
one part of the change will be to update this file.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2015-02-17 Thread Jeff King
On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote:

> > Whether or not we decide on a different error-handling convention in the
> > future, it is a fact of life that a good bit of code already uses the
> > "strbuf" convention documented by Jonathan's patch. So I think it is OK
> > to merge it as is. If we change the preferred convention in the future,
> > one part of the change will be to update this file.
> 
> I wasn't sure about "a good bit of code already", but that settles
> it.  Let's take it as-is and see how the code evolves.

I'm not convinced myself after a quick grep. But it's certainly
non-zero, and I think I would rather have the technique documented than
not, so I withdraw my earlier complaints.

-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/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2015-02-17 Thread Junio C Hamano
Michael Haggerty  writes:

>> I think we all agree that the early part of the new documentation
>> text is good, but the last section that proposes to store more
>> detailed errors in caller supplied strbuf in textual form was
>> controversial (and I have not convinced myself it is a good idea
>> yet).
>> 
>> I could chuck the last section and then start merging the remainder
>> to 'next' to salvage the "obviously good bits".  Or do people want
>> to hash its last section a bit more?
>
> Whether or not we decide on a different error-handling convention in the
> future, it is a fact of life that a good bit of code already uses the
> "strbuf" convention documented by Jonathan's patch. So I think it is OK
> to merge it as is. If we change the preferred convention in the future,
> one part of the change will be to update this file.

I wasn't sure about "a good bit of code already", but that settles
it.  Let's take it as-is and see how the code evolves.

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: Git gc removes all packs

2015-02-17 Thread Michael Haggerty
On 02/05/2015 09:03 PM, Jeff King wrote:
> On Thu, Feb 05, 2015 at 04:13:03PM +0100, Dmitry Neverov wrote:
>> [...]
>> One more thing about my setup: since git p4 promotes a use of a linear
>> history I use a separate repository for another branch in perforce. In
>> order to be able to cherry-pick between repositories I added this
>> another repo objects dir as an alternate and also added a ref which is a
>> symbolic link to a branch in another repo (so I don't have to do any
>> fetches).
> 
> You can't symlink refs like this. The loose refs in the filesystem may
> be migrated into the "packed-refs" file, at which point your symlink
> will be broken. That is a likely reason why git would not find any refs.
> 
> So your setup will not ever work reliably.  But IMHO, it is a bug that
> git does not notice the broken symlink and abort an operation which is
> computing reachability in order to drop objects. As you noticed, it
> means a misconfiguration or filesystem error results in data loss.

There's a bunch of code in refs.c that is there explicitly for reading
loose references that are symlinks. If the link contents literally start
with "refs/", then they are read and treated as a symbolic ref.
Otherwise, the symlink is just followed.

It is still possible to write symbolic refs that are represented as
symlinks (see core.preferSymlinkRefs), but that backwards-compatibility
code was added in 2006(!) Maybe it's time to deprecate it. And maybe we
should start working towards a future where any symlinks under "refs"
cause git to complain.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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 gc removes all packs

2015-02-17 Thread Jeff King
On Tue, Feb 17, 2015 at 05:39:27PM +0100, Michael Haggerty wrote:

> > You can't symlink refs like this. The loose refs in the filesystem may
> > be migrated into the "packed-refs" file, at which point your symlink
> > will be broken. That is a likely reason why git would not find any refs.
> > 
> > So your setup will not ever work reliably.  But IMHO, it is a bug that
> > git does not notice the broken symlink and abort an operation which is
> > computing reachability in order to drop objects. As you noticed, it
> > means a misconfiguration or filesystem error results in data loss.
> 
> There's a bunch of code in refs.c that is there explicitly for reading
> loose references that are symlinks. If the link contents literally start
> with "refs/", then they are read and treated as a symbolic ref.
> Otherwise, the symlink is just followed.

Right, but we should be able to notice that:

  1. We found a symlink.

  2. We couldn't read it its ref value (because it's a broken link).

I think we _do_ notice that at the lowest level, and set REF_ISBROKEN.
But the problem is that the reachability code in prune and in
pack-objects (triggered by "repack -ad") uses for_each_ref, and not
for_each_rawref. So they ignore "broken" refs rather than complaining,
even though failing to read a ref may mean we could drop objects which
were only mentioned by that ref.

> It is still possible to write symbolic refs that are represented as
> symlinks (see core.preferSymlinkRefs), but that backwards-compatibility
> code was added in 2006(!) Maybe it's time to deprecate it. And maybe we
> should start working towards a future where any symlinks under "refs"
> cause git to complain.

I wouldn't mind seeing all of the symlink code go away, but I think it
is orthogonal to the problem I mentioned.

-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: odb_mkstemp's 0444 permission broke write/delete access on AFP

2015-02-17 Thread Fairuzan Roslan

> On Feb 17, 2015, at 4:51 PM, Matthieu Moy  
> wrote:
> 
> Fairuzan Roslan  writes:
> 
>> $ git clone https://github.com/robbyrussell/oh-my-zsh.git
>> Cloning into 'oh-my-zsh'...
>> remote: Counting objects: 11830, done.
>> remote: Total 11830 (delta 0), reused 0 (delta 0)
>> Receiving objects: 100% (11830/11830), 2.12 MiB | 481.00 KiB/s, done.
>> Resolving deltas: 100% (6510/6510), done.
>> warning: unable to unlink 
>> /Volumes/installer/oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation 
>> not permitted
> 
> This should be fixable from Git itself, by replacing the calls to
> "unlink" with something like
> 
> int unlink_or_chmod(...) {
>   if (unlink(...)) {
>   chmod(...); // give user write permission
>   return unlink(...);
>   }
> }
> 
> This does not add extra cost in the normal case, and would fix this
> particular issue for afp shares. So, I think that would fix the biggest
> problem for afp-share users without disturbing others. It seems
> reasonable to me to do that unconditionnally.
> 
>> $ rm -rf oh-my-zsh/.git/objects/pack/tmp_*
>> rm: oh-my-zsh/.git/objects/pack/tmp_idx_oUN1sb: Operation not permitted
>> rm: oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not permitted
> 
> What happens if you do "rm -fr oh-my-zsh/.git/objects/pack/" (i.e.
> remove the directory, not the files)?
> 
> If you can still remove the directory, then I'd say the solution above
> could be sufficient: the user isn't supposed to interfer with the
> content of .git/objects other than by using Git, and if he or she does,
> then asking a chmod prior to an rm seems reasonable.
> 
> If you can't, then it's another problematic use-case (basically, you
> can't just "rm -fr" a whole clone), and then it deserves at least an
> opt-in configuration to get writable pack files.
> 
> (Unfortunately, I suspect we're in the later case)
> 
>> If you insist on setting the tmp idx & pack file permission to 0444 at
>> least give it a u+w permission whenever you try to unlink and rename
>> it so it won’t fail.
> 
> Yes. In case you hadn't guessed, this is precisely what I had in mind
> when I asked "Is it a problem when using Git [...] or when trying to
> remove files outside Git?".
> 
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Yes. It’s a problem when using Git where it fails to unlink and rename the tmp 
idx and pack files.
The reason I tries to rm -rf the tmp_idx_XX and tmp_pack_XX is to proof 
a point why Git fails

Perhaps my explanation wasn’t clear enough. Maybe it’s hard for you to 
understand without having to test it yourself on a AFP filesystem.

Let me explain why AFP filesystem is more strict and different from your 
typical filesystem like ext4,hfs+,etc.

$ mkdir testdir; chmod 0755 testdir; touch testdir/testfile; chmod 0444 
testdir/testfile; ls -la testdir
total 0
drwxr-xr-x  1 user  staff  264 Feb 18 00:26 .
drwx--  1 user  staff  264 Feb 18 00:26 ..
-r--r--r--  1 user  staff0 Feb 18 00:26 testfile

$ rm -rf testdir
rm: testdir/testfile: Operation not permitted
rm: testdir: Directory not empty

$ chmod +w testdir/testfile; ls -la testdir
total 0
drwxr-xr-x  1 riaf  staff  264 Feb 18 00:26 .
drwx--  1 riaf  staff  264 Feb 18 00:26 ..
-rw-r—r--  1 riaf  staff0 Feb 18 00:26 testfile

$ rm -rf testdir <—— No error message

This show that you cannot delete a directory or a file without a write 
permission in AFP filesystem.

The problem with Git failing is not because its inability to delete a directory 
but its inability to unlink and rename tmp_idx_XX and tmp_pack_XX 
because those files were set to 0444 by odb_mkstemp.
Try google for “Git AFP” and you will see a lot people are facing with the same 
problem.

Regarding your suggestion, yes I think it would work but you have to take care 
(chmod) every calls that rename or unlink or delete files with 0444 permission.

Regards,
Fairuzan




signature.asc
Description: Message signed with OpenPGP using GPGMail


[PATCH v3 08/13] commit: avoid race when creating orphan commits

2015-02-17 Thread Michael Haggerty
If HEAD doesn't point at anything during the initial check, then we
should make sure that it *still* doesn't point at anything when we are
ready to update the reference. Otherwise, another process might commit
while we are working (e.g., while we are waiting for the user to edit
the commit message) and we will silently overwrite it.

This fixes a failing test in t7516.

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 builtin/commit.c| 2 +-
 t/t7516-commit-races.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8afb0ff..682f922 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1766,7 +1766,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
if (!transaction ||
ref_transaction_update(transaction, "HEAD", sha1,
   current_head
-  ? current_head->object.sha1 : NULL,
+  ? current_head->object.sha1 : null_sha1,
   0, sb.buf, &err) ||
ref_transaction_commit(transaction, &err)) {
rollback_index_files();
diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
index ed04d1c..f2ce14e 100755
--- a/t/t7516-commit-races.sh
+++ b/t/t7516-commit-races.sh
@@ -3,7 +3,7 @@
 test_description='git commit races'
 . ./test-lib.sh
 
-test_expect_failure 'race to create orphan commit' '
+test_expect_success 'race to create orphan commit' '
write_script hare-editor <<-\EOF &&
git commit --allow-empty -m hare
EOF
-- 
2.1.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


[PATCH v3 01/13] refs: move REF_DELETING to refs.c

2015-02-17 Thread Michael Haggerty
It is only used internally now. Document it a little bit better, too.

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 refs.c | 6 ++
 refs.h | 4 +---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index ab2f2a9..5e6355c 100644
--- a/refs.c
+++ b/refs.c
@@ -35,6 +35,12 @@ static unsigned char refname_disposition[256] = {
 };
 
 /*
+ * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
+ * refs (i.e., because the reference is about to be deleted anyway).
+ */
+#define REF_DELETING   0x02
+
+/*
  * Used as a flag to ref_transaction_delete when a loose ref is being
  * pruned.
  */
diff --git a/refs.h b/refs.h
index afa3c4d..9bf2148 100644
--- a/refs.h
+++ b/refs.h
@@ -183,12 +183,10 @@ extern int peel_ref(const char *refname, unsigned char 
*sha1);
  * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
  * REF_NODEREF: act on the ref directly, instead of dereferencing
  *  symbolic references.
- * REF_DELETING: tolerate broken refs
  *
- * Flags >= 0x100 are reserved for internal use.
+ * Other flags are reserved for internal use.
  */
 #define REF_NODEREF0x01
-#define REF_DELETING   0x02
 
 /*
  * Setup reflog before using. Set errno to something meaningful on failure.
-- 
2.1.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


[PATCH v3 12/13] update_ref(): improve documentation

2015-02-17 Thread Michael Haggerty
Add a docstring for update_ref(), emphasizing its similarity to
ref_transaction_update(). Rename its parameters to match those of
ref_transaction_update().

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 refs.c |  8 
 refs.h | 13 ++---
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 1aaba3f..8d46b08 100644
--- a/refs.c
+++ b/refs.c
@@ -3738,8 +3738,8 @@ int ref_transaction_verify(struct ref_transaction 
*transaction,
  flags, NULL, err);
 }
 
-int update_ref(const char *action, const char *refname,
-  const unsigned char *sha1, const unsigned char *oldval,
+int update_ref(const char *msg, const char *refname,
+  const unsigned char *new_sha1, const unsigned char *old_sha1,
   unsigned int flags, enum action_on_err onerr)
 {
struct ref_transaction *t;
@@ -3747,8 +3747,8 @@ int update_ref(const char *action, const char *refname,
 
t = ref_transaction_begin(&err);
if (!t ||
-   ref_transaction_update(t, refname, sha1, oldval,
-  flags, action, &err) ||
+   ref_transaction_update(t, refname, new_sha1, old_sha1,
+  flags, msg, &err) ||
ref_transaction_commit(t, &err)) {
const char *str = "update_ref failed for ref '%s': %s";
 
diff --git a/refs.h b/refs.h
index 5e139d5..bb9d7b5 100644
--- a/refs.h
+++ b/refs.h
@@ -344,9 +344,16 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
  */
 void ref_transaction_free(struct ref_transaction *transaction);
 
-/** Lock a ref and then write its file */
-int update_ref(const char *action, const char *refname,
-  const unsigned char *sha1, const unsigned char *oldval,
+/**
+ * Lock, update, and unlock a single reference. This function
+ * basically does a transaction containing a single call to
+ * ref_transaction_update(). The parameters to this function have the
+ * same meaning as the corresponding parameters to
+ * ref_transaction_update(). Handle errors as requested by the `onerr`
+ * argument.
+ */
+int update_ref(const char *msg, const char *refname,
+  const unsigned char *new_sha1, const unsigned char *old_sha1,
   unsigned int flags, enum action_on_err onerr);
 
 extern int parse_hide_refs_config(const char *var, const char *value, const 
char *);
-- 
2.1.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


[PATCH v3 03/13] refs.c: Change some "flags" to "unsigned int"

2015-02-17 Thread Michael Haggerty
Change the following functions' "flags" arguments from "int" to
"unsigned int":

* ref_transaction_update()
* ref_transaction_create()
* ref_transaction_delete()
* update_ref()
* delete_ref()
* lock_ref_sha1_basic()

Also change the "flags" member in "struct ref_update" to unsigned.

Suggested-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
---
 builtin/update-ref.c |  3 ++-
 cache.h  |  2 +-
 refs.c   | 18 +-
 refs.h   | 10 +-
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 2497ba4..9a1659e 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -353,7 +353,8 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
 {
const char *refname, *oldval;
unsigned char sha1[20], oldsha1[20];
-   int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+   int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0;
+   unsigned int flags = 0;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the 
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
diff --git a/cache.h b/cache.h
index f704af5..ab265be 100644
--- a/cache.h
+++ b/cache.h
@@ -568,7 +568,7 @@ extern void update_index_if_able(struct index_state *, 
struct lock_file *);
 extern int hold_locked_index(struct lock_file *, int);
 extern void set_alternate_index_output(const char *);
 
-extern int delete_ref(const char *, const unsigned char *sha1, int delopt);
+extern int delete_ref(const char *, const unsigned char *sha1, unsigned int 
flags);
 
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
diff --git a/refs.c b/refs.c
index 4de1383..a203e44 100644
--- a/refs.c
+++ b/refs.c
@@ -2256,7 +2256,7 @@ int dwim_log(const char *str, int len, unsigned char 
*sha1, char **log)
 static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
const struct string_list *skip,
-   int flags, int *type_p)
+   unsigned int flags, int *type_p)
 {
char *ref_file;
const char *orig_refname = refname;
@@ -2740,14 +2740,14 @@ static int delete_ref_loose(struct ref_lock *lock, int 
flag, struct strbuf *err)
return 0;
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+int delete_ref(const char *refname, const unsigned char *sha1, unsigned int 
flags)
 {
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
transaction = ref_transaction_begin(&err);
if (!transaction ||
-   ref_transaction_delete(transaction, refname, sha1, delopt,
+   ref_transaction_delete(transaction, refname, sha1, flags,
   sha1 && !is_null_sha1(sha1), NULL, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
@@ -3571,7 +3571,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 struct ref_update {
unsigned char new_sha1[20];
unsigned char old_sha1[20];
-   int flags; /* REF_NODEREF? */
+   unsigned int flags; /* REF_NODEREF? */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
int type;
@@ -3644,7 +3644,7 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
   const char *refname,
   const unsigned char *new_sha1,
   const unsigned char *old_sha1,
-  int flags, int have_old, const char *msg,
+  unsigned int flags, int have_old, const char *msg,
   struct strbuf *err)
 {
struct ref_update *update;
@@ -3678,7 +3678,7 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
 int ref_transaction_create(struct ref_transaction *transaction,
   const char *refname,
   const unsigned char *new_sha1,
-  int flags, const char *msg,
+  unsigned int flags, const char *msg,
   struct strbuf *err)
 {
return ref_transaction_update(transaction, refname, new_sha1,
@@ -3688,7 +3688,7 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
   const char *refname,
   const unsigned char *old_sha1,
-  int flags, int have_old, const char *msg,
+  unsigned int flags, int have_old, const char *msg,
   struct strbuf *err)
 {
return re

[PATCH v3 00/13] Allow reference values to be checked in a transaction

2015-02-17 Thread Michael Haggerty
This is v3 of the patch series; I think I have addressed all of the
feedback raised about v1 [1] and v2 [2]. Thanks to Stefan Beller and
Junio for their feedback about v2.

There are only two significant changes since v2:

* Add a new patch [03/13] that changes a lot of "flags" variables from
  "int" to "unsigned int".

* Rework t7516:
  * Remove unneeded "test_tick".
  * Don't pass "-q" to "grep".
  * Use different tokens for the two tests to remove ambiguity.

This branch is also available in my GitHub account [3] as branch
"refs-have-new".

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/263522
[2] http://thread.gmane.org/gmane.comp.version-control.git/263718
[3] https://github.com/mhagger/git

Michael Haggerty (13):
  refs: move REF_DELETING to refs.c
  refs: remove the gap in the REF_* constant values
  refs.c: Change some "flags" to "unsigned int"
  struct ref_update: move "have_old" into "flags"
  ref_transaction_update(): remove "have_old" parameter
  ref_transaction_delete(): remove "have_old" parameter
  commit: add tests of commit races
  commit: avoid race when creating orphan commits
  ref_transaction_create(): check that new_sha1 is valid
  ref_transaction_delete(): check that old_sha1 is not null_sha1
  ref_transaction_verify(): new function to check a reference's value
  update_ref(): improve documentation
  refs.h: Remove duplication in function docstrings

 branch.c|   5 +-
 builtin/commit.c|   4 +-
 builtin/fetch.c |   6 ++-
 builtin/receive-pack.c  |   5 +-
 builtin/replace.c   |   2 +-
 builtin/tag.c   |   2 +-
 builtin/update-ref.c|  20 +++
 cache.h |   2 +-
 fast-import.c   |   6 +--
 refs.c  | 140 +---
 refs.h  | 113 ++
 sequencer.c |   2 +-
 t/t7516-commit-races.sh |  30 +++
 walker.c|   2 +-
 14 files changed, 233 insertions(+), 106 deletions(-)
 create mode 100755 t/t7516-commit-races.sh

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


[PATCH v3 11/13] ref_transaction_verify(): new function to check a reference's value

2015-02-17 Thread Michael Haggerty
If NULL is passed to ref_transaction_update()'s new_sha1 parameter,
then just verify old_sha1 (under lock) without trying to change the
new value of the reference.

Use this functionality to add a new function ref_transaction_verify(),
which checks the current value of the reference under lock but doesn't
change it.

Use ref_transaction_verify() in the implementation of "git update-ref
--stdin"'s "verify" command to avoid the awkward need to "update" the
reference to its existing value.

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 builtin/update-ref.c |  7 ++-
 refs.c   | 47 +++
 refs.h   | 34 ++
 3 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 226995f..3d79a46 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -282,7 +282,6 @@ static const char *parse_cmd_verify(struct ref_transaction 
*transaction,
 {
struct strbuf err = STRBUF_INIT;
char *refname;
-   unsigned char new_sha1[20];
unsigned char old_sha1[20];
 
refname = parse_refname(input, &next);
@@ -293,13 +292,11 @@ static const char *parse_cmd_verify(struct 
ref_transaction *transaction,
PARSE_SHA1_OLD))
hashclr(old_sha1);
 
-   hashcpy(new_sha1, old_sha1);
-
if (*next != line_termination)
die("verify %s: extra input: %s", refname, next);
 
-   if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-  update_flags, msg, &err))
+   if (ref_transaction_verify(transaction, refname, old_sha1,
+  update_flags, &err))
die("%s", err.buf);
 
update_flags = 0;
diff --git a/refs.c b/refs.c
index d5bfd11..1aaba3f 100644
--- a/refs.c
+++ b/refs.c
@@ -47,10 +47,16 @@ static unsigned char refname_disposition[256] = {
 #define REF_ISPRUNING  0x04
 
 /*
+ * Used as a flag in ref_update::flags when the reference should be
+ * updated to new_sha1.
+ */
+#define REF_HAVE_NEW   0x08
+
+/*
  * Used as a flag in ref_update::flags when old_sha1 should be
  * checked.
  */
-#define REF_HAVE_OLD   0x08
+#define REF_HAVE_OLD   0x10
 
 /*
  * Try to read one refname component from the front of refname.
@@ -3577,10 +3583,17 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
  * not exist before update.
  */
 struct ref_update {
+   /*
+* If (flags & REF_HAVE_NEW), set the reference to this value:
+*/
unsigned char new_sha1[20];
+   /*
+* If (flags & REF_HAVE_OLD), check that the reference
+* previously had this value:
+*/
unsigned char old_sha1[20];
/*
-* One or more of REF_HAVE_OLD, REF_NODEREF,
+* One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
 * REF_DELETING, and REF_ISPRUNING:
 */
unsigned int flags;
@@ -3665,7 +3678,7 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: update called for transaction that is not open");
 
-   if (!is_null_sha1(new_sha1) &&
+   if (new_sha1 && !is_null_sha1(new_sha1) &&
check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
strbuf_addf(err, "refusing to update ref with bad name %s",
refname);
@@ -3673,7 +3686,10 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
}
 
update = add_update(transaction, refname);
-   hashcpy(update->new_sha1, new_sha1);
+   if (new_sha1) {
+   hashcpy(update->new_sha1, new_sha1);
+   flags |= REF_HAVE_NEW;
+   }
if (old_sha1) {
hashcpy(update->old_sha1, old_sha1);
flags |= REF_HAVE_OLD;
@@ -3709,6 +3725,19 @@ int ref_transaction_delete(struct ref_transaction 
*transaction,
  flags, msg, err);
 }
 
+int ref_transaction_verify(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *old_sha1,
+  unsigned int flags,
+  struct strbuf *err)
+{
+   if (!old_sha1)
+   die("BUG: verify called with old_sha1 set to NULL");
+   return ref_transaction_update(transaction, refname,
+ NULL, old_sha1,
+ flags, NULL, err);
+}
+
 int update_ref(const char *action, const char *refname,
   const unsigned char *sha1, const unsigned char *oldval,
   unsigned int flags, enum action_on_err onerr)
@@ -3797,7 +3826,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
struct ref_update *update = updates[i];
unsigned 

[PATCH v3 06/13] ref_transaction_delete(): remove "have_old" parameter

2015-02-17 Thread Michael Haggerty
Instead, verify the reference's old value if and only if old_sha1 is
non-NULL.

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 builtin/receive-pack.c |  3 +--
 builtin/update-ref.c   |  5 +++--
 refs.c | 11 ++-
 refs.h |  6 +++---
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0be50e9..70e9ce5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -953,8 +953,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
if (ref_transaction_delete(transaction,
   namespaced_name,
   old_sha1,
-  0, old_sha1 != NULL,
-  "push", &err)) {
+  0, "push", &err)) {
rp_error("%s", err.buf);
strbuf_release(&err);
return "failed to delete";
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1ad6ce1..226995f 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -265,8 +265,9 @@ static const char *parse_cmd_delete(struct ref_transaction 
*transaction,
if (*next != line_termination)
die("delete %s: extra input: %s", refname, next);
 
-   if (ref_transaction_delete(transaction, refname, old_sha1,
-  update_flags, have_old, msg, &err))
+   if (ref_transaction_delete(transaction, refname,
+  have_old ? old_sha1 : NULL,
+  update_flags, msg, &err))
die("%s", err.buf);
 
update_flags = 0;
diff --git a/refs.c b/refs.c
index e88817c..e3c4ab5 100644
--- a/refs.c
+++ b/refs.c
@@ -2576,7 +2576,7 @@ static void prune_ref(struct ref_to_prune *r)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_delete(transaction, r->name, r->sha1,
-  REF_ISPRUNING, 1, NULL, &err) ||
+  REF_ISPRUNING, NULL, &err) ||
ref_transaction_commit(transaction, &err)) {
ref_transaction_free(transaction);
error("%s", err.buf);
@@ -2753,8 +2753,9 @@ int delete_ref(const char *refname, const unsigned char 
*sha1, unsigned int flag
 
transaction = ref_transaction_begin(&err);
if (!transaction ||
-   ref_transaction_delete(transaction, refname, sha1, flags,
-  sha1 && !is_null_sha1(sha1), NULL, &err) ||
+   ref_transaction_delete(transaction, refname,
+  (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL,
+  flags, NULL, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
ref_transaction_free(transaction);
@@ -3696,11 +3697,11 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
   const char *refname,
   const unsigned char *old_sha1,
-  unsigned int flags, int have_old, const char *msg,
+  unsigned int flags, const char *msg,
   struct strbuf *err)
 {
return ref_transaction_update(transaction, refname,
- null_sha1, have_old ? old_sha1 : NULL,
+ null_sha1, old_sha1,
  flags, msg, err);
 }
 
diff --git a/refs.h b/refs.h
index dced4c9..100731d 100644
--- a/refs.h
+++ b/refs.h
@@ -295,8 +295,8 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
   struct strbuf *err);
 
 /*
- * Add a reference deletion to transaction.  If have_old is true, then
- * old_sha1 holds the value that the reference should have had before
+ * Add a reference deletion to transaction.  If old_sha1 is non-NULL, then
+ * it holds the value that the reference should have had before
  * the update (which must not be the null SHA-1).
  * Function returns 0 on success and non-zero on failure. A failure to delete
  * means that the transaction as a whole has failed and will need to be
@@ -305,7 +305,7 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
 int ref_transaction_delete(struct ref_transaction *transaction,
   const char *refname,
   const unsigned char *old_sha1,
-  unsigned int flags, int have_old, const char *msg,
+  unsigned int flags, const char *msg,
   struct strbuf *err);
 
 /*
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsu

[PATCH v3 13/13] refs.h: Remove duplication in function docstrings

2015-02-17 Thread Michael Haggerty
Add more information to the comment introducing the four reference
transaction update functions, so that each function's docstring
doesn't have to repeat it. Add a pointer from the individual
functions' docstrings to the introductory comment.

Signed-off-by: Michael Haggerty 
---
 refs.h | 66 +++---
 1 file changed, 43 insertions(+), 23 deletions(-)

diff --git a/refs.h b/refs.h
index bb9d7b5..cf642e6 100644
--- a/refs.h
+++ b/refs.h
@@ -255,11 +255,31 @@ enum action_on_err {
 struct ref_transaction *ref_transaction_begin(struct strbuf *err);
 
 /*
- * The following functions add a reference check or update to a
- * ref_transaction.  In all of them, refname is the name of the
- * reference to be affected.  The functions make internal copies of
- * refname and msg, so the caller retains ownership of these parameters.
- * flags can be REF_NODEREF; it is passed to update_ref_lock().
+ * Reference transaction updates
+ *
+ * The following four functions add a reference check or update to a
+ * ref_transaction.  They have some common similar parameters:
+ *
+ * transaction -- a pointer to an open ref_transaction, obtained
+ * from ref_transaction_begin().
+ *
+ * refname -- the name of the reference to be affected.
+ *
+ * flags -- flags affecting the update, passed to
+ * update_ref_lock(). Can be REF_NODEREF, which means that
+ * symbolic references should not be followed.
+ *
+ * msg -- a message describing the change (for the reflog).
+ *
+ * err -- a strbuf for receiving a description of any error that
+ * might have occured.
+ *
+ * The functions make internal copies of refname and msg, so the
+ * caller retains ownership of these parameters.
+ *
+ * The functions return 0 on success and non-zero on failure. A
+ * failure means that the transaction as a whole has failed and needs
+ * to be rolled back.
  */
 
 /*
@@ -273,9 +293,8 @@ struct ref_transaction *ref_transaction_begin(struct strbuf 
*err);
  * whole transaction fails. If old_sha1 is NULL, then the previous
  * value is not checked.
  *
- * Return 0 on success and non-zero on failure. Any failure in the
- * transaction means that the transaction as a whole has failed and
- * will need to be rolled back.
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_update(struct ref_transaction *transaction,
   const char *refname,
@@ -285,13 +304,13 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
   struct strbuf *err);
 
 /*
- * Add a reference creation to transaction.  new_sha1 is the value
- * that the reference should have after the update; it must not be the
- * null SHA-1.  It is verified that the reference does not exist
+ * Add a reference creation to transaction. new_sha1 is the value that
+ * the reference should have after the update; it must not be
+ * null_sha1. It is verified that the reference does not exist
  * already.
- * Function returns 0 on success and non-zero on failure. A failure to create
- * means that the transaction as a whole has failed and will need to be
- * rolled back.
+ *
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_create(struct ref_transaction *transaction,
   const char *refname,
@@ -300,12 +319,12 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
   struct strbuf *err);
 
 /*
- * Add a reference deletion to transaction.  If old_sha1 is non-NULL, then
- * it holds the value that the reference should have had before
- * the update (which must not be the null SHA-1).
- * Function returns 0 on success and non-zero on failure. A failure to delete
- * means that the transaction as a whole has failed and will need to be
- * rolled back.
+ * Add a reference deletion to transaction. If old_sha1 is non-NULL,
+ * then it holds the value that the reference should have had before
+ * the update (which must not be null_sha1).
+ *
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_delete(struct ref_transaction *transaction,
   const char *refname,
@@ -316,9 +335,10 @@ int ref_transaction_delete(struct ref_transaction 
*transaction,
 /*
  * Verify, within a transaction, that refname has the value old_sha1,
  * or, if old_sha1 is null_sha1, then verify that the reference
- * doesn't exist. old_sha1 must be non-NULL. Function returns 0 on
- * success and non-zero on failure. A failure to verify means that the
- * transaction as a whole has failed and will need to be rolled back.
+ * doesn't exist. old_sha1 must be non-NULL.
+ *
+ * See the above comment "Reference transaction updates" for more
+ * information.
  */
 int ref_transaction_verify(struct ref_transaction *transaction,
  

[PATCH v3 10/13] ref_transaction_delete(): check that old_sha1 is not null_sha1

2015-02-17 Thread Michael Haggerty
It makes no sense to delete a reference that is already known not to
exist.

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 refs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs.c b/refs.c
index b9cf284..d5bfd11 100644
--- a/refs.c
+++ b/refs.c
@@ -3702,6 +3702,8 @@ int ref_transaction_delete(struct ref_transaction 
*transaction,
   unsigned int flags, const char *msg,
   struct strbuf *err)
 {
+   if (old_sha1 && is_null_sha1(old_sha1))
+   die("BUG: delete called with old_sha1 set to zeros");
return ref_transaction_update(transaction, refname,
  null_sha1, old_sha1,
  flags, msg, err);
-- 
2.1.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


[PATCH v3 04/13] struct ref_update: move "have_old" into "flags"

2015-02-17 Thread Michael Haggerty
Instead of having a separate have_old field, record this boolean value
as a bit in the "flags" field.

Signed-off-by: Michael Haggerty 
---
 refs.c | 45 -
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index a203e44..e3a2ba8 100644
--- a/refs.c
+++ b/refs.c
@@ -41,12 +41,18 @@ static unsigned char refname_disposition[256] = {
 #define REF_DELETING   0x02
 
 /*
- * Used as a flag to ref_transaction_delete when a loose ref is being
+ * Used as a flag in ref_update::flags when a loose ref is being
  * pruned.
  */
 #define REF_ISPRUNING  0x04
 
 /*
+ * Used as a flag in ref_update::flags when old_sha1 should be
+ * checked.
+ */
+#define REF_HAVE_OLD   0x08
+
+/*
  * Try to read one refname component from the front of refname.
  * Return the length of the component found, or -1 if the component is
  * not legal.  It is legal if it is something reasonable to have under
@@ -3563,16 +3569,20 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 }
 
 /**
- * Information needed for a single ref update.  Set new_sha1 to the
- * new value or to zero to delete the ref.  To check the old value
- * while locking the ref, set have_old to 1 and set old_sha1 to the
- * value or to zero to ensure the ref does not exist before update.
+ * Information needed for a single ref update. Set new_sha1 to the new
+ * value or to null_sha1 to delete the ref. To check the old value
+ * while the ref is locked, set (flags & REF_HAVE_OLD) and set
+ * old_sha1 to the old value, or to null_sha1 to ensure the ref does
+ * not exist before update.
  */
 struct ref_update {
unsigned char new_sha1[20];
unsigned char old_sha1[20];
-   unsigned int flags; /* REF_NODEREF? */
-   int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
+   /*
+* One or more of REF_HAVE_OLD, REF_NODEREF,
+* REF_DELETING, and REF_ISPRUNING:
+*/
+   unsigned int flags;
struct ref_lock *lock;
int type;
char *msg;
@@ -3666,10 +3676,11 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
 
update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
-   update->flags = flags;
-   update->have_old = have_old;
-   if (have_old)
+   if (have_old) {
hashcpy(update->old_sha1, old_sha1);
+   flags |= REF_HAVE_OLD;
+   }
+   update->flags = flags;
if (msg)
update->msg = xstrdup(msg);
return 0;
@@ -3785,13 +3796,13 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
if (is_null_sha1(update->new_sha1))
flags |= REF_DELETING;
-   update->lock = lock_ref_sha1_basic(update->refname,
-  (update->have_old ?
-   update->old_sha1 :
-   NULL),
-  NULL,
-  flags,
-  &update->type);
+   update->lock = lock_ref_sha1_basic(
+   update->refname,
+   ((update->flags & REF_HAVE_OLD) ?
+update->old_sha1 : NULL),
+   NULL,
+   flags,
+   &update->type);
if (!update->lock) {
ret = (errno == ENOTDIR)
? TRANSACTION_NAME_CONFLICT
-- 
2.1.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


[PATCH v3 02/13] refs: remove the gap in the REF_* constant values

2015-02-17 Thread Michael Haggerty
There is no reason to "reserve" a gap between the public and private
flags values.

Signed-off-by: Michael Haggerty 
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 5e6355c..4de1383 100644
--- a/refs.c
+++ b/refs.c
@@ -44,7 +44,8 @@ static unsigned char refname_disposition[256] = {
  * Used as a flag to ref_transaction_delete when a loose ref is being
  * pruned.
  */
-#define REF_ISPRUNING  0x0100
+#define REF_ISPRUNING  0x04
+
 /*
  * Try to read one refname component from the front of refname.
  * Return the length of the component found, or -1 if the component is
-- 
2.1.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


[PATCH v3 09/13] ref_transaction_create(): check that new_sha1 is valid

2015-02-17 Thread Michael Haggerty
Creating a reference requires a new_sha1 that is not NULL and not
null_sha1.

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 refs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs.c b/refs.c
index e3c4ab5..b9cf284 100644
--- a/refs.c
+++ b/refs.c
@@ -3690,6 +3690,8 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
   unsigned int flags, const char *msg,
   struct strbuf *err)
 {
+   if (!new_sha1 || is_null_sha1(new_sha1))
+   die("BUG: create called without valid new_sha1");
return ref_transaction_update(transaction, refname, new_sha1,
  null_sha1, flags, msg, err);
 }
-- 
2.1.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


[PATCH v3 07/13] commit: add tests of commit races

2015-02-17 Thread Michael Haggerty
Committing involves the following steps:

1. Determine the current value of HEAD (if any).
2. Create the new commit object.
3. Update HEAD.

Please note that step 2 can take arbitrarily long, because it might
involve the user editing a commit message.

If a second process sneaks in a commit during step 2, then the first
commit process should fail. This is usually done correctly, because
step 3 verifies that HEAD still points at the same commit that it
pointed to during step 1.

However, if there is a race when creating an *orphan* commit, then the
test in step 3 is skipped.

Add tests for proper handling of such races. One of the new tests
fails. It will be fixed in a moment.

Signed-off-by: Michael Haggerty 
---
 t/t7516-commit-races.sh | 30 ++
 1 file changed, 30 insertions(+)
 create mode 100755 t/t7516-commit-races.sh

diff --git a/t/t7516-commit-races.sh b/t/t7516-commit-races.sh
new file mode 100755
index 000..ed04d1c
--- /dev/null
+++ b/t/t7516-commit-races.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='git commit races'
+. ./test-lib.sh
+
+test_expect_failure 'race to create orphan commit' '
+   write_script hare-editor <<-\EOF &&
+   git commit --allow-empty -m hare
+   EOF
+   test_must_fail env EDITOR=./hare-editor git commit --allow-empty -m 
tortoise -e &&
+   git show -s --pretty=format:%s >subject &&
+   grep hare subject &&
+   test -z "$(git show -s --pretty=format:%P)"
+'
+
+test_expect_success 'race to create non-orphan commit' '
+   write_script airplane-editor <<-\EOF &&
+   git commit --allow-empty -m airplane
+   EOF
+   git checkout --orphan branch &&
+   git commit --allow-empty -m base &&
+   git rev-parse HEAD >base &&
+   test_must_fail env EDITOR=./airplane-editor git commit --allow-empty -m 
ship -e &&
+   git show -s --pretty=format:%s >subject &&
+   grep airplane subject &&
+   git rev-parse HEAD^ >parent &&
+   test_cmp base parent
+'
+
+test_done
-- 
2.1.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


[PATCH v3 05/13] ref_transaction_update(): remove "have_old" parameter

2015-02-17 Thread Michael Haggerty
Instead, verify the reference's old value if and only if old_sha1 is
non-NULL.

ref_transaction_delete() will get the same treatment in a moment.

Signed-off-by: Michael Haggerty 
Reviewed-by: Stefan Beller 
---
 branch.c   |  5 +++--
 builtin/commit.c   |  2 +-
 builtin/fetch.c|  6 --
 builtin/receive-pack.c |  2 +-
 builtin/replace.c  |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-ref.c   |  7 ---
 fast-import.c  |  6 +++---
 refs.c | 18 --
 refs.h |  6 +++---
 sequencer.c|  2 +-
 walker.c   |  2 +-
 12 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/branch.c b/branch.c
index 4bab55a..b002435 100644
--- a/branch.c
+++ b/branch.c
@@ -284,8 +284,9 @@ void create_branch(const char *head,
 
transaction = ref_transaction_begin(&err);
if (!transaction ||
-   ref_transaction_update(transaction, ref.buf, sha1,
-  null_sha1, 0, !forcing, msg, &err) ||
+   ref_transaction_update(transaction, ref.buf,
+  sha1, forcing ? NULL : null_sha1,
+  0, msg, &err) ||
ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/commit.c b/builtin/commit.c
index 7f46713..8afb0ff 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1767,7 +1767,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
ref_transaction_update(transaction, "HEAD", sha1,
   current_head
   ? current_head->object.sha1 : NULL,
-  0, !!current_head, sb.buf, &err) ||
+  0, sb.buf, &err) ||
ref_transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7b84d35..719bf4f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -416,8 +416,10 @@ static int s_update_ref(const char *action,
 
transaction = ref_transaction_begin(&err);
if (!transaction ||
-   ref_transaction_update(transaction, ref->name, ref->new_sha1,
-  ref->old_sha1, 0, check_old, msg, &err))
+   ref_transaction_update(transaction, ref->name,
+  ref->new_sha1,
+  check_old ? ref->old_sha1 : NULL,
+  0, msg, &err))
goto fail;
 
ret = ref_transaction_commit(transaction, &err);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e0ce78e..0be50e9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -971,7 +971,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
if (ref_transaction_update(transaction,
   namespaced_name,
   new_sha1, old_sha1,
-  0, 1, "push",
+  0, "push",
   &err)) {
rp_error("%s", err.buf);
strbuf_release(&err);
diff --git a/builtin/replace.c b/builtin/replace.c
index 85d39b5..54bf01a 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -172,7 +172,7 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
-  0, 1, NULL, &err) ||
+  0, NULL, &err) ||
ref_transaction_commit(transaction, &err))
die("%s", err.buf);
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 6dc85a9..4194b9a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -733,7 +733,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
-  0, 1, NULL, &err) ||
+  0, NULL, &err) ||
ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 9a1659e..1ad6ce1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -198,8 +198,9 @@ static const char *parse_cmd_update(struct ref_transaction 
*transaction,
if (*next != line_termination)
die("update %s: extra input: %s", refname, next);
 
-   if (r

Re: odb_mkstemp's 0444 permission broke write/delete access on AFP

2015-02-17 Thread Junio C Hamano
Matthieu Moy  writes:

> This should be fixable from Git itself, by replacing the calls to
> "unlink" with something like
>
> int unlink_or_chmod(...) {
>   if (unlink(...)) {
>   chmod(...); // give user write permission
>   return unlink(...);
>   }
> }

I agree with the approach in principle, but I wonder if we want to
contaminate the generic codepath with unlink_or_chmod().

Don't we want to have this

#undef unlink
int workaround_broken_unlink(...) {
... the same ...
}

in compat/broken-unlink.c and something like this

#ifdef BROKEN_UNLINK
#define unlink(x) workaround_broken_unlink(x)
#endif

in git-compat-util.h instead?  That way, people on well behaving
systems do not have to worry about clobbering errno and stuff,
perhaps?

--
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] builtin/push.c: make push_default a static variable

2015-02-17 Thread Junio C Hamano
Jeff King  writes:

> If we wanted to implement "@{push}" (or "@{publish}") to mean "the
> tracking ref of the remote ref you would push to if you ran git-push",
> then this is a step in the wrong direction.

Is that because push_default variable needs to be looked at from
sha1_name.c when resolving "@{push}", optionally prefixed with the
name of the branch?  I wonder if that codepath should know the gory
details of which ref at the remote the branch is pushed to and which
remote-tracking ref we use in the local repository to mirror that
remote ref in the first place?

What do we do for the @{upstream} side of the things---it calls
branch_get() and when the branch structure is returned, the details
have been computed for us so get_upstream_branch() only needs to use
the information already computed.  The interesting parts of the
computation all happen inside remote.c, it seems.

So we probably would do something similar to @{push} side, which
would mean that push_default variable and the logic needs to be
visible to remote.c if we want to have the helper that is similar to
set_merge() that is used from branch_get() to support @{upstream}.

Hmmm, I have a feeling that "with default configuration, where does
'git push' send this branch to?" logic should be contained within
the source file whose name has "push" in it and exposed as a helper
function, instead of exposing just one of the lowest level knob
push_default to outside callers and have them figure things out.

Viewed from that angle, it might be the case that remote.c knows too
much about what happens during fetch and pull, but I dunno.

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


Re: odb_mkstemp's 0444 permission broke write/delete access on AFP

2015-02-17 Thread Torsten Bögershausen
On 17/02/15 17:58, Fairuzan Roslan wrote:
> 
>> On Feb 17, 2015, at 4:51 PM, Matthieu Moy  
>> wrote:
>>
>> Fairuzan Roslan  writes:
>>
>>> $ git clone https://github.com/robbyrussell/oh-my-zsh.git
>>> Cloning into 'oh-my-zsh'...
>>> remote: Counting objects: 11830, done.
>>> remote: Total 11830 (delta 0), reused 0 (delta 0)
>>> Receiving objects: 100% (11830/11830), 2.12 MiB | 481.00 KiB/s, done.
>>> Resolving deltas: 100% (6510/6510), done.
>>> warning: unable to unlink 
>>> /Volumes/installer/oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation 
>>> not permitted
>>
>> This should be fixable from Git itself, by replacing the calls to
>> "unlink" with something like
>>
>> int unlink_or_chmod(...) {
>>  if (unlink(...)) {
>>  chmod(...); // give user write permission
>>  return unlink(...);
>>  }
>> }
>>
>> This does not add extra cost in the normal case, and would fix this
>> particular issue for afp shares. So, I think that would fix the biggest
>> problem for afp-share users without disturbing others. It seems
>> reasonable to me to do that unconditionnally.
>>
>>> $ rm -rf oh-my-zsh/.git/objects/pack/tmp_*
>>> rm: oh-my-zsh/.git/objects/pack/tmp_idx_oUN1sb: Operation not permitted
>>> rm: oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not permitted
>>
>> What happens if you do "rm -fr oh-my-zsh/.git/objects/pack/" (i.e.
>> remove the directory, not the files)?
>>
>> If you can still remove the directory, then I'd say the solution above
>> could be sufficient: the user isn't supposed to interfer with the
>> content of .git/objects other than by using Git, and if he or she does,
>> then asking a chmod prior to an rm seems reasonable.
>>
>> If you can't, then it's another problematic use-case (basically, you
>> can't just "rm -fr" a whole clone), and then it deserves at least an
>> opt-in configuration to get writable pack files.
>>
>> (Unfortunately, I suspect we're in the later case)
>>
>>> If you insist on setting the tmp idx & pack file permission to 0444 at
>>> least give it a u+w permission whenever you try to unlink and rename
>>> it so it won’t fail.
>>
>> Yes. In case you hadn't guessed, this is precisely what I had in mind
>> when I asked "Is it a problem when using Git [...] or when trying to
>> remove files outside Git?".
>>
>> --
>> Matthieu Moy
>> http://www-verimag.imag.fr/~moy/
> 
> Yes. It’s a problem when using Git where it fails to unlink and rename the 
> tmp idx and pack files.
> The reason I tries to rm -rf the tmp_idx_XX and tmp_pack_XX is to 
> proof a point why Git fails
> 
> Perhaps my explanation wasn’t clear enough. Maybe it’s hard for you to 
> understand without having to test it yourself on a AFP filesystem.
> 
> Let me explain why AFP filesystem is more strict and different from your 
> typical filesystem like ext4,hfs+,etc.
> 
> $ mkdir testdir; chmod 0755 testdir; touch testdir/testfile; chmod 0444 
> testdir/testfile; ls -la testdir
> total 0
> drwxr-xr-x  1 user  staff  264 Feb 18 00:26 .
> drwx--  1 user  staff  264 Feb 18 00:26 ..
> -r--r--r--  1 user  staff0 Feb 18 00:26 testfile
> 
> $ rm -rf testdir
> rm: testdir/testfile: Operation not permitted
> rm: testdir: Directory not empty
> 
This works on my system (Mac OS 10.9 as server and client)

> $ chmod +w testdir/testfile; ls -la testdir
> total 0
> drwxr-xr-x  1 riaf  staff  264 Feb 18 00:26 .
> drwx--  1 riaf  staff  264 Feb 18 00:26 ..
> -rw-r—r--  1 riaf  staff0 Feb 18 00:26 testfile
> 
> $ rm -rf testdir <—— No error message
> 
> This show that you cannot delete a directory or a file without a write 
> permission in AFP filesystem.
> 
> The problem with Git failing is not because its inability to delete a 
> directory but its inability to unlink and rename tmp_idx_XX and 
> tmp_pack_XX because those files were set to 0444 by odb_mkstemp.
> Try google for “Git AFP” and you will see a lot people are facing with the 
> same problem.
Yes, (at least to my knowledge) you seem to be one of the first to report it 
here, thanks for that.
> 
> Regarding your suggestion, yes I think it would work but you have to take 
> care (chmod) every calls that rename or unlink or delete files with 0444 
> permission.
> 
> Regards,
> Fairuzan
> 
The "right" solution is to make a wrapper function, and to re-define unlink() 
and rename() with help
of the preprocessor.

git-compat-util.h has an example for fopen(), so that can be used for a patch.

And no, as I can not reproduce it here, I can only help with reviews or so.







 

--
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] read-cache.c: free cache entry when refreshing fails

2015-02-17 Thread Stefan Beller
This fixes a memory leak, when building the cache entries as
refresh_cache_entry may decide to return NULL in case of

I am not sure how severe this memory leak is as it was found by
scan.coverity.com, CID 290041.

Signed-off-by: Stefan Beller 
---
 read-cache.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 9cff715..8d71860 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -725,7 +725,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
unsigned int refresh_options)
 {
int size, len;
-   struct cache_entry *ce;
+   struct cache_entry *ce, *ret;
 
if (!verify_path(path)) {
error("Invalid path '%s'", path);
@@ -742,7 +742,13 @@ struct cache_entry *make_cache_entry(unsigned int mode,
ce->ce_namelen = len;
ce->ce_mode = create_ce_mode(mode);
 
-   return refresh_cache_entry(ce, refresh_options);
+   ret = refresh_cache_entry(ce, refresh_options);
+   if (!ret) {
+   free(ce);
+   return NULL;
+   } else {
+   return ret;
+   }
 }
 
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
-- 
2.3.0.81.gc37f363

--
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] read-cache.c: free cache entry when refreshing fails

2015-02-17 Thread Junio C Hamano
Stefan Beller  writes:

> This fixes a memory leak, when building the cache entries as
> refresh_cache_entry may decide to return NULL in case of
>

in case of what?

I briefly wondered if refresh_cache_ent() should do the freeing but
that does not make sense at all if done unconditionally because the
other caller of the function does want to retain ce on error, and it
makes little sense to invent FREE_CE_ON_ERROR bit in refresh_option,
either, so this fix looks sensible.

> I am not sure how severe this memory leak is as it was found by
> scan.coverity.com, CID 290041.
>
> Signed-off-by: Stefan Beller 
> ---
>  read-cache.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 9cff715..8d71860 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -725,7 +725,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
>   unsigned int refresh_options)
>  {
>   int size, len;
> - struct cache_entry *ce;
> + struct cache_entry *ce, *ret;
>  
>   if (!verify_path(path)) {
>   error("Invalid path '%s'", path);
> @@ -742,7 +742,13 @@ struct cache_entry *make_cache_entry(unsigned int mode,
>   ce->ce_namelen = len;
>   ce->ce_mode = create_ce_mode(mode);
>  
> - return refresh_cache_entry(ce, refresh_options);
> + ret = refresh_cache_entry(ce, refresh_options);
> + if (!ret) {
> + free(ce);
> + return NULL;
> + } else {
> + return ret;
> + }
>  }
>  
>  int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
--
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] builtin/push.c: make push_default a static variable

2015-02-17 Thread Jeff King
On Tue, Feb 17, 2015 at 09:45:07AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > If we wanted to implement "@{push}" (or "@{publish}") to mean "the
> > tracking ref of the remote ref you would push to if you ran git-push",
> > then this is a step in the wrong direction.
> 
> Is that because push_default variable needs to be looked at from
> sha1_name.c when resolving "@{push}", optionally prefixed with the
> name of the branch?

Yes, exactly.

> I wonder if that codepath should know the gory details of which ref at
> the remote the branch is pushed to and which remote-tracking ref we
> use in the local repository to mirror that remote ref in the first
> place?

I think that was one of the ugly bits from the series; that we had to
reimplement "where would we push" and "what would it be called if we
pushed and then fetched"? The former cares about push_default, and the
latter has to apply push and then fetch refspecs.

If you want to peek at it again, it's at:

  https://github.com/peff/git/commit/8859afb1af63cb3cb0bc4cc8c1719c2011f406c9

(but note that it should not be called @{publish}, as per earlier
discussions).

> What do we do for the @{upstream} side of the things---it calls
> branch_get() and when the branch structure is returned, the details
> have been computed for us so get_upstream_branch() only needs to use
> the information already computed.  The interesting parts of the
> computation all happen inside remote.c, it seems.
> 
> So we probably would do something similar to @{push} side, which
> would mean that push_default variable and the logic needs to be
> visible to remote.c if we want to have the helper that is similar to
> set_merge() that is used from branch_get() to support @{upstream}.

Sure, we could go that way. But I don't think it changes the issue for
_this_ patch series, which is that the variable needs visibility outside
of builtin/push.c (and we need to load the config for programs besides
git-push).

> Hmmm, I have a feeling that "with default configuration, where does
> 'git push' send this branch to?" logic should be contained within
> the source file whose name has "push" in it and exposed as a helper
> function, instead of exposing just one of the lowest level knob
> push_default to outside callers and have them figure things out.
> 
> Viewed from that angle, it might be the case that remote.c knows too
> much about what happens during fetch and pull, but I dunno.

Yeah, it would be nice if there were a convenient lib-ified set of
functions for getting this information, and "fetch" and "push" commands
were built on top of it. I don't know how painful that would be, though.
The existing code has grown somewhat organically.

But even with that change, the lib-ified code needs to hook into
git_default_config (or do its own config lookup) so that we get the
proper value no matter who the caller is.

-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


[no subject]

2015-02-17 Thread Stefan Beller

On Tue, Feb 17, 2015 at 10:14 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This fixes a memory leak, when building the cache entries as
>> refresh_cache_entry may decide to return NULL in case of
>>
>
> in case of what?

Yeah, I got distracted when rewriting the commit message as I was 
looking at refresh_cache_ent() wondering if there is a better place to 
free the memory. Just as you said below.

Maybe we can drop that part of the sentence as it doesn't matter
why refresh_cache_ent() returns NULL. All that matters is the 
possibility of it returning NULL.

>
> I briefly wondered if refresh_cache_ent() should do the freeing but
> that does not make sense at all if done unconditionally because the
> other caller of the function does want to retain ce on error, and it
> makes little sense to invent FREE_CE_ON_ERROR bit in refresh_option,
> either, so this fix looks sensible.
>

So here is a reworded commit message:

---8<---
>From 3a1f646c1dbe828b68e1b269290d2b5593f86455 Mon Sep 17 00:00:00 2001
From: Stefan Beller 
Date: Tue, 17 Feb 2015 10:05:36 -0800
Subject: [PATCH] read-cache.c: free cache entry when refreshing fails

This fixes a memory leak when building the cache entries as
refresh_cache_entry may decide to return NULL, but it doesn't
free the cache entry structure which was passed in as an argument.

I am not sure how severe this memory leak is as it was found by
scan.coverity.com, CID 290041.

Signed-off-by: Stefan Beller 
---
 read-cache.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 9cff715..8d71860 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -725,7 +725,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
unsigned int refresh_options)
 {
int size, len;
-   struct cache_entry *ce;
+   struct cache_entry *ce, *ret;
 
if (!verify_path(path)) {
error("Invalid path '%s'", path);
@@ -742,7 +742,13 @@ struct cache_entry *make_cache_entry(unsigned int mode,
ce->ce_namelen = len;
ce->ce_mode = create_ce_mode(mode);
 
-   return refresh_cache_entry(ce, refresh_options);
+   ret = refresh_cache_entry(ce, refresh_options);
+   if (!ret) {
+   free(ce);
+   return NULL;
+   } else {
+   return ret;
+   }
 }
 
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
-- 
2.3.0.81.gc37f363

--
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: 'make test' fails in pu

2015-02-17 Thread Michael Blume
For the record, that commit also sporadically breaks test 3910 on my
system (mentioning since it's not on the list)

On Tue, Feb 17, 2015 at 12:55 AM, Jeff King  wrote:
> On Tue, Feb 17, 2015 at 09:39:17AM +0100, Dennis Kaarsemaker wrote:
>
>> Make test has been failing for 'pu' yesterday for and today at
>> t4016-diff-quote.sh. Full log:
>> http://ci.kaarsemaker.net/git/refs/heads/pu/1df29c71a731c679de9055ae5e407f3a4e18740a/artefact/test/log
>>
>> I noticed this a few times before and it tends to get fixed again
>> relatively quickly. So I'm wondering:
>>
>> - Should I even mention that it's failing, or is that just useless
>>   noise?
>> - If I should report this, I could also make my testing thing send
>>   mails. Would that be useful?
>
> If you bisect this, it turns up commit 30cd8f94f, which says:
>
> WIP: diff-b-m
>
> [...]
>
> This update is still broken and breaks a handful of tests:
>
>  4016 4023 4047 4130 6022 6031 6032 9300 9200 9300 9350
>
> Sometimes a breakage in pu is surprising (e.g., it breaks only on a
> platform that the maintainer does not run "make test" on) and we would
> want to know about it. But sometimes it is merely that there is a
> work-in-progress. And it probably requires a human to tell the
> difference.
>
> So no, I do not think automatically mailing on test failures in pu is a
> good idea. Manually peeking at them and sending fixes before the series
> is merged to next _is_ very much encouraged, though. :)
>
> Unlike "pu", "next" and "master" should never fail tests (I think that
> Junio will not push them out if the tests have failed on his system). So
> failures there are much more likely to be interesting platform bugs (but
> of course, testing "pu" is still encouraged, as we may catch problems).
>
> But even for "next", I would say blind automated emails are not nearly
> as useful as a human who has looked at the problem (and especially
> bisected).
>
> -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
--
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 00/13] Allow reference values to be checked in a transaction

2015-02-17 Thread Junio C Hamano
All looked sensible from a cursory read.

Will replace, wait for a few days and will merge to 'next' unless I
hear otherwise from others.

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: 'make test' fails in pu

2015-02-17 Thread Junio C Hamano
Jeff King  writes:

> Sometimes a breakage in pu is surprising (e.g., it breaks only on a
> platform that the maintainer does not run "make test" on) and we would
> want to know about it. But sometimes it is merely that there is a
> work-in-progress. And it probably requires a human to tell the
> difference.
>
> So no, I do not think automatically mailing on test failures in pu is a
> good idea. Manually peeking at them and sending fixes before the series
> is merged to next _is_ very much encouraged, though. :)

Thanks, that is exactly what people saw.  From time to time, I queue
a topic that does not pass the tests on 'pu', hoping that somebody
sufficiently interested would step in to collaborate with the author
of the topic to move things forward when the breakage looks simple
enough, and sometimes that original author happens to be me.

This case, it turns out that the breakage is not so simple, though.
Inside the rename detection logic, I want to peek the rename source
array to decide which deletion filepair to keep, but rename source
array itself has pointers to the original filepairs the loop wants
to free, so the WIP code was peeking into a freed piece memory X-<.

> Unlike "pu", "next" and "master" should never fail tests (I think that
> Junio will not push them out if the tests have failed on his system). So
> failures there are much more likely to be interesting platform bugs (but
> of course, testing "pu" is still encouraged, as we may catch problems).

True.  I do not mind automated tests on 'next', and testing 'pu' and
helping the topic to move forward is very much encouraged, but
sending test results on 'pu' blindly is often more noise than it is
worth.

--
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 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Junio C Hamano
Paolo Bonzini  writes:

> On 16/02/2015 20:47, Junio C Hamano wrote:
>> Paolo Bonzini  writes:
>> 
>>> From: Paolo Bonzini 
>>>
>>> After updating to git 2.3.0, "git request-pull" is stubbornly complaining
>>> that I lack a matching tag on the remote side unless I pass the third
>>> argument.  But I did prepare and push a signed tag.
>> 
>> A few questions.
>> 
>>  - what old version did you update from?  I think the "correct
>>over-eager dwimming" change was from v2.0 days.
>
> I upgraded from 1.9.  My workflow is to make a signed tag, push it and
> do "git request-pull origin/master ".
>
> My branches have a different name locally vs. remotely (e.g.
> "kvm-master" and "kvm-next" locally vs. refs/heads/master and
> refs/heads/next remotely) exactly to avoid overeager matching in
> git-request-pull.  I only ever want to request pulls based on signed tags.

So I think you would want something like this:

git tag -s for-linus kvm-next
git push  kvm-next:next tags/for-linus
git request-pull origin/master  for-linus

in the post 2.0 world with 024d34cb (request-pull: more strictly
match local/remote branches, 2014-01-22)?

>>  - what exactly do you mean by "stubbornly complain"?  I think we
>>say something about HEAD not matching the HEAD over there, which
>>I think is bogus (we should instead say things about the branch
>>you are on and the branch over there with the same name) and is
>>worth fixing.
>
> I tried both "git checkout kvm-next" and "git checkout tags/for-linus",
> and it still complains.

Sorry, I was asking what you mean by "complains" (i.e. the exact
error message).  I was and am guessing it is something like this: 

warn: No match for commit 3188ab3... found at 
warn: Are you sure you pushed 'HEAD' there?

Asking to pull 'HEAD' may be often a wrong thing to do, and I
wouldn't mind if this sequence:

git checkout kvm-next
git request-pull origin/master 

behaved the same way as

git request-pull origin/master  kvm-next

But I do not know if the implicit HEAD should DWIM locally to this:

git request-pull origin/master  for-linus

> ...  Based on your answer, it seems like you are focusing mostly
> on a branch-based workflow; ...

Not really.  I am focusing mostly on not breaking what 024d34cb0 and
dc2eacc58c fixed earlier.

> ... the two definitely have
> different requirements for DWIMming (since you cannot get a tag name via
> "git symbolic-ref" for example).  On the other hand most of the
> un-DWIMming changes were done by Linus who works a lot with (other
> people's) signed tags...
--
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 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Paolo Bonzini


On 17/02/2015 20:57, Junio C Hamano wrote:
> Sorry, I was asking what you mean by "complains" (i.e. the exact
> error message).  I was and am guessing it is something like this: 
> 
> warn: No match for commit 3188ab3... found at 
> warn: Are you sure you pushed 'HEAD' there?

Yes, it is.

> Asking to pull 'HEAD' may be often a wrong thing to do, and I
> wouldn't mind if this sequence:
> 
>   git checkout kvm-next
> git request-pull origin/master 
> 
> behaved the same way as
> 
> git request-pull origin/master  kvm-next

FWIW, that would always be wrong for my scenario.

> But I do not know if the implicit HEAD should DWIM locally to this:
> 
> git request-pull origin/master  for-linus

I guess only Linus could answer that, since he wrote 024d34cb0 and he
knows the intent better than anyone else.

Paolo
--
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 gc removes all packs

2015-02-17 Thread Michael Haggerty
On 02/17/2015 05:55 PM, Jeff King wrote:
> On Tue, Feb 17, 2015 at 05:39:27PM +0100, Michael Haggerty wrote:
> 
>>> You can't symlink refs like this. The loose refs in the filesystem may
>>> be migrated into the "packed-refs" file, at which point your symlink
>>> will be broken. That is a likely reason why git would not find any refs.
>>>
>>> So your setup will not ever work reliably.  But IMHO, it is a bug that
>>> git does not notice the broken symlink and abort an operation which is
>>> computing reachability in order to drop objects. As you noticed, it
>>> means a misconfiguration or filesystem error results in data loss.
>>
>> There's a bunch of code in refs.c that is there explicitly for reading
>> loose references that are symlinks. If the link contents literally start
>> with "refs/", then they are read and treated as a symbolic ref.
>> Otherwise, the symlink is just followed.
> 
> Right, but we should be able to notice that:
> 
>   1. We found a symlink.
> 
>   2. We couldn't read it its ref value (because it's a broken link).
> 
> I think we _do_ notice that at the lowest level, and set REF_ISBROKEN.
> But the problem is that the reachability code in prune and in
> pack-objects (triggered by "repack -ad") uses for_each_ref, and not
> for_each_rawref. So they ignore "broken" refs rather than complaining,
> even though failing to read a ref may mean we could drop objects which
> were only mentioned by that ref.

Yes, this makes sense too. But my point was that sticking symlinks to
random files in your refs hierarchy is pretty questionable even *before*
the symlink gets broken. If we would warn the user as soon as we saw
such a thing, then the user's problem would never have advanced as far
as it did. Do you think that emitting warnings on *intact* symlinks is
too draconian?

> [...]

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Linus Torvalds
On Tue, Feb 17, 2015 at 12:34 PM, Paolo Bonzini  wrote:
>
> I guess only Linus could answer that, since he wrote 024d34cb0 and he
> knows the intent better than anyone else.

I don't even understand your problem.

You said

  "when $3 is not passed git will try to use "HEAD" as the default but
it cannot be resolved to a tag, neither locally (patch 2) nor remotely
(patch 3)"

which makes absolutely no sense.

HEAD is not a tag. Never has been, never will be. If you want me to
pull a tag, then you damn well should say what tag you want, not just
randomly say HEAD.

So what is it you want to do? At no point is "HEAD should resolve as a
tag" sensible.

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


Re: [PATCH 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Paolo Bonzini


On 17/02/2015 21:42, Linus Torvalds wrote:
>   "when $3 is not passed git will try to use "HEAD" as the default but
> it cannot be resolved to a tag, neither locally (patch 2) nor remotely
> (patch 3)"
> 
> which makes absolutely no sense.

Indeed, that's why I wrote patches even though I did find the patches
that you wrote for 2.0.

Without $3, git tries to do things that make no sense like "git show-ref
--heads --tags HEAD"; or that make little sense when requesting a pull,
like looking for HEAD in the output of "git ls-remote".  But from the
release notes of 2.0 it looks like it's intended and the script is just
taking shortcuts.

> HEAD is not a tag. Never has been, never will be. If you want me to
> pull a tag, then you damn well should say what tag you want, not just
> randomly say HEAD.

Ok, in 1.9.x I used to not say anything; if the new workflow is to
always specify a tag, that's okay.

> So what is it you want to do? At no point is "HEAD should resolve as a
> tag" sensible.

I wanted git to find the matching tag on the remote side when I use "git
request-pull origin/master URL" with no third parameter, since I never
request pulls except with a single signed tag.  But I'll adjust my aliases.

Paolo
--
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 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Junio C Hamano
Linus Torvalds  writes:

> HEAD is not a tag. Never has been, never will be. If you want me to
> pull a tag, then you damn well should say what tag you want, not just
> randomly say HEAD.
>
> So what is it you want to do? At no point is "HEAD should resolve as a
> tag" sensible.

"HEAD should resolve as a tag" is not sensible, but "HEAD should
locally DWIM to something sensible" is still possible, no?

We could for example make the rule for unset $3 case like this,
instead of the current "missing $3 is a request to pull HEAD":

If you have one and only one signed tag that happens to point at
the commit sitting at HEAD, behave as if that tag was given as
the third argument from the command line.

Otherwise, if you are on a branch, behave as if that branch was
given as the third argument from the command line.

If you are not on any branch, error 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


Re: [PATCH 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Linus Torvalds
On Tue, Feb 17, 2015 at 12:53 PM, Paolo Bonzini  wrote:
>
> Without $3, git tries to do things that make no sense like "git show-ref
> --heads --tags HEAD"; or that make little sense when requesting a pull,
> like looking for HEAD in the output of "git ls-remote".  But from the
> release notes of 2.0 it looks like it's intended and the script is just
> taking shortcuts.

It is *you* who make no sense.

Looking for HEAD in "git ls-remote"? Perfectly sensible:

[torvalds@i7 linux]$ git ls-remote origin | grep HEAD
cc4f9c2a91b7be7b3590bb1cbe8148873556aa3f HEAD

that's the default thing when you don't specify any particular branch or tag.

> Ok, in 1.9.x I used to not say anything; if the new workflow is to
> always specify a tag, that's okay.

Indeed. You have to specify what you want me to pull. Exactly because
in 1.9.x people didn't, and I got *really* tired of getting bogus pull
requests that didn't work, or pointed at the wrong branch when people
had multiple branches with the same contents etc.

> I wanted git to find the matching tag on the remote side when I use "git
> request-pull origin/master URL" with no third parameter, since I never
> request pulls except with a single signed tag.

The thing is, HEAD works. Not for you, because you don't use HEAD. But
because you don't use HEAD, you shouldn't use the default.

I *would* agree to making $3 be mandatory, but there are still people
out there who just use a single branch per repository and no signed
branches. Which is the only reason that "default HEAD' thing exists.

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


Re: [PATCH 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Paolo Bonzini

> Looking for HEAD in "git ls-remote"? Perfectly sensible:
> 
> [torvalds@i7 linux]$ git ls-remote origin | grep HEAD
> cc4f9c2a91b7be7b3590bb1cbe8148873556aa3f HEAD
> 
> that's the default thing when you don't specify any particular branch or tag.

Sure.  But if I got a pull request saying "please pull
git://example.org/foo.git HEAD" I would think that the sender
messed up the pull request.  So *in the context of git-request-pull*
${remote:-HEAD} makes little sense to me.

But hey, you said it's me who makes no sense.  Maybe I really don't.

> The thing is, HEAD works. Not for you, because you don't use HEAD. But
> because you don't use HEAD, you shouldn't use the default.

Oki.  Will adjust my scripts.  Junio, you may still want to apply patch
1 if only for documentation purposes (the "tag foo" functionality is
unused in the rest of the test).

Paolo
--
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 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Linus Torvalds
On Tue, Feb 17, 2015 at 1:03 PM, Junio C Hamano  wrote:
>
> "HEAD should resolve as a tag" is not sensible, but "HEAD should
> locally DWIM to something sensible" is still possible, no?

I disagree. Why? Because what you have locally is *not* necessarily
the same thing you have remotely.

And that's *exactly* why people used to send me broken pull requests.
"git pull-request" would guess on things, and it would get the guesses
wrong, and write the pull request wrong.

> We could for example make the rule for unset $3 case like this:
> instead of the current "missing $3 is a request to pull HEAD":
>
> If you have one and only one signed tag that happens to point at
> the commit sitting at HEAD, behave as if that tag was given as
> the third argument from the command line.

If you verify that "one and only" to be true both locally and
remotely, then I guess I would be ok with it. But it really would have
to be unique. And truly unique, as in no confusion about branches or
tags, only one or the other. Because the "tag vs branch" was one of
the main sources of confusion that made me repeatedly get bad pull
requests, particularly when there was something locally that wasn't
actually named the same thing remotely.

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


Re: [PATCH 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Linus Torvalds
On Tue, Feb 17, 2015 at 1:10 PM, Paolo Bonzini  wrote:
>
> Sure.  But if I got a pull request saying "please pull
> git://example.org/foo.git HEAD" I would think that the sender
> messed up the pull request.  So *in the context of git-request-pull*
> ${remote:-HEAD} makes little sense to me.

Umm. If somebody actually leaves off the third argument THAT IS NOT AT
ALL what it prints.

It will show

The following changes since commit ...

.. base commit description ..

   are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

for you to fetch changes up to cc4f9c2a91b7be7b3590bb1cbe8148873556aa3f:
...

IOW, it does exactly the right thing. It gives the contents of HEAD,
but it doesn't actually say HEAD anywhere.

And just look at lkml. The above kind of branch-less and tag-less pull
requests are still fairly common. It's the original git model, and it
may be a bit archaic, and I much prefer people to send me signed tags,
but hey, that's what "don't mention a branch or tag" means.

And no, I don't think git request-pull is at all different from other
git commands. "git log" means the same thing as "git log HEAD". Exact
same thing, and nobody would actually write out that HEAD (except
inside scripts, perhaps).

So basically I agree that git request-pull has changed behavior, but
the new behavior is *more* in line with other git commands, and the
old behavior was actually really really odd with that whole extensive
"guess what the user means". No other git command ever did that
guessing thing (ok, famous last words, maybe somebody can come up with
one), and not mentioning a branch/tag/commit explicitly pretty much
always means "HEAD".

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


Re: Git gc removes all packs

2015-02-17 Thread Junio C Hamano
Michael Haggerty  writes:

> On 02/17/2015 05:55 PM, Jeff King wrote:
>> On Tue, Feb 17, 2015 at 05:39:27PM +0100, Michael Haggerty wrote:
>> 
>>> There's a bunch of code in refs.c that is there explicitly for reading
>>> loose references that are symlinks. If the link contents literally start
>>> with "refs/", then they are read and treated as a symbolic ref.
>>> Otherwise, the symlink is just followed.
>> ...
> Yes, this makes sense too. But my point was that sticking symlinks to
> random files in your refs hierarchy is pretty questionable even *before*
> the symlink gets broken. If we would warn the user as soon as we saw
> such a thing, then the user's problem would never have advanced as far
> as it did. Do you think that emitting warnings on *intact* symlinks is
> too draconian?

Do you mean that we would end up reading refs/heads/hold if the user
did this:

git rev-parse --verify HEAD -- >precious
ln -s ../../../precious .git/refs/heads/hold

because that symbolic link does not begin with "refs/", and is an
accident waiting to happen so we should forbid it in the longer
term and warning when we see it would be the first step?
--
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 0/3] Win32: nanosecond-precision file times

2015-02-17 Thread Karsten Blees
Am 16.02.2015 um 23:10 schrieb Junio C Hamano:
> Karsten Blees  writes:
> 
>> However, the Makefile has this to say on the subject:
>>
>> # Define USE_NSEC below if you want git to care about sub-second file mtimes
>> # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and
>> # it will BREAK YOUR LOCAL DIFFS! show-diff and anything using it will likely
>> # randomly break unless your underlying filesystem supports those sub-second
>> # times (my ext3 doesn't).
>>
>> Am I missing something?
> 
[...]
> 
> If you use NSEC, however, and "refresh" grabbed a subsecond time and
> then later "diff-files" learned a truncated/rounded time because the
> filesystem later purged the cached inodes and re-read it from the
> underlying filesystem with no subsecond time resolution, the times
> would not match so you will again see "diff-files" report that "foo"
> is now different.
> 
> That is what the comment you cited is about.
> 

OK, so it all boils down to the "inode cache doesn't round to on-disk
resolution" issue after all, as explained in racy-git.txt.

But then the Makefile comment is quite misleading. Enabling USE_NSEC
will not unconditionally "BREAK YOUR LOCAL DIFFS". Show-diff / diff-files
will also not "break", but may report false positives instead (which may
be worse than failing hard...).

It also seems to me that this is a Linux-only issue which is only remotely
related to the USE_NSEC setting or file systems' timestamp resolutions.

The kernel patch referenced in racy-git.txt only addresses sub-second
resolutions. So even if USE_NSEC is *disabled*, the diff-files issue will
bite you on e.g. FAT32-formatted flash-drives on Linux, at least on
re-mount ("sync && echo 2>/proc/sys/vm/drop_caches" didn't seem to trigger
the rounding, though).

I also suspect that the sub-second rounding function of that patch
(timespec_trunc()) takes some invalid shortcuts - if you configure the
kernel for 300 jiffies per second (i.e. 3,333,333 ns per tick), UDF, NTFS,
SMBFS and CIFS file times will most likely not be properly rounded in the
inode cache. Haven't tested this, though.

So the only file systems with reliable file times on Linux seem to be
those with exactly 1s or 1ns resolution...?

--
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] builtin/push.c: make push_default a static variable

2015-02-17 Thread Junio C Hamano
Jeff King  writes:

>> So we probably would do something similar to @{push} side, which
>> would mean that push_default variable and the logic needs to be
>> visible to remote.c if we want to have the helper that is similar to
>> set_merge() that is used from branch_get() to support @{upstream}.
>
> Sure, we could go that way. But I don't think it changes the issue for
> _this_ patch series, which is that the variable needs visibility outside
> of builtin/push.c (and we need to load the config for programs besides
> git-push).

I do not disagree.  push_default and other things that affect the
computation needs to be visible to the code that implements the
logic.

Do you want to resurrect that @{publish} stuff?  I think it had
sensible semantics, and I do not think we mind keeping the
push_default configuration to be read from the default_config
codepath.

If we decide to go that route, then the series would become
something like this:

$gmane/263871 [1/4] git_push_config: drop cargo-culted wt_status pointer
$gmane/263878 [2/4] cmd_push: set "atomic" bit directly
$gmane/263879 [3/4] cmd_push: pass "flags" pointer to config callback
$gmane/263880 [4/4] push: allow --follow-tags to be set by config 
push.followTags

omitting the original 2/2 patch we are discussing.  I am inclined to
replace what I queued with the above four.

The last one needs a bit of tweaking and should look like the
attached.  Again, as you wrote in $gmane/263880, this is just a
preview. Dave should send the final when he thinks it is good,
possibly with some tests.

-- >8 --
From: Dave Olszewski 
Date: Mon, 16 Feb 2015 01:16:19 -0500
Subject: [PATCH] [NEEDSACK] push: allow --follow-tags to be set by config 
push.followTags

Signed-off-by: Dave Olszewski 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt   |  6 ++
 Documentation/git-push.txt |  5 -
 builtin/push.c | 10 ++
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae6791d..e01d21c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2079,6 +2079,12 @@ new default).
 
 --
 
+push.followTags::
+   If set to true enable '--follow-tags' option by default.  You
+   may override this configuration at time of push by specifying
+   '--no-follow-tags'.
+
+
 rebase.stat::
Whether to show a diffstat of what changed upstream since the last
rebase. False by default.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index ea97576..caa187b 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -128,7 +128,10 @@ already exists on the remote side.
Push all the refs that would be pushed without this option,
and also push annotated tags in `refs/tags` that are missing
from the remote but are pointing at commit-ish that are
-   reachable from the refs being pushed.
+   reachable from the refs being pushed.  This can also be specified
+   with configuration variable 'push.followTags'.  For more
+   information, see 'push.followTags' in linkgit:git-config[1].
+
 
 --signed::
GPG-sign the push request to update refs on the receiving
diff --git a/builtin/push.c b/builtin/push.c
index bba22b8..57c138b 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -473,11 +473,21 @@ static int option_parse_recurse_submodules(const struct 
option *opt,
 
 static int git_push_config(const char *k, const char *v, void *cb)
 {
+   int *flags = cb;
int status;
 
status = git_gpg_config(k, v, NULL);
if (status)
return status;
+
+   if (!strcmp(k, "push.followtags")) {
+   if (git_config_bool(k, v))
+   *flags |= TRANSPORT_PUSH_FOLLOW_TAGS;
+   else
+   *flags &= ~TRANSPORT_PUSH_FOLLOW_TAGS;
+   return 0;
+   }
+
return git_default_config(k, v, NULL);
 }
 
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index c21190d..cffb2b8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2188,6 +2188,7 @@ _git_config ()
pull.octopus
pull.twohead
push.default
+   push.followTags
rebase.autosquash
rebase.stat
receive.autogc
-- 
2.3.0-283-g21bf3f5


--
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 gc removes all packs

2015-02-17 Thread Michael Haggerty
On 02/17/2015 10:57 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> On 02/17/2015 05:55 PM, Jeff King wrote:
>>> On Tue, Feb 17, 2015 at 05:39:27PM +0100, Michael Haggerty wrote:
>>>
 There's a bunch of code in refs.c that is there explicitly for reading
 loose references that are symlinks. If the link contents literally start
 with "refs/", then they are read and treated as a symbolic ref.
 Otherwise, the symlink is just followed.
>>> ...
>> Yes, this makes sense too. But my point was that sticking symlinks to
>> random files in your refs hierarchy is pretty questionable even *before*
>> the symlink gets broken. If we would warn the user as soon as we saw
>> such a thing, then the user's problem would never have advanced as far
>> as it did. Do you think that emitting warnings on *intact* symlinks is
>> too draconian?
> 
> Do you mean that we would end up reading refs/heads/hold if the user
> did this:
> 
> git rev-parse --verify HEAD -- >precious
> ln -s ../../../precious .git/refs/heads/hold
> 
> because that symbolic link does not begin with "refs/",

Correct, you can do exactly that. The "hold" reference is resolvable and
listable using "for-each-ref". But if I try to update it, the contents
of the "precious" file are overwritten. On the other hand, if I run
"pack-refs", then the current value of the "hold" reference is moved to
"packed-refs" and the symlink is removed. This behavior is not sane.

> and is an
> accident waiting to happen so we should forbid it in the longer
> term and warning when we see it would be the first step?

Yes, I am proposing that approach, though if somebody can suggest a use
case I'm willing to be convinced otherwise. The only thing I can imagine
symlinks being useful for might be to temporarily create a fake repo,
run one or two specific known-safe commands, then delete the repo again.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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] submodule: Fix documentation of update subcommand

2015-02-17 Thread Junio C Hamano
Jens Lehmann  writes:

> Yup, but we should also mention '--merge' and '--rebase' here.

This has been sitting in the Stalled pile for quite a while and I am
getting tired of waiting.  How does this look?

-- >8 --
From: Michal Sojka 
Date: Mon, 3 Nov 2014 11:09:51 +0100
Subject: [PATCH] submodule: clarify documentation for update subcommand

e6a1c43a (document submdule.$name.update=none option for gitmodules,
2012-05-10) meant to say "Unlike the case where your .update
configuration is set to either 'rebase' or 'merge', when it is set
to 'none', the tip of the submodule would never move.  You can use
the --checkout option if you want the contents of the submodule to
be updated to some other commit."

But the resulting text made it sound as if using "--checkout" would
have no effect when .update configuration is set to 'rebase' or
'merge', which was misleading.  In fact, with the "--checkout"
option, the tip of the submodule moves to the exact commit that is
recorded in the superproject tree, regardless of .update
configuration.

Signed-off-by: Michal Sojka 
Signed-off-by: Junio C Hamano 
---
 Documentation/git-submodule.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 8e6af65..9bfcdf5 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -158,7 +158,8 @@ update::
checkout the commit specified in the index of the containing repository.
This will make the submodules HEAD be detached unless `--rebase` or
`--merge` is specified or the key `submodule.$name.update` is set to
-   `rebase`, `merge` or `none`. `none` can be overridden by specifying
+   `rebase`, `merge` or `none`. The configuration can be overridden by
+   specifying `--rebase`, `--merge`, or
`--checkout`. Setting the key `submodule.$name.update` to `!command`
will cause `command` to be run. `command` can be any arbitrary shell
command that takes a single argument, namely the sha1 to update to.
-- 
2.3.0-301-g71e72fe
--
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/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)

2015-02-17 Thread Junio C Hamano
Jeff King  writes:

> On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote:
>
>> > Whether or not we decide on a different error-handling convention in the
>> > future, it is a fact of life that a good bit of code already uses the
>> > "strbuf" convention documented by Jonathan's patch. So I think it is OK
>> > to merge it as is. If we change the preferred convention in the future,
>> > one part of the change will be to update this file.
>> 
>> I wasn't sure about "a good bit of code already", but that settles
>> it.  Let's take it as-is and see how the code evolves.
>
> I'm not convinced myself after a quick grep. But it's certainly
> non-zero, and I think I would rather have the technique documented than
> not, so I withdraw my earlier complaints.

OK.
--
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 (Feb 2015, #04; Tue, 17)

2015-02-17 Thread Junio C Hamano
Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

The second batch of topics have been merged to 'master'.  I am
tempted to start discarding topics in the Stalled category that
haven't seen much reviews and discussions on for a long time.

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

* ak/add-i-empty-candidates (2015-01-22) 1 commit
  (merged to 'next' on 2015-02-12 at 0d3cc64)
 + add -i: return from list_and_choose if there is no candidate

 The interactive "show a list and let the user choose from it"
 interface "add -i" used showed and prompted to the user even when
 the candidate list was empty, against which the only "choice" the
 user could have made was to choose nothing.


* jc/apply-ws-fix-expands (2015-01-22) 3 commits
  (merged to 'next' on 2015-02-12 at 9a45b66)
 + apply: count the size of postimage correctly
 + apply: make update_pre_post_images() sanity check the given postlen
 + apply.c: typofix
 (this branch is used by jc/apply-ws-fix-expands-report.)

 "git apply --whitespace=fix" used to under-allocate the memory
 when the fix resulted in a longer text than the original patch.


* jc/diff-format-doc (2015-01-28) 1 commit
  (merged to 'next' on 2015-02-12 at 72a018a)
 + diff-format doc: a score can follow M for rewrite

 The documentation incorrectly said that C(opy) and R(ename) are the
 only ones that can be followed by the score number in the output in
 the --raw format.


* jc/doc-log-rev-list-options (2015-01-23) 1 commit
  (merged to 'next' on 2015-02-12 at 614331f)
 + Documentation: what does "git log --indexed-objects" even mean?

 "git log --help" used to show rev-list options that are irrelevant
 to the "log" command.


* jc/t4122-use-test-write-lines (2015-01-28) 1 commit
  (merged to 'next' on 2015-02-12 at 3ceaae3)
 + t4122: use test_write_lines from test-lib-functions


* jk/dumb-http-idx-fetch-fix (2015-01-27) 1 commit
  (merged to 'next' on 2015-02-12 at 6338345)
 + dumb-http: do not pass NULL path to parse_pack_index

 A broken pack .idx file in the receiving repository prevented the
 dumb http transport from fetching a good copy of it from the other
 side.


* jk/remote-curl-an-array-in-struct-cannot-be-null (2015-01-28) 1 commit
  (merged to 'next' on 2015-02-12 at 669040d)
 + do not check truth value of flex arrays

 Fix a misspelled conditional that is always true.


* jk/status-read-branch-name-fix (2015-01-28) 1 commit
  (merged to 'next' on 2015-02-12 at 1af96a9)
 + read_and_strip_branch: fix typo'd address-of operator

 Code to read branch name from various files in .git/ directory
 would have misbehaved if the code to write them left an empty file.


* ks/rebase-i-abbrev (2015-01-22) 1 commit
  (merged to 'next' on 2015-02-12 at 35c3739)
 + rebase -i: use full object name internally throughout the script

 The insn sheet "git rebase -i" creates did not fully honor
 core.abbrev settings.


* mg/commit-author-no-match-malformed-message (2015-01-26) 1 commit
  (merged to 'next' on 2015-02-12 at 200cd9c)
 + commit: reword --author error message

 The error message from "git commit", when a non-existing author
 name was given as value to the "--author=" parameter, has been
 reworded to avoid misunderstanding.


* mg/push-repo-option-doc (2015-01-28) 1 commit
  (merged to 'next' on 2015-02-12 at 021ec32)
 + git-push.txt: document the behavior of --repo

 The "git push" documentation made the "--repo=" option
 easily misunderstood.


* mh/deref-symref-over-helper-transport (2015-01-21) 1 commit
  (merged to 'next' on 2015-02-12 at de36191)
 + transport-helper: do not request symbolic refs to remote helpers

 "git fetch" over a remote-helper that cannot respond to "list"
 command could not fetch from a symbolic reference e.g. HEAD.

--
[New Topics]

* jc/push-cert (2015-02-12) 1 commit
  (merged to 'next' on 2015-02-16 at f40b3c5)
 + transport-helper: fix typo in error message when --signed is not supported

 "git push --signed" gave an incorrectly worded error message when
 the other side did not support the capability.

 Will merge to 'master'.


* dp/remove-duplicated-header-inclusion (2015-02-13) 1 commit
 - do not include the same header twice

 Code clean-up.

 Will merge to 'next'.


* jc/diff-test-updates (2015-02-15) 6 commits
 - t4008: modernise style
 - t/diff-lib: check exact object names in compare_diff_raw
 - tests: do not borrow from COPYING and README from the real source
 - t4010: correct expected object names
 - t9300: correct expected object names
 - t4008: correct stale comments
 (this branch is used by jc/diff-b-m.)

 Test clean-up.

 Will merge to 'next'.


* jc/send-email-sensible-encoding (2015-02-13) 1 co

Re: [PATCH 0/7] migrate api-strbuf.txt into strbuf.h

2015-02-17 Thread Jonathan Nieder
Jeff King wrote:
> On Thu, Feb 12, 2015 at 03:01:18PM -0800, Junio C Hamano wrote:

>> I am inclined to merge this to 'next', if there is a general
>> understanding that we will try to make the headers _the_ single
>> source of truth of the API by (1) not adding to api-*.txt without
>> describing new things in the headers and (2) moving things from
>> api-*.txt to corresponding headers when clarifying, fixing or
>> updating the API.
>
> I'm fine with that (unsurprisingly), but I would like to hear an "OK"
> from Jonathan before going ahead.

Sorry for the slow reply.  Sounds good to me.  I'd prefer for the
in-between state to last as short a period as possible, but I realize
that preference isn't all that meaningful in the absence of patches. :)

Jonathan
--
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] remote-curl: fall back to Basic auth if Negotiate fails

2015-02-17 Thread Dan Langille (dalangil)
> On Jan 20, 2015, at 7:22 PM, Junio C Hamano  wrote:
> 
> "Dan Langille (dalangil)"  writes:
> 
>> I did not test this patch.  Is that holding up a commit?
> 
> I am hoping that you rebuilt the Git you use with this patch by the
> time you wrote the message I am responding to and have been using it
> for your daily Git needs ;-)
> 
> I believe it is queued on the 'next' branch so that others like you
> who need the change can verify the improvements, and others unlike
> you who do not need the change can make sure the change does not
> cause unintended consequences.

Is this the patch in question?

 https://github.com/git/git/commit/4dbe66464b4fd695c5989cc272fa0edd6475037c

I ask because previous versions of the patch acted against http.h as well and 
my failure with it.

Could I expect that patch work against 2.3.0?

It applies cleanly, compiles, but cores when I try a ‘git clone’.  Unmatched 
2.3.0 succeeds.

Re: [ANNOUNCE] Git Merge, April 8-9, Paris

2015-02-17 Thread Ævar Arnfjörð Bjarmason
On Sat, Jan 24, 2015 at 12:37 AM, Jeff King  wrote:
> GitHub is organizing a Git-related conference to be held April 8-9,
> 2015, in Paris.  Details here:
>
>   http://git-merge.com/
>
> The exact schedule is still being worked out, but there is going to be
> some dedicated time/space for Git (and libgit2 and JGit) developers to
> meet and talk to each other.
>
> If you have patches in Git, I'd encourage you to consider attending. If
> travel finances are a problem, please talk to me. GitHub may be able to
> defray the cost of travel.
>
> I hope to see people there!

I'll be there, excited to be there and meet you all.

I'm even more excited in a way to be traveling from The Netherlands to
Paris to attend conference claiming to be governed by California
law[1] :)

1. Small print at https://ti.to/github-events/git-merge-2015
--
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 GSoC 2015

2015-02-17 Thread Moritz Neeb

Le 12/02/2015 10:34, Jeff King a écrit :

The beginnings of the Google Summer of Code deadlines are upon us again.
Organization applications are due February 20th (next Friday).

  - Do we want to do it?



Unfortunately not so much response on this topic. I was planning to 
apply as a student. Therefore it would be great, if you'd apply. Being 
totally altruistic here, of course.


Moritz
--
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: Multi-threaded 'git clone'

2015-02-17 Thread Junio C Hamano
Martin Fick  writes:

> Sorry for the long winded rant. I suspect that some variation of all
> my suggestions have already been suggested, but maybe they will
> rekindle some older, now useful thoughts, or inspire some new ones.
> And maybe some of these are better to pursue then more parallelism?

We avoid doing a grand design document without having some prototype
implementation, but I think the limitation of the current protocol
has become apparent enough that we should do something about it, and
we should do it in a way that different implementations of Git can
all implement.

I think "multi-threaded clone" is a wrong title for this discussion,
in that the user does not care if it is done by multi-threading the
current logic or in any other way.  The user just wants a faster
clone.

In addition, the current "fetch" protocol has the following problems
that limit us:

 - It is not easy to make it resumable, because we recompute every
   time.  This is especially problematic for the initial fetch aka
   "clone" as we will be talking about a large transfer [*1*].

 - The protocol extension has a fairly low length limit [*2*].

 - Because the protocol exchange starts by the server side
   advertising all its refs, even when the fetcher is interested in
   a single ref, the initial overhead is nontrivial, especially when
   you are doing a small incremental update.  The worst case is an
   auto-builder that polls every five minutes, even when there is no
   new commits to be fetched [*3*].

 - Because we recompute every time, taking into account of what the
   fetcher has, in addition to what the fetcher obtained earlier
   from us in order to reduce the transferred bytes, the payload for
   incremental updates become tailor-made for each fetch and cannot
   be easily reused [*4*].

I'd like to see a new protocol that lets us overcome the above
limitations (did I miss others? I am sure people can help here)
sometime this year.



[Footnotes]

*1* The "first fetch this bundle from elsewhere and then come back
here for incremental updates" raised earlier in this thread may
be a way to alleviate this, as the large bundle can be served
from a static file.

*2* An earlier "this symbolic ref points at that concrete ref"
attempt failed because of this and we only talk about HEAD.

*3* A new "fetch" protocol must avoid this "one side blindly gives a
large message as the first thing".  I have been toying with the
idea of making the fetcher talk first, by declaring "I am
interested in your refs that match refs/heads/* or refs/tags/*,
and I have a superset of objects that are reachable from the
set of refs' values X you gave me earlier", where X is a small
token generated by hashing the output from "git ls-remote $there
refs/heads/* refs/tags/*".  In the best case where the server
understands what X is and has a cached pack data, it can then
send:

- differences in the refs that match the wildcards (e.g. "Back
  then at X I did not have refs/heads/next but now I do and it
  points at this commit.  My refs/heads/master is now at that
  commit.  I no longer have refs/heads/pu.  Everything else in
  the refs/ hierarchy you are interested in is the same as state
  X").

- The new name of the state Y (again, the hashed value of the
  output from "git ls-remote $there refs/heads/* refs/tags/*")
  to make sure the above differences can be verified at the
  receiving end.

- the cached pack data that contains all necessary objects
  between X and Y.

Note that the above would work if and only if we accept that it
is OK to send objects between the remote tracking branches the
fetcher has (i.e. the objects it last fetched from the server)
and the current tips of branches the server has, without
optimizing by taking into account that some commits in that set
may have already been obtained by the fetcher from a
third-party.

If the server does not recognize state X (after all it is just a
SHA-1 hash value, so the server cannot recreate the set of refs
and their values from it unless it remembers), the exchange
would have to degenerate to the traditional transfer.

The server would want to recognize the result of hashing an
empty string, though.  The fetcher is saying "I have nothing"
in that case.


*4* The scheme in *3* can be extended to bring the fetcher
step-wise.  If the server's state was X when the fetcher last
contacted it, and since then the server received multiple pushes
and has two snapshots of states, Y and Z, then the exchange may
go like this:

fetcher: I am interested in refs/heads/* and refs/tags/* and I
 have your state X.

server:  Here is the incremental difference to the refs and the
 end result should hash to Y.  Here comes the pack data
 to bring you up to date.

fetcher: (after receiving, un

Re: [PATCH v3] remote-curl: fall back to Basic auth if Negotiate fails

2015-02-17 Thread Junio C Hamano
"Dan Langille (dalangil)"  writes:

>> On Jan 20, 2015, at 7:22 PM, Junio C Hamano  wrote:
>> 
>> "Dan Langille (dalangil)"  writes:
>> 
>>> I did not test this patch.  Is that holding up a commit?
>> 
>> I am hoping that you rebuilt the Git you use with this patch by the
>> time you wrote the message I am responding to and have been using it
>> for your daily Git needs ;-)
>> 
>> I believe it is queued on the 'next' branch so that others like you
>> who need the change can verify the improvements, and others unlike
>> you who do not need the change can make sure the change does not
>> cause unintended consequences.
>
> Is this the patch in question?
>
>  https://github.com/git/git/commit/4dbe66464b4fd695c5989cc272fa0edd6475037c
>
> I ask because previous versions of the patch acted against http.h as
> well and my failure with it.
>
> Could I expect that patch work against 2.3.0?
>
> It applies cleanly, compiles, but cores when I try a ‘git clone’.
> Unmatched 2.3.0 succeeds.

It already is in 'master', so please holler if things break with
that version.

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: Git GSoC 2015

2015-02-17 Thread Stefan Beller
On Thu, Feb 12, 2015 at 1:34 AM, Jeff King  wrote:
> The beginnings of the Google Summer of Code deadlines are upon us again.
> Organization applications are due February 20th (next Friday).
>
>  - Do we want to do it?
>
>  - Who would like to volunteer to be the org admin?
>
>I would like for it not to be me again, but I can help walk anyone
>through the application process (which largely just pulls from the
>prior year's application).
>
>  - Any thoughts on procedures or lessons learned from last year?
>
>Personally, I really liked the "micro-project" system from last year.
>It made it easy to drop spammy candidates (because they didn't bother
>to do a micro-project at all), and it gave us some interaction with
>the candidates before we had to make a decision.
>
> -Peff


I have observed the GSoC last year and the micro projects seem to be
have helped a lot. Although I don't have a strong proficiency yet, I'd
may be a mentor this year?

Why do you not like to be the org admin again this year?

Thanks,
Stefan
--
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: Multi-threaded 'git clone'

2015-02-17 Thread Junio C Hamano
On Tue, Feb 17, 2015 at 3:32 PM, Junio C Hamano  wrote:

A few typofixes and clarifications.

> *4* The scheme in *3* can be extended to bring the fetcher
> step-wise.  If the server's state was X when the fetcher last

"bring the fetcher up-to-date step-wise", or "update the fetcher step-wise".

> contacted it, and since then the server received multiple pushes
> and has two snapshots of states, Y and Z, then the exchange may
> go like this:
>
> fetcher: I am interested in refs/heads/* and refs/tags/* and I
>  have your state X.
>
> server:  Here is the incremental difference to the refs and the
>  end result should hash to Y.  Here comes the pack data
>  to bring you up to date.
>
> fetcher: (after receiving, unpacking and updating the
>  remote-tracking refs) Thanks.  Do you have more?
>
> server:  Yes, here is the incremental difference to the refs and the
>  end result should hash to Z.  Here comes the pack data
>  to bring you up to date.
>
> fetcher: (after receiving, unpacking and updating the
>  remote-tracking refs) Thanks.  Do you have more?
>
> server:  No, you are now fully up to date with me.  Bye.

The initial part of this exchange may go like this, if the state the
fetcher grabbed the last time from this server is even older than X:

  fetcher: I am interested in refs/heads/* and refs/tags/* and I have
your state W (or "I know I am too old that I do not know what
you call that state").

  server: Sorry, I do not know what W is. Please be more specific.

  fetcher: Here are the refs and their objects: refs/heads/master points
at commit A, refs/heads/maint points at commit B, ...

  server: (goes and checks and finds out that fetcher is behind X).
OK, I'll compute a custom pack to bring you up to date with
one of my states. Here is the incremental difference to the refs,
and the end result should hash to X. Here comes the pack data.

  fetcher: (after receiving, unpacking and updating the
remote-tracking refs) Thanks. Do you have more?

After that, the server would update this client to state Y and then
state Z as above.

Needless to say, this would naturally extend to a case where you
follow only a single branch (you would say "I am interested in your
refs/heads/dev" with a wildcard that matches exactly that branch).
Of course, depending on the access pattern by common project
participants, the server side may choose what set of refs to prepare
such snapshots and uncommon requests may always be served by
the traditional object enumeration codepath.
--
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 0/3] request-pull: do something if $3 is passed

2015-02-17 Thread Junio C Hamano
Linus Torvalds  writes:

> So basically I agree that git request-pull has changed behavior, but
> the new behavior is *more* in line with other git commands, and the
> old behavior was actually really really odd with that whole extensive
> "guess what the user means". No other git command ever did that
> guessing thing (ok, famous last words, maybe somebody can come up with
> one), and not mentioning a branch/tag/commit explicitly pretty much
> always means "HEAD".

OK.

There may be some stuff that DWIMs "HEAD" to something other than
the commit that is at the tip of HEAD, but I agree that the fewer we
have such oddballs, the better.

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: Git gc removes all packs

2015-02-17 Thread Junio C Hamano
Michael Haggerty  writes:

> On 02/17/2015 10:57 PM, Junio C Hamano wrote:
> ...
>> Do you mean that we would end up reading refs/heads/hold if the user
>> did this:
>> 
>> git rev-parse --verify HEAD -- >precious
>> ln -s ../../../precious .git/refs/heads/hold
>> 
>> because that symbolic link does not begin with "refs/",
>
> Correct, you can do exactly that. The "hold" reference is resolvable and
> listable using "for-each-ref". But if I try to update it, the contents
> of the "precious" file are overwritten. On the other hand, if I run
> "pack-refs", then the current value of the "hold" reference is moved to
> "packed-refs" and the symlink is removed. This behavior is not sane.
>
>> and is an
>> accident waiting to happen so we should forbid it in the longer
>> term and warning when we see it would be the first step?
>
> Yes, I am proposing that approach, though if somebody can suggest a use
> case I'm willing to be convinced otherwise.

Thanks.  I agree the proposed tightening is probably harmless, but I
too would want to see if somebody comes up with a valid use case.  I
do not think of anything offhand.

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