Re: [PATCH v3 24/25] prune: strategies for linked checkouts
On Thu, Feb 20, 2014 at 3:32 AM, Junio C Hamano gits...@pobox.com wrote:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
(Only nitpicks during this round of review).
+ if (get_device_or_die(path) != get_device_or_die(get_git_dir())) {
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/locked, sb_repo.buf);
+ write_file(sb.buf, 1, located on a different file system\n);
+ keep_locked = 1;
+ } else {
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/link, sb_repo.buf);
+ link(sb_git.buf, sb.buf); /* it's ok to fail */
If so, perhaps tell that to the code by saying something like
(void) link(...);
?
But why is it OK to fail in the first place? If we couldn't link,
don't you want to fall back to the lock codepath? After all, the
are we on the same device? check is to cope with the case where
we cannot link and that alternate codepath is supposed to be
prepared to handle the ah, we cannot link case correctly, no?
Filesystems not supporting hardlinks are one reason. The idea behind
locked is that the new checkout could be on a portable device and we
don't want to prune its metadata just because the device is not
plugged in. Checking same device help but even that can't guarantee no
false positives, unless your only mount point is /. So no I don't
really think we should go lock whenever link() fails, that would just
make fewer checkouts prunable.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 24/25] prune: strategies for linked checkouts
On Thu, Feb 20, 2014 at 5:08 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
+static dev_t get_device_or_die(const char *path)
+{
+ struct stat buf;
+ if (stat(path, buf))
+ die_errno(failed to stat '%s', path);
+ /* Ah Windows! Make different drives different partitions */
+ if (buf.st_dev == 0 has_dos_drive_prefix(c:\\))
+ buf.st_dev = toupper(real_path(path)[0]);
This invocation of has_dos_drive_prefix() with hardcoded c:\\ at
first looks like an error until the reader realizes that it's an
#ifdef-less check if the platforms is Windows. Would it make more
sense to encapsulate this anomaly and abstract it away by fixing
compat/mingw.c:do_lstat() to instead set 'st_dev' automatically like
you do here? In particular, this line in mingw.c:
buf-st_dev = buf-st_rdev = 0; /* not used by Git */
I don't want to hide too much magic behind compat curtain, especially
when these magic is just about 90% as real, the rest is mysterious
corner cases. I guess we could just add an inline function
is_windows() that always returns 0 or 1 based on #ifdef.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 24/25] prune: strategies for linked checkouts
Duy Nguyen pclo...@gmail.com writes:
But why is it OK to fail in the first place? If we couldn't link,
don't you want to fall back to the lock codepath? After all, the
are we on the same device? check is to cope with the case where
we cannot link and that alternate codepath is supposed to be
prepared to handle the ah, we cannot link case correctly, no?
Filesystems not supporting hardlinks are one reason. The idea behind
locked is that the new checkout could be on a portable device and we
don't want to prune its metadata just because the device is not
plugged in. Checking same device help but even that can't guarantee no
false positives, unless your only mount point is /. So no I don't
really think we should go lock whenever link() fails, that would just
make fewer checkouts prunable.
It just looked wrong to have a logic that goes like this:
if (we detect one case link() can never possibly work) {
do something for a case we do not use link;
} else {
prepare parameters that is used when we do link;
do link() but ignore error;
}
and felt that I would have understood if it were more like this:
prepare parameters that is used when we do link;
if (try link() and it fails) {
do something for a case we do not use link;
}
but I did not mean that was the only alternative to correct the
wrongness.
I do not think I yet know the motivation behind this step well
enough to further comment---until I read the earlier parts of the
series that lead to this step, so I'll do that first.
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 v3 24/25] prune: strategies for linked checkouts
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
(Only nitpicks during this round of review).
+ if (get_device_or_die(path) != get_device_or_die(get_git_dir())) {
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/locked, sb_repo.buf);
+ write_file(sb.buf, 1, located on a different file system\n);
+ keep_locked = 1;
+ } else {
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/link, sb_repo.buf);
+ link(sb_git.buf, sb.buf); /* it's ok to fail */
If so, perhaps tell that to the code by saying something like
(void) link(...);
?
But why is it OK to fail in the first place? If we couldn't link,
don't you want to fall back to the lock codepath? After all, the
are we on the same device? check is to cope with the case where
we cannot link and that alternate codepath is supposed to be
prepared to handle the ah, we cannot link case correctly, no?
--
To unsubscribe from this list: send the line 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 24/25] prune: strategies for linked checkouts
Junio C Hamano gits...@pobox.com writes:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
(Only nitpicks during this round of review).
+if (get_device_or_die(path) != get_device_or_die(get_git_dir())) {
+strbuf_reset(sb);
+strbuf_addf(sb, %s/locked, sb_repo.buf);
+write_file(sb.buf, 1, located on a different file system\n);
+keep_locked = 1;
+} else {
+strbuf_reset(sb);
+strbuf_addf(sb, %s/link, sb_repo.buf);
+link(sb_git.buf, sb.buf); /* it's ok to fail */
If so, perhaps tell that to the code by saying something like
(void) link(...);
?
But why is it OK to fail in the first place? If we couldn't link,
don't you want to fall back to the lock codepath? After all, the
are we on the same device? check is to cope with the case where
we cannot link and that alternate codepath is supposed to be
prepared to handle the ah, we cannot link case correctly, no?
In other words, couldn't that part more like this? That is, we
optimisiticly try the link(2) first and if it fails for whatever
reason fall back to whatever magic the lock codepath implements.
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/link, sb_repo.buf);
+ if (link(sb_git.buf, sb.buf) 0) {
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/locked, sb_repo.buf);
+ write_file(sb.buf, 1, located on a different file system\n);
+ keep_locked = 1;
+ }
+
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 24/25] prune: strategies for linked checkouts
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
The pruning rules are:
- if $REPO/locked exists, repos/id is not supposed to be pruned.
- if $REPO/locked exists and $REPO/gitdir's mtimer is older than a
s/mtimer/mtime/
really long limit, warn about old unused repo.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7b86f2b..c501e9c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -854,6 +854,17 @@ static void remove_junk_on_signal(int signo)
raise(signo);
}
+static dev_t get_device_or_die(const char *path)
+{
+ struct stat buf;
+ if (stat(path, buf))
+ die_errno(failed to stat '%s', path);
+ /* Ah Windows! Make different drives different partitions */
+ if (buf.st_dev == 0 has_dos_drive_prefix(c:\\))
+ buf.st_dev = toupper(real_path(path)[0]);
This invocation of has_dos_drive_prefix() with hardcoded c:\\ at
first looks like an error until the reader realizes that it's an
#ifdef-less check if the platforms is Windows. Would it make more
sense to encapsulate this anomaly and abstract it away by fixing
compat/mingw.c:do_lstat() to instead set 'st_dev' automatically like
you do here? In particular, this line in mingw.c:
buf-st_dev = buf-st_rdev = 0; /* not used by Git */
+ return buf.st_dev;
+}
--
To unsubscribe from this list: send the line 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 24/25] prune: strategies for linked checkouts
On Wed, Feb 19, 2014 at 5:08 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com
wrote:
+static dev_t get_device_or_die(const char *path)
+{
+ struct stat buf;
+ if (stat(path, buf))
+ die_errno(failed to stat '%s', path);
+ /* Ah Windows! Make different drives different partitions */
+ if (buf.st_dev == 0 has_dos_drive_prefix(c:\\))
+ buf.st_dev = toupper(real_path(path)[0]);
This invocation of has_dos_drive_prefix() with hardcoded c:\\ at
first looks like an error until the reader realizes that it's an
#ifdef-less check if the platforms is Windows. Would it make more
sense to encapsulate this anomaly and abstract it away by fixing
compat/mingw.c:do_lstat() to instead set 'st_dev' automatically like
you do here? In particular, this line in mingw.c:
buf-st_dev = buf-st_rdev = 0; /* not used by Git */
+ return buf.st_dev;
Or, if doing this in do_lstat() is too expensive for normal stat()
operations (which is likely), then a simple #ifdef might be easier to
read; or abstract it into a get_device() function which Windows/MinGW
can override, doing buf.st_dev = toupper(real_path(...)), thus also
making the code easier to understand.
--
To unsubscribe from this list: send the line 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 24/25] prune: strategies for linked checkouts
alias REPO=$GIT_COMMON_DIR/repos/id
- linked checkouts are supposed to update mtime of $REPO/gitdir
- linked checkouts are supposed to keep its location in $REPO/gitdir up to
date
- git checkout --to is supposed to create $REPO/locked if the new
repo is on a different partition than the shared one.
- git checkout --to is supposed to make hardlink named $REPO/link
pointing to the .git file
The pruning rules are:
- if $REPO/locked exists, repos/id is not supposed to be pruned.
- if $REPO/locked exists and $REPO/gitdir's mtimer is older than a
really long limit, warn about old unused repo.
- if $REPO/link exists and its link count is greated than 1, the repo
is kept.
- if $REPO/gitdir's mtime is older than a limit, and it points to
nowhere, repos/id is to be pruned.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/git-prune.txt| 3 ++
Documentation/gitrepository-layout.txt | 19 +
builtin/checkout.c | 36 +++-
builtin/prune.c| 75 ++
setup.c| 13 ++
5 files changed, 145 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-prune.txt b/Documentation/git-prune.txt
index 058ac0d..7babf11 100644
--- a/Documentation/git-prune.txt
+++ b/Documentation/git-prune.txt
@@ -48,6 +48,9 @@ OPTIONS
--expire time::
Only expire loose objects older than time.
+--repos::
+ Prune directories in $GIT_DIR/repos.
+
head...::
In addition to objects
reachable from any of our references, keep objects
diff --git a/Documentation/gitrepository-layout.txt
b/Documentation/gitrepository-layout.txt
index 495a937..784d0a5 100644
--- a/Documentation/gitrepository-layout.txt
+++ b/Documentation/gitrepository-layout.txt
@@ -221,6 +221,25 @@ modules::
repos::
Contains worktree specific information of linked checkouts.
+repos/id/gitdir::
+ A text file containing the absolute path back to the .git file
+ that points to here. This is used to check if the linked
+ repository has been manually removed and there is no need to
+ keep this directory any more. mtime of this file should be
+ updated every time the linked repository is accessed.
+
+repos/id/locked::
+ If this file exists, the linked repository may be on a
+ portable device and not available. It does not mean that the
+ linked repository is gone and `repos/id` could be
+ removed. The file's content contains a reason string on why
+ the repository is locked.
+
+repos/id/link::
+ If this file exists, it is a hard link to the linked .git
+ file. It is used to detect if the linked repository is
+ manually removed.
+
SEE ALSO
linkgit:git-init[1],
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7b86f2b..c501e9c 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -854,6 +854,17 @@ static void remove_junk_on_signal(int signo)
raise(signo);
}
+static dev_t get_device_or_die(const char *path)
+{
+ struct stat buf;
+ if (stat(path, buf))
+ die_errno(failed to stat '%s', path);
+ /* Ah Windows! Make different drives different partitions */
+ if (buf.st_dev == 0 has_dos_drive_prefix(c:\\))
+ buf.st_dev = toupper(real_path(path)[0]);
+ return buf.st_dev;
+}
+
static int prepare_linked_checkout(const struct checkout_opts *opts,
struct branch_info *new)
{
@@ -863,7 +874,7 @@ static int prepare_linked_checkout(const struct
checkout_opts *opts,
struct stat st;
const char *name;
struct child_process cp;
- int counter = 0, len, ret;
+ int counter = 0, len, keep_locked = 0, ret;
if (!new-commit)
die(_(no branch specified));
@@ -898,12 +909,18 @@ static int prepare_linked_checkout(const struct
checkout_opts *opts,
junk_git_dir = sb_repo.buf;
is_junk = 1;
+ strbuf_addf(sb, %s/locked, sb_repo.buf);
+ write_file(sb.buf, 1, initializing\n);
+
strbuf_addf(sb_git, %s/.git, path);
if (safe_create_leading_directories_const(sb_git.buf))
die_errno(_(could not create leading directories of '%s'),
sb_git.buf);
junk_work_tree = path;
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/gitdir, sb_repo.buf);
+ write_file(sb.buf, 1, %s\n, real_path(sb_git.buf));
write_file(sb_git.buf, 1, gitdir: %s/repos/%s\n,
real_path(get_git_dir()), name);
/*
@@ -912,12 +929,24 @@ static int prepare_linked_checkout(const struct
checkout_opts *opts,
* value would do because this value will be ignored and
* replaced at the next (real) checkout.
*/
+ strbuf_reset(sb);
strbuf_addf(sb, %s/HEAD, sb_repo.buf);
write_file(sb.buf, 1,
