date:20150217

Jeff King p...@peff.net 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)

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=there 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 commit
 - send-email: ask

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 gits...@pobox.com wrote:
 
 Dan Langille (dalangil) dalan...@cisco.com 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: Multi-threaded 'git clone'

Martin Fick mf...@codeaurora.org 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, unpacking

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

Dan Langille (dalangil) dalan...@cisco.com writes:

 On Jan 20, 2015, at 7:22 PM, Junio C Hamano gits...@pobox.com wrote:
 
 Dan Langille (dalangil) dalan...@cisco.com 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: Multi-threaded 'git clone'

On Tue, Feb 17, 2015 at 3:32 PM, Junio C Hamano gits...@pobox.com 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] submodule: Fix documentation of update subcommand

Jens Lehmann jens.lehm...@web.de 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 sojk...@fel.cvut.cz
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 sojk...@fel.cvut.cz
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 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: Git GSoC 2015

2015-02-17 Thread Stefan Beller

On Thu, Feb 12, 2015 at 1:34 AM, Jeff King p...@peff.net 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: Git gc removes all packs

Michael Haggerty mhag...@alum.mit.edu 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

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

Linus Torvalds torva...@linux-foundation.org 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

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

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

There is no reason to reserve a gap between the public and private
flags values.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 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

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

Matthieu Moy matthieu@grenoble-inp.fr 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: Git gc removes all packs

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

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

Creating a reference requires a new_sha1 that is not NULL and not
null_sha1.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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

Re: [PATCH v2 03/12] struct ref_update: move have_old into flags

Michael Haggerty mhag...@alum.mit.edu 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)

On 02/13/2015 12:08 AM, Junio C Hamano wrote:
 Junio C Hamano gits...@pobox.com writes:
 
 Jeff King p...@peff.net 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)

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

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

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

It makes no sense to delete a reference that is already known not to
exist.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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 06/13] ref_transaction_delete(): remove have_old parameter

Instead, verify the reference's old value if and only if old_sha1 is
non-NULL.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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

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

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 mhag...@alum.mit.edu
---
 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 04/13] struct ref_update: move have_old into flags

Instead of having a separate have_old field, record this boolean value
as a bit in the flags field.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 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 07/13] commit: add tests of commit races

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 mhag...@alum.mit.edu
---
 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

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 mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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

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 matthieu@grenoble-inp.fr 
 wrote:
 
 Fairuzan Roslan fairuzan.ros...@gmail.com 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

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 mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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

It is only used internally now. Document it a little bit better, too.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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

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 mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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

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 gits...@pobox.com
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 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)
 {

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

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

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 mhag...@alum.mit.edu
Reviewed-by: Stefan Beller sbel...@google.com
---
 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];

Re: [PATCH v2 06/12] commit: add tests of commit races

On 02/12/2015 07:13 PM, Stefan Beller wrote:
 On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty mhag...@alum.mit.edu 
 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

On 02/12/2015 08:36 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu 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

On 02/12/2015 08:15 PM, Junio C Hamano wrote:
 Stefan Beller sbel...@google.com writes:
 
 On Thu, Feb 12, 2015 at 3:12 AM, Michael Haggerty mhag...@alum.mit.edu 
 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: 'make test' fails in pu

Jeff King p...@peff.net 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



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 url
 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 url
 
 behaved the same way as
 
 git request-pull origin/master url 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 url 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

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

Jeff King p...@peff.net 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

[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 sbel...@google.com
---
 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: 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 matthieu@grenoble-inp.fr 
 wrote:

 Fairuzan Roslan fairuzan.ros...@gmail.com 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

[no subject]

2015-02-17 Thread Stefan Beller


On Tue, Feb 17, 2015 at 10:14 AM, Junio C Hamano gits...@pobox.com wrote:
 Stefan Beller sbel...@google.com 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 sbel...@google.com
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 sbel...@google.com
---
 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 p...@peff.net 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] read-cache.c: free cache entry when refreshing fails

Stefan Beller sbel...@google.com 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 sbel...@google.com
 ---
  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

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

 Jeff King p...@peff.net 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

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



On 16/02/2015 20:47, Junio C Hamano wrote:
 Paolo Bonzini bonz...@gnu.org writes:
 
 From: Paolo Bonzini pbonz...@redhat.com

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

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

Paolo Bonzini bonz...@gnu.org writes:

 On 16/02/2015 20:47, Junio C Hamano wrote:
 Paolo Bonzini bonz...@gnu.org writes:
 
 From: Paolo Bonzini pbonz...@redhat.com

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

 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 url kvm-next:next tags/for-linus
git request-pull origin/master url 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 url
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 url

behaved the same way as

git request-pull origin/master url kvm-next

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

git request-pull origin/master url 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 v3 00/13] Allow reference values to be checked in a transaction

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

On Tue, Feb 17, 2015 at 12:34 PM, Paolo Bonzini bonz...@gnu.org 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



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

Linus Torvalds torva...@linux-foundation.org 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

On Tue, Feb 17, 2015 at 12:53 PM, Paolo Bonzini bonz...@gnu.org 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


 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

On Tue, Feb 17, 2015 at 1:03 PM, Junio C Hamano gits...@pobox.com 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: Git gc removes all packs

Michael Haggerty mhag...@alum.mit.edu 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] request-pull: do something if $3 is passed

On Tue, Feb 17, 2015 at 1:10 PM, Paolo Bonzini pbonz...@redhat.com 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...

.. 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: [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 karsten.bl...@gmail.com 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

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

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

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

2015-02-17 Thread Matthieu Moy

Fairuzan Roslan fairuzan.ros...@gmail.com 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: [PATCH 2/2] builtin/push.c: make push_default a static variable

Jeff King p...@peff.net 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 cx...@pobox.com
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 cx...@pobox.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 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

On 02/17/2015 10:57 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu 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 2/2] builtin/push.c: make push_default a static variable

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

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

On Tue, Feb 17, 2015 at 05:36:30PM +0700, Duy Nguyen wrote:

 On Tue, Feb 17, 2015 at 5:13 PM, Jeff King p...@peff.net wrote:
  If the only reason is for gdb, then perhaps:
 
set args pack-objects --stdout /dev/null /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 30
  exec 41
  gdb /dev/tty /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: Lift --stdout restriction for using reachability bitmap in pack-objects?