Git development
 help / color / mirror / Atom feed
* Re: [PATCH v2] all: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-23 18:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwqv3hlu7.fsf@alter.siamese.dyndns.org>

On Wed, Jan 23, 2013 at 6:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> But I still do not think this loop is correct.  In a repository that
>> has a working tree, you would learn that directory $D has $D/.git in
>> it, feed $D to handle_repo(), and then descend into $D/.git/objects/,
>> $D/.git/refs, and other random directories to see if you can find
>> other repositories....
>
> Ahh, no, you don't.
>
> I still think calling is_git_directory() on $D + "/.git" would be a
> better implementation, though.

Except for the .gitfile case, which is_git_directory() doesn't seem to
handle. I guess I can invoke read_gitfile() when i see that .git is a
file.

-- 
larsh

^ permalink raw reply

* Re: [PATCH v2] all: new command used for multi-repo operations
From: Lars Hjemli @ 2013-01-23 18:19 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v622nj0ys.fsf@alter.siamese.dyndns.org>

On Wed, Jan 23, 2013 at 5:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
>> +static int walk(struct strbuf *path, int argc, const char **argv)
>> +{
>> +     DIR *dir;
>> +     struct dirent *ent;
>> +     struct stat st;
>> +     size_t len;
>> +
>> +     dir = opendir(path->buf);
>> +     if (!dir)
>> +             return errno;
>> +     strbuf_addstr(path, "/");
>> +     len = path->len;
>> +     while ((ent = readdir(dir))) {
>> +             if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
>> +                     continue;
>> +             if (!strcmp(ent->d_name, ".git")) {
>> +                     strbuf_addstr(path, ent->d_name);
>> +                     setenv(GIT_DIR_ENVIRONMENT, path->buf, 1);
>> +                     strbuf_setlen(path, len - 1);
>> +                     setenv(GIT_WORK_TREE_ENVIRONMENT, path->buf, 1);
>> +                     handle_repo(path->buf, argv);
>> +                     strbuf_addstr(path, "/");
>> +                     continue;
>> +             }
>> +             strbuf_setlen(path, len);
>> +             strbuf_addstr(path, ent->d_name);
>> +             switch (DTYPE(ent)) {
>> +             case DT_UNKNOWN:
>> +                     /* Use stat() instead of lstat(), since we want to
>> +                      * know if we can follow this path into another
>> +                      * directory - it's  not important if it's actually
>> +                      * a symlink which gets us there.
>> +                      */
>
> This is wrong if you are on a platform that does have d_type, no?
> It may say it is a symbolic link, and until you stat you wouldn't
> know if it may lead to a directory.  You can add "case DT_LNK:" that
> behaves the same as DT_UNKNOWN, I think.

Yeah, that seems right, thanks.

-- 
larsh

^ permalink raw reply

* Defensive publication on Git replication
From: Don Marti @ 2013-01-23 18:00 UTC (permalink / raw)
  To: git
In-Reply-To: <a3951c54-efb8-476f-952f-4893bcc5e4a9@caret.perforce.com>

Just wanted to get this simple scheme out there in the hope of minimizing
patent troll risks for people working on replication.

You run an update hook that blocks pushes unless the branch reference in
the repository matches the corresponding reference stored in a synchronized
system, and flood the objects out using an inexpensive, simple system.

(Apologies if this is too obvious for the list; I didn't want to take
chances with the patent office definition of "obvious.")

 Replication Mechanism for a Distributed Version Control System 
 http://ip.com/IPCOM/000225058

 Disclosed is a mechanism for replication of content repositories of a
 distributed version control system (DVCS), which is unique in that objects
 and branch references are transferred between nodes by independent means,
 with only branch references subject to synchronization among nodes. The
 object of this disclosure is to simplify deployment of a DVCS in a
 replicated configuration. Replication can be performed efficiently by
 transferring objects using a key-value store, such as a distributed hash
 table (DHT), independently of a repository's branch references, which are
 transferred using a synchronized data store.

-- 
Don Marti           Perforce Software          +1-510-473-3142
http://www.perforce.com/git-fusion

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Jonathan Nieder @ 2013-01-23 17:23 UTC (permalink / raw)
  To: Thomas Rast
  Cc: kusmabite, Thomas Rast, Eric Chamberland, Brian J. Murrell, git,
	Pyeron, Jason J CTR (US), Maxime Boissonneault, Philippe Vaucher,
	Sébastien Boisvert
In-Reply-To: <871udbc3af.fsf@pctrast.inf.ethz.ch>

Thomas Rast wrote:

> Taken together this should mean that the bug is in fact simply that the
> calls do not *restart*.  They are (like you say) allowed to return EINTR
> despite not being specified to, *but* SA_RESTART should restart it.
>
> Now, does that make it a lustre bug or a glibc bug? :-)

The kernel takes care of SA_RESTART, if I remember correctly.  (See
arch/x86/kernel/signal.c::handle_signal() case -ERESTARTSYS.)

^ permalink raw reply

* Re: What's cooking in git.git (Jan 2013, #08; Tue, 22)
From: Junio C Hamano @ 2013-01-23 17:13 UTC (permalink / raw)
  To: John Keeping; +Cc: git, Eric S. Raymond, Chris Rorvick
In-Reply-To: <20130123092858.GJ7498@serenity.lan>

John Keeping <john@keeping.me.uk> writes:

> My preference would be for something like this, possibly with an
> expanded examples section showing how to pipe the output of cvsps-3 or
> cvs2git into git-fast-import:
>
> -- >8 --
>
> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
> index 9d5353e..20b846e 100644
> --- a/Documentation/git-cvsimport.txt
> +++ b/Documentation/git-cvsimport.txt
> @@ -18,6 +18,11 @@ SYNOPSIS
>  
>  DESCRIPTION
>  -----------
> +*WARNING:* `git cvsimport` uses cvsps version 2, which is considered
> +deprecated; it does not work with cvsps version 3 and later.  If you are
> +performing a one-shot import of a CVS repository consider using cvsps-3,
> +cvs2git or parsecvs directly.
> +
>  Imports a CVS repository into git. It will either create a new
>  repository, or incrementally import into an existing one.
>  
> -- 8< --

OK, that is certainly a lot simpler to explain.

Is it "it does not work yet with cvsps3", or "it will not ever work
with cvsps3"?  The impression I am getting is that it is the latter.

Also, should we have a suggestion to people who are *not* performing
a one-shot import, i.e. doing incremental or bidirectional?

^ permalink raw reply

* Re: [PATCH v2] all: new command used for multi-repo operations
From: Junio C Hamano @ 2013-01-23 17:04 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: git
In-Reply-To: <7v622nj0ys.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> But I still do not think this loop is correct.  In a repository that
> has a working tree, you would learn that directory $D has $D/.git in
> it, feed $D to handle_repo(), and then descend into $D/.git/objects/,
> $D/.git/refs, and other random directories to see if you can find
> other repositories....

Ahh, no, you don't.

I still think calling is_git_directory() on $D + "/.git" would be a
better implementation, though.

^ permalink raw reply

* Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
From: Junio C Hamano @ 2013-01-23 17:01 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Jens Lehmann
In-Reply-To: <1358948067-2792-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> add_submodule_odb() can be used to import objects from another
> repository temporarily. After this point we don't know which objects
> are ours, which are external. If we create an object that refers to an
> external object, next time git runs, it may find a hole in the object
> graph because the external repository may not be imported. The same
> goes for pointing a ref to an external SHA-1.
>
> To protect ourselves, once add_submodule_odb() is used:
>
>  - trees, tags and commits cannot be created
>  - refs cannot be updated
>
> In certain cases that submodule code knows that it's safe to write, it
> can turn the readonly flag off.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  I think this is a good safety check.

Two step implementation to bring "read-only" support into a testable
shape and then flip that bit in add_submdule_odb() would be a
sensible approach.

I however have this suspicion that this will become a losing battle
and we would be better off getting rid of add_submodule_odb();
instead operations that work across repositories will be done as a
subprocess, which will get us back closer to one of the original
design goals of submodule support to have a clear separation between
the superproject and its submodules.

^ permalink raw reply

* Re: [PATCH v2] all: new command used for multi-repo operations
From: Junio C Hamano @ 2013-01-23 16:52 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: git
In-Reply-To: <1358928767-16283-1-git-send-email-hjemli@gmail.com>

Lars Hjemli <hjemli@gmail.com> writes:

> +static int walk(struct strbuf *path, int argc, const char **argv)
> +{
> +	DIR *dir;
> +	struct dirent *ent;
> +	struct stat st;
> +	size_t len;
> +
> +	dir = opendir(path->buf);
> +	if (!dir)
> +		return errno;
> +	strbuf_addstr(path, "/");
> +	len = path->len;
> +	while ((ent = readdir(dir))) {
> +		if (!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, ".."))
> +			continue;
> +		if (!strcmp(ent->d_name, ".git")) {
> +			strbuf_addstr(path, ent->d_name);
> +			setenv(GIT_DIR_ENVIRONMENT, path->buf, 1);
> +			strbuf_setlen(path, len - 1);
> +			setenv(GIT_WORK_TREE_ENVIRONMENT, path->buf, 1);
> +			handle_repo(path->buf, argv);
> +			strbuf_addstr(path, "/");
> +			continue;
> +		}
> +		strbuf_setlen(path, len);
> +		strbuf_addstr(path, ent->d_name);
> +		switch (DTYPE(ent)) {
> +		case DT_UNKNOWN:
> +			/* Use stat() instead of lstat(), since we want to
> +			 * know if we can follow this path into another
> +			 * directory - it's  not important if it's actually
> +			 * a symlink which gets us there.
> +			 */

This is wrong if you are on a platform that does have d_type, no?
It may say it is a symbolic link, and until you stat you wouldn't
know if it may lead to a directory.  You can add "case DT_LNK:" that
behaves the same as DT_UNKNOWN, I think.

> +			if (stat(path->buf, &st) || !S_ISDIR(st.st_mode))
> +				break;
> +			/* fallthrough */
> +		case DT_DIR:
> +			walk(path, argc, argv);
> +			break;
> +		}
> +		strbuf_setlen(path, len);
> +	}

But I still do not think this loop is correct.  In a repository that
has a working tree, you would learn that directory $D has $D/.git in
it, feed $D to handle_repo(), and then descend into $D/.git/objects/,
$D/.git/refs, and other random directories to see if you can find
other repositories.  That is just not right.

If this check were doing something like "The directory $D is worth
handing to handle_repo() if it has all of the following: objects/,
refs/ and HEAD that either points inside refs/ or 40-hex.", then it
would make a lot more sense to me, including the part that goes on
to check sibling directories.  As a bonus side effect, it will give
you a support for bare repositories for free.

^ permalink raw reply

* Re: Bug in latest gitk - can't click lines connecting commits
From: Junio C Hamano @ 2013-01-23 16:35 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: git
In-Reply-To: <20130123114806.GA21124@iris.ozlabs.ibm.com>

Paul Mackerras <paulus@samba.org> writes:

> On Tue, Jan 22, 2013 at 09:28:23AM -0800, Junio C Hamano wrote:
>> 
>> I notice that I have a handful of commits that I haven't pulled from
>> your repository, and the last commit on your 'master' is about 20
>> days old.  Is it safe for me to pull these now?
>
> Yes, please pull them now.

Thanks.

^ permalink raw reply

* Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-23 16:28 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Chris Rorvick
In-Reply-To: <20130123065640.GB10306@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:
>
>> When we push to update an existing ref, if:
>> 
>>  * we do not have the object at the tip of the remote; or
>>  * the object at the tip of the remote is not a commit; or
>>  * the object we are pushing is not a commit,
>> 
>> there is no point suggesting to fetch, integrate and push again.
>> 
>> If we do not have the current object at the tip of the remote, we
>> should tell the user to fetch first and evaluate the situation
>> before deciding what to do next.
>
> Should we? I know that it is more correct to do so, because we do not
> even know for sure that the remote object is a commit, and fetching
> _might_ lead to us saying "hey, this is not something that can be
> fast-forwarded".
>
> But by far the common case will be that it _is_ a commit, and the right
> thing is going to be to pull....
> Is the extra hassle in the common case worth it for the off chance that
> we might give a more accurate message? Should the "fetch first" message
> be some hybrid that covers both cases accurately, but still points the
> user towards "git pull" (which will fail anyway if the remote ref is not
> a commit)?

I was actually much less happy with "needs force" than this one, as
you have to assume too many things for the message to be a useful
and a safe advise: the user has actually examined the situation and
forcing the push is the right thing to do.  Both old and new objects
exist, so the user _could_ have done so, but did he really check
them, thought about the situation and made the right decision?
Perhaps the attempted push had a typo in the object name it wanted
to update the other end with, and the right thing to do is not to
force but to fix the refspec instead?  "You need --force to perform
this push" was a very counter-productive advice in this case, but I
didn't think of a better wording.

The "fetch first and inspect" was an attempt to reduce the risk of
that "needs force" message that could encourage brainless forced
pushes.  Perhaps if we reword "needs force" to something less risky,
we do not have to be so explicit in "You have to fetch first and
examine".

How about doing this?

For "needs force" cases, we say this instead:

 hint: you cannot update a ref that points at a non-commit object, or
 hint: update a ref to point at a non-commit object, without --force.

Being explicit about "non-commit" twice will catch user's eyes and
cause him to double check that it is not a mistyped LHS of the push
refspec (if he is sending a non-commit) or mistyped RHS (if the ref
is pointing at a non-commit).  If he _is_ trying to push a blob out,
the advice makes it clear what to do next: he does want to force it.

If we did that, then we could loosen the "You should fetch first"
case to say something like this:

 hint: you do not have the object at the tip of the remote ref;
 hint: perhaps you want to pull from there first?

This explicitly denies one of Chris's wish "we shouldn't suggest to
merge something that we may not be able to", but in the "You should
fetch first" case, we cannot fundamentally know if we can merge
until we fetch.  I agree with you that the most common case is that
the unknown object is a commit, and that suggesting to pull is a
good compromise.

Note that you _could_ split the "needs force" case into two, namely,
"cannot replace a non-commit" and "cannot push a non-commit".  You
could even further split them into combinations (e.g. an attempt to
replace an annotated tag with a commit and an attempt to replace a
tree with a commit may be different situations), but I think the
advices we can give to these cases would end up being the same, so I
tend to think it is not worth it.  That is what I meant by "I do not
expect me doing the type-based policy myself" in the concluding
message of the series.

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Erik Faye-Lund @ 2013-01-23 15:54 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Thomas Rast, Eric Chamberland, Brian J. Murrell, git,
	Pyeron, Jason J CTR (US), Maxime Boissonneault, Philippe Vaucher,
	Sébastien Boisvert
In-Reply-To: <871udbc3af.fsf@pctrast.inf.ethz.ch>

On Wed, Jan 23, 2013 at 4:44 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> On Wed, Jan 23, 2013 at 4:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>>
>>>> POSIX allows error codes
>>>> to be generated other than those defined. From
>>>> http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html:
>>>>
>>>> "Implementations may support additional errors not included in this
>>>> list, *may generate errors included in this list under circumstances
>>>> other than those described here*, or may contain extensions or
>>>> limitations that prevent some errors from occurring."
>>>
>>> That same page says, however:
>>>
>>>   For functions under the Threads option for which [EINTR] is not listed
>>>   as a possible error condition in this volume of IEEE Std 1003.1-2001,
>>>   an implementation shall not return an error code of [EINTR].
>>
>> Yes, but surely that's for pthreads functions, no? utime is not one of
>> those functions...
>
> Ah, my bad.  In fact in
>
>   http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
>
> there is a paragraph "Signal Effects on Other Functions", which says
>
> <snip>
>
> Taken together this should mean that the bug is in fact simply that the
> calls do not *restart*.  They are (like you say) allowed to return EINTR
> despite not being specified to, *but* SA_RESTART should restart it.
>

Right, thanks for clearing that up.

> Now, does that make it a lustre bug or a glibc bug? :-)

That's kind of uninteresting, the important bit is that it is indeed a
bug (outside of Git).

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Thomas Rast @ 2013-01-23 15:44 UTC (permalink / raw)
  To: kusmabite
  Cc: Thomas Rast, Eric Chamberland, Brian J. Murrell, git,
	Pyeron, Jason J CTR (US), Maxime Boissonneault, Philippe Vaucher,
	Sébastien Boisvert
In-Reply-To: <CABPQNSb89h28O_a3uVoVrNisZqPcHHVFm8nP7GdFGCb=PVdcsQ@mail.gmail.com>

Erik Faye-Lund <kusmabite@gmail.com> writes:

> On Wed, Jan 23, 2013 at 4:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>> POSIX allows error codes
>>> to be generated other than those defined. From
>>> http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html:
>>>
>>> "Implementations may support additional errors not included in this
>>> list, *may generate errors included in this list under circumstances
>>> other than those described here*, or may contain extensions or
>>> limitations that prevent some errors from occurring."
>>
>> That same page says, however:
>>
>>   For functions under the Threads option for which [EINTR] is not listed
>>   as a possible error condition in this volume of IEEE Std 1003.1-2001,
>>   an implementation shall not return an error code of [EINTR].
>
> Yes, but surely that's for pthreads functions, no? utime is not one of
> those functions...

Ah, my bad.  In fact in

  http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html

there is a paragraph "Signal Effects on Other Functions", which says

  The most common behavior of an interrupted function after a
  signal-catching function returns is for the interrupted function to
  give an [EINTR] error unless the SA_RESTART flag is in effect for the
  signal. However, there are a number of specific exceptions, including
  sleep() and certain situations with read() and write().

  The historical implementations of many functions defined by IEEE Std
  1003.1-2001 are not interruptible[...]

  Functions not mentioned explicitly as interruptible may be so on some
  implementations, possibly as an extension where the function gives an
  [EINTR] error. There are several functions (for example, getpid(),
  getuid()) that are specified as never returning an error, which can
  thus never be extended in this way.

  If a signal-catching function returns while the SA_RESTART flag is in
  effect, an interrupted function is restarted at the point it was
  interrupted. Conforming applications cannot make assumptions about the
  internal behavior of interrupted functions, even if the functions are
  async-signal-safe. For example, suppose the read() function is
  interrupted with SA_RESTART in effect, the signal-catching function
  closes the file descriptor being read from and returns, and the read()
  function is then restarted; in this case the application cannot assume
  that the read() function will give an [EBADF] error, since read()
  might have checked the file descriptor for validity before being
  interrupted.

Taken together this should mean that the bug is in fact simply that the
calls do not *restart*.  They are (like you say) allowed to return EINTR
despite not being specified to, *but* SA_RESTART should restart it.

Now, does that make it a lustre bug or a glibc bug? :-)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* AW: Moving commits from one branch to another
From: Schulze, Stefan @ 2013-01-23 15:44 UTC (permalink / raw)
  To: git@vger.kernel.org
In-Reply-To: <20130123144941.GO7498@serenity.lan>

> git rebase --onto svnbranch publishedToSvn master^0
> git checkout -B svnbranch HEAD

Great! This does exactly what I want!

Thanks for your support,
  Stefan Schulze

--------------------------
ckc ag
Sitz:
Am Alten Bahnhof 13
38122 Braunschweig

Telefon +49 (0)531 / 80 110 0
Telefax +49 (0)531 / 80 110 18444
http://www.ckc-group.de

Amtsgericht Braunschweig
HRB 5405

Vorstand:
H.-G. Christian Krentel
(Vorsitzender)
Karsten Kisser

Aufsichtsrat:
Dr. Heinz-Werner Weinrich
(Vorsitzender)
Jens Fokuhl
(stellv. Vorsitzender)
Prof. Dr. Reinhold Haux
Cäsar Jaworski
Dr. Rita Schulz
Thorsten Sponholz
--------------------------

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Erik Faye-Lund @ 2013-01-23 15:32 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Eric Chamberland, Brian J. Murrell, git, Pyeron, Jason J CTR (US),
	Maxime Boissonneault, Philippe Vaucher, Sébastien Boisvert
In-Reply-To: <87d2wvc3v0.fsf@pctrast.inf.ethz.ch>

On Wed, Jan 23, 2013 at 4:32 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> On Tue, Jan 22, 2013 at 11:14 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>>> Eric Chamberland <Eric.Chamberland@giref.ulaval.ca> writes:
>>>
>>> Other than that I agree with Junio, from what we've seen so far, Lustre
>>> returns EINTR on all sorts of calls that simply aren't allowed to do so.
>>
>> I don't think this analysis is 100% accurate, POSIX allows error codes
>> to be generated other than those defined. From
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html:
>>
>> "Implementations may support additional errors not included in this
>> list, *may generate errors included in this list under circumstances
>> other than those described here*, or may contain extensions or
>> limitations that prevent some errors from occurring."
>
> That same page says, however:
>
>   For functions under the Threads option for which [EINTR] is not listed
>   as a possible error condition in this volume of IEEE Std 1003.1-2001,
>   an implementation shall not return an error code of [EINTR].

Yes, but surely that's for pthreads functions, no? utime is not one of
those functions...

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Thomas Rast @ 2013-01-23 15:32 UTC (permalink / raw)
  To: kusmabite
  Cc: Thomas Rast, Eric Chamberland, Brian J. Murrell, git,
	Pyeron, Jason J CTR (US), Maxime Boissonneault, Philippe Vaucher,
	Sébastien Boisvert
In-Reply-To: <CABPQNSad1EKbmt3Gjs+uB9fud4YBqmk++5GMqF2s047Lcc8GwQ@mail.gmail.com>

Erik Faye-Lund <kusmabite@gmail.com> writes:

> On Tue, Jan 22, 2013 at 11:14 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>> Eric Chamberland <Eric.Chamberland@giref.ulaval.ca> writes:
>>
>> Other than that I agree with Junio, from what we've seen so far, Lustre
>> returns EINTR on all sorts of calls that simply aren't allowed to do so.
>
> I don't think this analysis is 100% accurate, POSIX allows error codes
> to be generated other than those defined. From
> http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html:
>
> "Implementations may support additional errors not included in this
> list, *may generate errors included in this list under circumstances
> other than those described here*, or may contain extensions or
> limitations that prevent some errors from occurring."

That same page says, however:

  For functions under the Threads option for which [EINTR] is not listed
  as a possible error condition in this volume of IEEE Std 1003.1-2001,
  an implementation shall not return an error code of [EINTR].

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Erik Faye-Lund @ 2013-01-23 15:23 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Eric Chamberland, Brian J. Murrell, git, Pyeron, Jason J CTR (US),
	Maxime Boissonneault, Philippe Vaucher, Sébastien Boisvert
In-Reply-To: <878v7keuh3.fsf@pctrast.inf.ethz.ch>

On Tue, Jan 22, 2013 at 11:14 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Eric Chamberland <Eric.Chamberland@giref.ulaval.ca> writes:
>
> Other than that I agree with Junio, from what we've seen so far, Lustre
> returns EINTR on all sorts of calls that simply aren't allowed to do so.

I don't think this analysis is 100% accurate, POSIX allows error codes
to be generated other than those defined. From
http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_03.html:

"Implementations may support additional errors not included in this
list, *may generate errors included in this list under circumstances
other than those described here*, or may contain extensions or
limitations that prevent some errors from occurring."

So I don't think Lustre violates POSIX by erroring with errno=EINTR,
but I also think guarding every single function call for EINTR just to
be safe to be insane :)

However, looking at Eric's log, I can't see that being what has
happened here - grepping it for EINTR does not produce a single match.

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Sébastien Boisvert @ 2013-01-23 14:50 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Eric Chamberland, Brian J. Murrell, git, kusmabite,
	Pyeron, Jason J CTR (US), Maxime Boissonneault, Philippe Vaucher
In-Reply-To: <878v7keuh3.fsf@pctrast.inf.ethz.ch>

[I forgot to subscribe to the git mailing list, sorry for that]

On 01/22/2013 05:14 PM, Thomas Rast wrote:
> Eric Chamberland <Eric.Chamberland@giref.ulaval.ca> writes:
>
>> So, hum, do we have some sort of conclusion?
>>
>> Shall it be a fix for git to get around that lustre "behavior"?
>>
>> If something can be done in git it would be great: it is a *lot*
>> easier to change git than the lustre filesystem software for a cluster
>> in running in production mode... (words from cluster team) :-/
>
> I thought you already established that simply disabling the progress
> display is a sufficient workaround?  If that doesn't help, you can try
> patching out all use of SIGALRM within git.
>

In git (9591fcc6d66), I have found these SIGALRM signal handling:

builtin/log.c:268:    sigaction(SIGALRM, &sa, NULL);
builtin/log.c:285:    signal(SIGALRM, SIG_IGN);
compat/mingw.c:1590:        mingw_raise(SIGALRM);
compat/mingw.c:1666:    if (sig != SIGALRM)
compat/mingw.c:1668:            error("sigaction only implemented for SIGALRM");
compat/mingw.c:1683:    case SIGALRM:
compat/mingw.c:1702:    case SIGALRM:
compat/mingw.c:1706:            exit(128 + SIGALRM);
compat/mingw.c:1708:            timer_fn(SIGALRM);
compat/mingw.h:42:#define SIGALRM 14
perl/Git/SVN.pm:2121:            SIGALRM, SIGUSR1, SIGUSR2);
progress.c:56:    sigaction(SIGALRM, &sa, NULL);
progress.c:68:    signal(SIGALRM, SIG_IGN);


I suppose that compat/mingw.{h,c} and SVN.pm can be ignored as our patch to work
around this problem won't be pushed upstream because the real problem is not in git, right ?

If I understand correctly, some VFS system calls get interrupted by SIGALRM, but when
they resume (via SA_RESTART) they return EINTR. Thomas said that these failed calls may need to be retried,
but that open(O_CREAT|O_EXCL) is still tricky around this case.


progress.c SIGALRM code paths are for progress and therefore are required, right ?

builtin/log.c SIGALRM code paths are for early output, and the comments in the code say that

    "If we can get the whole output in less than a tenth of a second, don't even bother doing the
     early-output thing."


So where do I start for the patch ?

> Other than that I agree with Junio, from what we've seen so far, Lustre
> returns EINTR on all sorts of calls that simply aren't allowed to do so.
>


-- 
---
Spécialiste en granularité (1 journée / semaine)
Calcul Québec / Calcul Canada
Pavillon Adrien-Pouliot, Université Laval, Québec (Québec), Canada

^ permalink raw reply

* Re: Moving commits from one branch to another
From: John Keeping @ 2013-01-23 14:49 UTC (permalink / raw)
  To: Stefan Schulze; +Cc: git
In-Reply-To: <000b01cdf973$cc685fc0$65391f40$@de>

On Wed, Jan 23, 2013 at 03:13:19PM +0100, Stefan Schulze wrote:
> > > Is there any way to move/copy commits from one branch to another
> > > without a common base-commit and without a forced push of master?
> >
> > Did you try "git rebase" with "--onto"?  You probably want something
> > like this:
> > 
> >     git rebase --onto svnbranch publishedToSvn master
> 
> I already tried this some days ago, but wasn't sure about the result. The
> resulting history looks exactly what I expected, but all the commits are on
> master after executing this commands and svnbranch only contains the
> original two commits (svn-commit creating the root directory and the
> cherry-picked commit from master)

Ah, I missed that you wanted to update svnbranch.  I don't think there's
a single command that will do that, but this should work:

    git rebase --onto svnbranch publishedToSvn master^0
    git checkout -B svnbranch HEAD

This uses a detached head to avoid modifying the wrong branch and then
updates "svnbranch" to point at that after the rebase.

> Does the current branch matter if I call git-rebase with the
> <branch>-argument?

No it will checkout that branch first.


John

^ permalink raw reply

* Re: GIT get corrupted on lustre
From: Sébastien Boisvert @ 2013-01-23 14:45 UTC (permalink / raw)
  To: Thomas Rast
  Cc: Eric Chamberland, Brian J. Murrell, git, kusmabite,
	Pyeron, Jason J CTR (US), Maxime Boissonneault, Philippe Vaucher
In-Reply-To: <878v7keuh3.fsf@pctrast.inf.ethz.ch>

On 01/22/2013 05:14 PM, Thomas Rast wrote:
> Eric Chamberland <Eric.Chamberland@giref.ulaval.ca> writes:
>
>> So, hum, do we have some sort of conclusion?
>>
>> Shall it be a fix for git to get around that lustre "behavior"?
>>
>> If something can be done in git it would be great: it is a *lot*
>> easier to change git than the lustre filesystem software for a cluster
>> in running in production mode... (words from cluster team) :-/
>
> I thought you already established that simply disabling the progress
> display is a sufficient workaround?  If that doesn't help, you can try
> patching out all use of SIGALRM within git.
>

In git (9591fcc6d66), I have found these SIGALRM signal handling:

builtin/log.c:268:	sigaction(SIGALRM, &sa, NULL);
builtin/log.c:285:	signal(SIGALRM, SIG_IGN);
compat/mingw.c:1590:		mingw_raise(SIGALRM);
compat/mingw.c:1666:	if (sig != SIGALRM)
compat/mingw.c:1668:			error("sigaction only implemented for SIGALRM");
compat/mingw.c:1683:	case SIGALRM:
compat/mingw.c:1702:	case SIGALRM:
compat/mingw.c:1706:			exit(128 + SIGALRM);
compat/mingw.c:1708:			timer_fn(SIGALRM);
compat/mingw.h:42:#define SIGALRM 14
perl/Git/SVN.pm:2121:			SIGALRM, SIGUSR1, SIGUSR2);
progress.c:56:	sigaction(SIGALRM, &sa, NULL);
progress.c:68:	signal(SIGALRM, SIG_IGN);


I suppose that compat/mingw.{h,c} and SVN.pm can be ignored as our patch to work
around this problem won't be pushed upstream because the real problem is not in git, right ?

If I understand correctly, some VFS system calls get interrupted by SIGALRM, but when
they resume (via SA_RESTART) they return EINTR. Thomas said that these failed calls may need to be retried,
but that open(O_CREAT|O_EXCL) is still tricky around this case.


progress.c SIGALRM code paths are for progress and therefore are required, right ?

builtin/log.c SIGALRM code paths are for early output, and the comments in the code say that

    "If we can get the whole output in less than a tenth of a second, don't even bother doing the
     early-output thing."


So where do I start for the patch ?

> Other than that I agree with Junio, from what we've seen so far, Lustre
> returns EINTR on all sorts of calls that simply aren't allowed to do so.
>


-- 
---
Spécialiste en granularité (1 journée / semaine)
Calcul Québec / Calcul Canada
Pavillon Adrien-Pouliot, Université Laval, Québec (Québec), Canada

^ permalink raw reply

* segmentation fault (nullpointer) with git log --submodule -p
From: Armin @ 2013-01-23 14:38 UTC (permalink / raw)
  To: git; +Cc: netzverweigerer

Hello dear git people.

I experience a reproducible segmentation fault on one of my repositories when doing a "git log --submodule -p", tested with newest version on Arch Linux (git version 1.8.1.1) and built fresh (git version 1.8.1.1.347.g9591fcc), tried on 2 seperate systems:


Program terminated with signal 11, Segmentation fault.
#0  0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752
752     for (i = 0; msg[i]; i++) {
    (gdb) bt
#0  0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752
#1  format_commit_one (context=<optimized out>, placeholder=0x526b1e "s", sb=0x7ffff69b6ad0) at pretty.c:1157
#2  format_commit_item (sb=0x7ffff69b6ad0, placeholder=0x526b1e "s", context=<optimized out>) at pretty.c:1224
#3  0x00000000004dacd9 in strbuf_expand (sb=sb@entry=0x7ffff69b6ad0, format=0x526b1e "s", format@entry=0x526b18 "  %m %s", fn=fn@entry=0x4b4730 <format_commit_item>, context=context@entry=0x7ffff69b6980)
    at strbuf.c:247
#4  0x00000000004b5816 in format_commit_message (commit=commit@entry=0x1ffafd8, format=format@entry=0x526b18 "  %m %s", sb=sb@entry=0x7ffff69b6ad0, pretty_ctx=pretty_ctx@entry=0x7ffff69b6af0) at pretty.c:1284
#5  0x00000000004dde52 in print_submodule_summary (reset=0x754640 "\033[m", add=0x754708 "\033[32m", del=0x7546e0 "\033[31m", f=0x7f0685bac7a0, rev=0x7ffff69b6b40) at submodule.c:236
#6  show_submodule_summary (f=0x7f0685bac7a0, path=<optimized out>, one=one@entry=0x1ff2af0 "\020\\vC\371\070\vJ\352\344\205\340\226u\273\021\372\330\234\004", 
    two=two@entry=0x2030a60 "\301a(\350\371\372\340mb[խo_\272\301\223V˙", dirty_submodule=<optimized out>, meta=meta@entry=0x754690 "\033[1m", del=del@entry=0x7546e0 "\033[31m", add=0x754708 "\033[32m", 
        reset=reset@entry=0x754640 "\033[m") at submodule.c:307
#7  0x000000000048dd1d in builtin_diff (name_a=name_a@entry=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", name_b=name_b@entry=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", 
    one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, xfrm_msg=0x2039a20 "\033[1mindex 105c764..c16128e 160000\033[m\n", must_show_header=must_show_header@entry=0, o=o@entry=0x7ffff69b7b88, 
        complete_rewrite=complete_rewrite@entry=0) at diff.c:2267
#8  0x000000000048e60e in run_diff_cmd (pgm=pgm@entry=0x0, name=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", other=<optimized out>, 
    attr_path=attr_path@entry=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, msg=msg@entry=0x7ffff69b74b0, o=o@entry=0x7ffff69b7b88, p=p@entry=0x20371b0)
    at diff.c:3057

(gdb) bt
#0  0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752
#1  format_commit_one (context=<optimized out>, placeholder=0x526b1e "s", sb=0x7ffff69b6ad0) at pretty.c:1157
#2  format_commit_item (sb=0x7ffff69b6ad0, placeholder=0x526b1e "s", context=<optimized out>) at pretty.c:1224
#3  0x00000000004dacd9 in strbuf_expand (sb=sb@entry=0x7ffff69b6ad0, format=0x526b1e "s", format@entry=0x526b18 "  %m %s", fn=fn@entry=0x4b4730 <format_commit_item>, context=context@entry=0x7ffff69b6980)
    at strbuf.c:247
#4  0x00000000004b5816 in format_commit_message (commit=commit@entry=0x1ffafd8, format=format@entry=0x526b18 "  %m %s", sb=sb@entry=0x7ffff69b6ad0, pretty_ctx=pretty_ctx@entry=0x7ffff69b6af0) at pretty.c:1284
#5  0x00000000004dde52 in print_submodule_summary (reset=0x754640 "\033[m", add=0x754708 "\033[32m", del=0x7546e0 "\033[31m", f=0x7f0685bac7a0, rev=0x7ffff69b6b40) at submodule.c:236
#6  show_submodule_summary (f=0x7f0685bac7a0, path=<optimized out>, one=one@entry=0x1ff2af0 "\020\\vC\371\070\vJ\352\344\205\340\226u\273\021\372\330\234\004", 
    two=two@entry=0x2030a60 "\301a(\350\371\372\340mb[խo_\272\301\223V˙", dirty_submodule=<optimized out>, meta=meta@entry=0x754690 "\033[1m", del=del@entry=0x7546e0 "\033[31m", add=0x754708 "\033[32m", 
    reset=reset@entry=0x754640 "\033[m") at submodule.c:307
#7  0x000000000048dd1d in builtin_diff (name_a=name_a@entry=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", name_b=name_b@entry=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", 
    one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, xfrm_msg=0x2039a20 "\033[1mindex 105c764..c16128e 160000\033[m\n", must_show_header=must_show_header@entry=0, o=o@entry=0x7ffff69b7b88, 
    complete_rewrite=complete_rewrite@entry=0) at diff.c:2267
#8  0x000000000048e60e in run_diff_cmd (pgm=pgm@entry=0x0, name=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", other=<optimized out>, 
    attr_path=attr_path@entry=0x1ff2b50 "Packages/Application/Amadeus.Somea.Dialog", one=one@entry=0x1ff2af0, two=two@entry=0x2030a60, msg=msg@entry=0x7ffff69b74b0, o=o@entry=0x7ffff69b7b88, p=p@entry=0x20371b0)
    at diff.c:3057
#9  0x000000000048eb3d in run_diff (o=0x7ffff69b7b88, p=0x20371b0) at diff.c:3145
#10 diff_flush_patch (o=0x7ffff69b7b88, p=0x20371b0) at diff.c:3979
#11 diff_flush_patch (p=0x20371b0, o=0x7ffff69b7b88) at diff.c:3970
#12 0x000000000048f15f in diff_flush (options=options@entry=0x7ffff69b7b88) at diff.c:4500
#13 0x00000000004a211a in log_tree_diff_flush (opt=opt@entry=0x7ffff69b7850) at log-tree.c:776
#14 0x00000000004a22b2 in log_tree_diff (log=0x7ffff69b7720, commit=0x1ffdf60, opt=0x7ffff69b7850) at log-tree.c:836
#15 log_tree_commit (opt=opt@entry=0x7ffff69b7850, commit=commit@entry=0x1ffdf60) at log-tree.c:859
#16 0x00000000004393d3 in cmd_log_walk (rev=rev@entry=0x7ffff69b7850) at builtin/log.c:310
#17 0x0000000000439f38 in cmd_log (argc=3, argv=0x7ffff69b80c0, prefix=0x0) at builtin/log.c:582
#18 0x0000000000405978 in run_builtin (argv=0x7ffff69b80c0, argc=3, p=0x74fd20) at git.c:281
#19 handle_internal_command (argc=3, argv=0x7ffff69b80c0) at git.c:442
#20 0x0000000000404de2 in run_argv (argv=0x7ffff69b7f50, argcp=0x7ffff69b7f5c) at git.c:488
#21 main (argc=3, argv=0x7ffff69b80c0) at git.c:563
(gdb) f 0
#0  0x00000000004b51e5 in parse_commit_header (context=0x7ffff69b6980) at pretty.c:752
752     for (i = 0; msg[i]; i++) {
(gdb) l
747 static void parse_commit_header(struct format_commit_context *context)
748 {
749     const char *msg = context->message;
750     int i;
751 
752     for (i = 0; msg[i]; i++) {
753         int eol;
754         for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
755             ; /* do nothing */
756 
(gdb) p msg
$7 = <optimized out>
(gdb) p context->message
$8 = 0x0
(gdb) x/8i $pc
=> 0x4b51e5 <format_commit_item+2741>:  movzbl (%rcx),%eax
   0x4b51e8 <format_commit_item+2744>:  mov    %rcx,0x18(%rsp)
   0x4b51ed <format_commit_item+2749>:  mov    %rcx,%r10
   0x4b51f0 <format_commit_item+2752>:  test   %al,%al
   0x4b51f2 <format_commit_item+2754>:  je     0x4b52a3 <format_commit_item+2931>
   0x4b51f8 <format_commit_item+2760>:  nopl   0x0(%rax,%rax,1)
   0x4b5200 <format_commit_item+2768>:  test   %al,%al
   0x4b5202 <format_commit_item+2770>:  je     0x4b529e <format_commit_item+2926>
(gdb) i r rcx
rcx            0x0  0


Does this help in any way? Can i provide any further information that helps?

Many thanks for reading this and all the best,


Armin

^ permalink raw reply

* AW: Moving commits from one branch to another
From: Stefan Schulze @ 2013-01-23 14:13 UTC (permalink / raw)
  To: git
In-Reply-To: <20130123121203.GM7498@serenity.lan>

> > Is there any way to move/copy commits from one branch to another
> > without a common base-commit and without a forced push of master?
>
> Did you try "git rebase" with "--onto"?  You probably want something
> like this:
> 
>     git rebase --onto svnbranch publishedToSvn master

I already tried this some days ago, but wasn't sure about the result. The
resulting history looks exactly what I expected, but all the commits are on
master after executing this commands and svnbranch only contains the
original two commits (svn-commit creating the root directory and the
cherry-picked commit from master)

Does the current branch matter if I call git-rebase with the
<branch>-argument?

^ permalink raw reply

* Minor gitk bug - update on empty project : can't read "viewmainheadid_orig(1)": no such variable
From: Indigo Frankencastle @ 2013-01-23 14:00 UTC (permalink / raw)
  To: git

One of these "weird usage causes weird bugs".

One way to reproduce:

mkdir foo
cd foo
git init
git commit -am "Initial"
gitk &
# or
gitk --all &
<F5>

Resulting in (gitk-git/gitk line 503 - 512):

can't read "viewmainheadid_orig(1)": no such variable
can't read "viewmainheadid_orig(1)": no such variable
    while executing
"if {$mainheadid ne $viewmainheadid_orig($view)} {
	if {$showlocalchanges} {
	    dohidelocalchanges
	}
	set viewmainheadid($view) $mainheadid
	set vie..."
    (procedure "updatecommits" line 13)
    invoked from within
"updatecommits"
    (command bound to event)

As "$mainheadid" is not set, should there be something in the
direction of this perhaps?

    ...
Subject: [PATCH] gitk catch missing head id on update

---
 gitk-git/gitk |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index d93bd99..fa869d7 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -499,6 +499,11 @@ proc updatecommits {} {

     set hasworktree [hasworktree]
     rereadrefs
+    if {$mainheadid eq ""} {
+	# error_popup "[mc "Error updating commits:"] No main HEAD id"
+	show_status [mc "No main HEAD id"]
+	return {}
+    }
     set view $curview
     if {$mainheadid ne $viewmainheadid_orig($view)} {
 	if {$showlocalchanges} {
-- 
1.7.10.4

    ...
Or perhaps some logic in:
proc rereadrefs {}

----
Best regards,
IF

^ permalink raw reply related

* Re: What's cooking in git.git (Jan 2013, #08; Tue, 22)
From: John Keeping @ 2013-01-23 13:55 UTC (permalink / raw)
  To: Chris Rorvick; +Cc: Junio C Hamano, git, Eric S. Raymond
In-Reply-To: <CAEUsAPaUy5ug0_HPjWDTSnAG0kURhP-1-9nOu9_Tpn5nEv6N_Q@mail.gmail.com>

On Wed, Jan 23, 2013 at 07:26:24AM -0600, Chris Rorvick wrote:
> On Wed, Jan 23, 2013 at 3:28 AM, John Keeping <john@keeping.me.uk> wrote:
> > In my opinion the incremental import support really is substantially
> > worse in cvsimport-3 than cvsimport-2.  cvsimport-2 looks at the output
> > of git-for-each-ref to calculate the dates from which to continue each
> > branch.  cvsps cannot be told this information and so the cvsimport-3
> > script just takes the date of the last commit on the current branch.
> 
> Do you really need a timestamp per branch, though?  If you have
> branches A and B, and B has a commit timestamp 5 minutes after A, you
> can infer that nothing happened on A for those five minutes, right?
> So maybe a single timestamp is sufficient, it just may not be picking
> the right one.  Instead cvsimport-3 should compute the latest
> timestamp across all import branches.

The problem is telling which is an import branch, since it currently
just used "refs/heads/<branch>".

I do have a change to write the timestamp to a file, which takes the
newest commit across all of the branches that have changed during an
import.  That may well be good enough but doesn't let you incrementally
update a repository that has been cloned from elsewhere.


John

^ permalink raw reply

* Re: [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
From: Duy Nguyen @ 2013-01-23 13:38 UTC (permalink / raw)
  To: git; +Cc: Jens Lehmann
In-Reply-To: <1358948067-2792-1-git-send-email-pclouds@gmail.com>

On Wed, Jan 23, 2013 at 8:34 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> add_submodule_odb() can be used to import objects from another
> repository temporarily. After this point we don't know which objects
> are ours, which are external. If we create an object that refers to an
> external object, next time git runs, it may find a hole in the object
> graph because the external repository may not be imported. The same
> goes for pointing a ref to an external SHA-1.
>
> To protect ourselves, once add_submodule_odb() is used:
>
>  - trees, tags and commits cannot be created
>  - refs cannot be updated

.. and soon after I pressed send, I realized I missed at least two
other places write should not be allowed:

 - index
 - reflog

But the general idea is probably more important than implementation
details at this stage.
-- 
Duy

^ permalink raw reply

* [PATCH/RFC] Revoke write access to refs and odb after importing another repo's odb
From: Nguyễn Thái Ngọc Duy @ 2013-01-23 13:34 UTC (permalink / raw)
  To: git; +Cc: Jens Lehmann, Nguyễn Thái Ngọc Duy

add_submodule_odb() can be used to import objects from another
repository temporarily. After this point we don't know which objects
are ours, which are external. If we create an object that refers to an
external object, next time git runs, it may find a hole in the object
graph because the external repository may not be imported. The same
goes for pointing a ref to an external SHA-1.

To protect ourselves, once add_submodule_odb() is used:

 - trees, tags and commits cannot be created
 - refs cannot be updated

In certain cases that submodule code knows that it's safe to write, it
can turn the readonly flag off.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I think this is a good safety check. It catches at least a case in
 t7405.3. I did not investigate further though.

 cache.h      | 1 +
 refs.c       | 2 ++
 sha1_file.c  | 2 ++
 submodule.c  | 7 +++++++
 5 files changed, 16 insertions(+)

diff --git a/cache.h b/cache.h
index c257953..772d229 100644
--- a/cache.h
+++ b/cache.h
@@ -753,6 +753,7 @@ extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
 extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
+extern int git_repo_readonly();
 
 /* global flag to enable extra checks when accessing packed objects */
 extern int do_check_packed_object_crc;
diff --git a/refs.c b/refs.c
index 541fec2..22b13f4 100644
--- a/refs.c
+++ b/refs.c
@@ -1711,6 +1711,8 @@ struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha
 struct ref_lock *lock_any_ref_for_update(const char *refname,
 					 const unsigned char *old_sha1, int flags)
 {
+	if (git_repo_readonly())
+		die("repository in read-only mode, cannot update refs");
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
 		return NULL;
 	return lock_ref_sha1_basic(refname, old_sha1, flags, NULL);
diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..b9e8b59 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2575,6 +2575,8 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
 	char hdr[32];
 	int hdrlen;
 
+	if (git_repo_readonly() && strcmp(type, "blob"))
+		die("repository in read-only mode, cannot update object database");
 	/* Normally if we have it in the pack then we do not bother writing
 	 * it out into .git/objects/??/?{38} file.
 	 */
diff --git a/submodule.c b/submodule.c
index 2f55436..5eba597 100644
--- a/submodule.c
+++ b/submodule.c
@@ -19,6 +19,7 @@ static struct string_list changed_submodule_paths;
 static int initialized_fetch_ref_tips;
 static struct sha1_array ref_tips_before_fetch;
 static struct sha1_array ref_tips_after_fetch;
+static int readonly;
 
 /*
  * The following flag is set if the .gitmodules file is unmerged. We then
@@ -30,6 +31,11 @@ static struct sha1_array ref_tips_after_fetch;
  */
 static int gitmodules_is_unmerged;
 
+int git_repo_readonly()
+{
+	return readonly;
+}
+
 static int add_submodule_odb(const char *path)
 {
 	struct strbuf objects_directory = STRBUF_INIT;
@@ -67,6 +73,7 @@ static int add_submodule_odb(const char *path)
 	/* add possible alternates from the submodule */
 	read_info_alternates(objects_directory.buf, 0);
 	prepare_alt_odb();
+	readonly = 1;
 done:
 	strbuf_release(&objects_directory);
 	return ret;
-- 
1.8.0.rc2.23.g1fb49df

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox