Re: [PATCH] clone: forbid --bare --separate-git-dir dir
On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:
Unfortunately we forgot to forbid the --bare
--separate-git-dir combination. In practice, we know no one
could be using --bare with --separate-git-dir because it is
broken in the following way: explanation here. So it is
safe to make good on our mistake and forbid the combination,
making the command easier to explain.
I don't know what would go in the explanation here blank above,
though. Is it possible that some people are relying on this option
combination?
I can't say it's broken in what way. Maybe it's easier to just support
this case, it's not much work anyway. Jens, maybe squash this to your
original patch?
-- 8 --
diff --git a/builtin/clone.c b/builtin/clone.c
index 8d23a62..c8b09ad 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -375,6 +375,7 @@ static void clone_local(const char *src_repo, const char
*dest_repo)
static const char *junk_work_tree;
static const char *junk_git_dir;
+static const char *junk_git_file;
static pid_t junk_pid;
static void remove_junk(void)
@@ -392,6 +393,8 @@ static void remove_junk(void)
remove_dir_recursively(sb, 0);
strbuf_reset(sb);
}
+ if (junk_git_file)
+ unlink(junk_git_file);
}
static void remove_junk_on_signal(int signo)
@@ -772,6 +775,8 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
set_git_dir_init(git_dir, real_git_dir, 0);
if (real_git_dir) {
+ if (option_bare)
+ junk_git_file = git_dir;
git_dir = real_git_dir;
junk_git_dir = real_git_dir;
}
diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 4435693..231bc8a 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -49,4 +49,14 @@ test_expect_success 'failed clone --separate-git-dir should
not leave any direct
rmdir foo/.git/objects.bak
'
+test_expect_success 'failed clone --separate-git-dir --bare should not leave
any directories' '
+ mkdir foo/.git/objects.bak/
+ mv foo/.git/objects/* foo/.git/objects.bak/
+ test_must_fail git clone --bare --separate-git-dir gitdir foo baaar
+ test_must_fail test -e gitdir
+ test_must_fail test -e baaar
+ mv foo/.git/objects.bak/* foo/.git/objects/
+ rmdir foo/.git/objects.bak
+'
+
test_done
-- 8 --
--
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] clone: forbid --bare --separate-git-dir dir
Am 08.01.2013 15:16, schrieb Duy Nguyen:
On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote:
Unfortunately we forgot to forbid the --bare
--separate-git-dir combination. In practice, we know no one
could be using --bare with --separate-git-dir because it is
broken in the following way: explanation here. So it is
safe to make good on our mistake and forbid the combination,
making the command easier to explain.
I don't know what would go in the explanation here blank above,
though. Is it possible that some people are relying on this option
combination?
I can't say it's broken in what way. Maybe it's easier to just support
this case, it's not much work anyway. Jens, maybe squash this to your
original patch?
I'd be fine with that, though my gut feeling is that this should
be a patch of its own (My patch handles the git dir, your's handles
a work tree issue). But I don't care much either way, what do others
think?
-- 8 --
diff --git a/builtin/clone.c b/builtin/clone.c
index 8d23a62..c8b09ad 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -375,6 +375,7 @@ static void clone_local(const char *src_repo, const char
*dest_repo)
static const char *junk_work_tree;
static const char *junk_git_dir;
+static const char *junk_git_file;
static pid_t junk_pid;
static void remove_junk(void)
@@ -392,6 +393,8 @@ static void remove_junk(void)
remove_dir_recursively(sb, 0);
strbuf_reset(sb);
}
+ if (junk_git_file)
+ unlink(junk_git_file);
}
static void remove_junk_on_signal(int signo)
@@ -772,6 +775,8 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
set_git_dir_init(git_dir, real_git_dir, 0);
if (real_git_dir) {
+ if (option_bare)
+ junk_git_file = git_dir;
git_dir = real_git_dir;
junk_git_dir = real_git_dir;
}
diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 4435693..231bc8a 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -49,4 +49,14 @@ test_expect_success 'failed clone --separate-git-dir
should not leave any direct
rmdir foo/.git/objects.bak
'
+test_expect_success 'failed clone --separate-git-dir --bare should not leave
any directories' '
+ mkdir foo/.git/objects.bak/
+ mv foo/.git/objects/* foo/.git/objects.bak/
+ test_must_fail git clone --bare --separate-git-dir gitdir foo baaar
+ test_must_fail test -e gitdir
+ test_must_fail test -e baaar
+ mv foo/.git/objects.bak/* foo/.git/objects/
+ rmdir foo/.git/objects.bak
+'
+
test_done
-- 8 --
--
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] clone: forbid --bare --separate-git-dir dir
Jens Lehmann jens.lehm...@web.de writes: Am 08.01.2013 15:16, schrieb Duy Nguyen: On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote: Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: explanation here. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain. I don't know what would go in the explanation here blank above, though. Is it possible that some people are relying on this option combination? I can't say it's broken in what way. Maybe it's easier to just support this case, it's not much work anyway. Jens, maybe squash this to your original patch? I'd be fine with that, though my gut feeling is that this should be a patch of its own (My patch handles the git dir, your's handles a work tree issue). I agree that these are totally unrelated issues. After all, Jonathan's suggestion to forbid it was because the combination does not make sense and does not have practical uses, and forbidding it would make the command easier to explain than leaving it accepted from the command line. If you choose to go in the opposite direction and make clone --bare --separate-git-dir do something useful, it should be explained very well in the documentation part of the patch why such a combination is a good idea, and in what situation the behaviour is useful and the user may want to consider using it, I think. -- 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] clone: forbid --bare --separate-git-dir dir
On Wed, Jan 9, 2013 at 12:45 AM, Junio C Hamano gits...@pobox.com wrote: Jens Lehmann jens.lehm...@web.de writes: Am 08.01.2013 15:16, schrieb Duy Nguyen: On Sun, Jan 06, 2013 at 02:19:48AM -0800, Jonathan Nieder wrote: Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: explanation here. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain. I don't know what would go in the explanation here blank above, though. Is it possible that some people are relying on this option combination? I can't say it's broken in what way. Maybe it's easier to just support this case, it's not much work anyway. Jens, maybe squash this to your original patch? I'd be fine with that, though my gut feeling is that this should be a patch of its own (My patch handles the git dir, your's handles a work tree issue). I agree that these are totally unrelated issues. After all, Jonathan's suggestion to forbid it was because the combination does not make sense and does not have practical uses, and forbidding it would make the command easier to explain than leaving it accepted from the command line. If you choose to go in the opposite direction and make clone --bare --separate-git-dir do something useful, it should be explained very well in the documentation part of the patch why such a combination is a good idea, and in what situation the behaviour is useful and the user may want to consider using it, I think. It is more like postponing the usefulness evaluation of the combination until later (maybe someone will come up with an actual use case). As of now, --separate-git-dir --bare is a valid combination. Jens' patch fixes one case but leave the other case broken, which is why I think it should be in one patch. It's rather ducking head in the sand than actually declaring that the combination is useful. -- 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] clone: forbid --bare --separate-git-dir dir
Duy Nguyen pclo...@gmail.com writes: After all, Jonathan's suggestion to forbid it was because the combination does not make sense and does not have practical uses, and forbidding it would make the command easier to explain than leaving it accepted from the command line. If you choose to go in the opposite direction and make clone --bare --separate-git-dir do something useful, it should be explained very well in the documentation part of the patch why such a combination is a good idea, and in what situation the behaviour is useful and the user may want to consider using it, I think. It is more like postponing the usefulness evaluation of the combination until later (maybe someone will come up with an actual use case). As of now, --separate-git-dir --bare is a valid combination. Jens' patch fixes one case but leave the other case broken, which is why I think it should be in one patch. It's rather ducking head in the sand than actually declaring that the combination is useful. When a user comes and asks how git clone --bare --separate-git-dir is meant to be used, you are saying that your answer will be Eh, it does something random that I cannot explain, and I cannot even suggest a good use case for it, but somebody may find it useful.? If we get rid of it, we do not have to explain what such a useless combination would/should do, 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
[PATCH] clone: forbid --bare --separate-git-dir dir
--separate-git-dir was added to clone with the repository away from standard position worktree/.git. It does not make sense to use it without creating working directory. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- On Sun, Jan 6, 2013 at 4:16 PM, Jonathan Nieder jrnie...@gmail.com wrote: Duy Nguyen wrote: And because junk_work_tree is not set up for --bare, I guess we still need to fix --bare --separate-git-dir case, or forbid it because i'm not sure if that case makes sense at all. Forbidding it makes sense to me. If some day we want a git command to create a .git file, I don't think it should be spelled as git clone --bare --separate-git-dir repo. That command does not work because --separate-git-dir requires an argument in addition to repo, which is taken as the target repo. Still it's hard to find a use case for git clone --bare --separate-git-dir real-repo symlink-repo In the meantime, printf '%s\n' 'gitdir: /path/to/repo' repo.git works fine. Yeah. A separate clone operation following by that printf should be clearer. builtin/clone.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index ec2f75b..360e430 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -704,6 +704,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (option_origin) die(_(--bare and --origin %s options are incompatible.), option_origin); + if (real_git_dir) + /* +* If you lift this restriction, remember to +* clean .git in this case in remove_junk_on_signal +*/ + die(_(--bare and --separate-git-dir are incompatible.)); option_no_checkout = 1; } -- 1.8.0.rc2.23.g1fb49df -- 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] clone: forbid --bare --separate-git-dir dir
Nguyễn Thái Ngọc Duy wrote: --separate-git-dir was added to clone with the repository away from standard position worktree/.git. It does not make sense to use it without creating working directory. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com The patch correctly implements the above. The description leaves out detail. I'd say something like The --separate-git-dir option was introduced to make it simple to put the git directory somewhere outside the worktree, for example when cloning a repository for use as a submodule. It was not intended for use when creating a bare repository. In that case there is no worktree and it is more natural to directly clone the repository and create a .git file as separate steps: git clone --bare /path/to/repo.git bar.git printf 'gitdir: bar.git\n' foo.git Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: explanation here. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain. I don't know what would go in the explanation here blank above, though. Is it possible that some people are relying on this option combination? -- 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] clone: forbid --bare --separate-git-dir dir
Jonathan Nieder jrnie...@gmail.com writes: Nguyễn Thái Ngọc Duy wrote: --separate-git-dir was added to clone with the repository away from standard position worktree/.git. It does not make sense to use it without creating working directory. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com The patch correctly implements the above. The description leaves out detail. I'd say something like The --separate-git-dir option was introduced to make it simple to put the git directory somewhere outside the worktree, for example when cloning a repository for use as a submodule. It was not intended for use when creating a bare repository. In that case there is no worktree and it is more natural to directly clone the repository and create a .git file as separate steps: git clone --bare /path/to/repo.git bar.git printf 'gitdir: bar.git\n' foo.git Unfortunately we forgot to forbid the --bare --separate-git-dir combination. In practice, we know no one could be using --bare with --separate-git-dir because it is broken in the following way: explanation here. So it is safe to make good on our mistake and forbid the combination, making the command easier to explain. I don't know what would go in the explanation here blank above, though. Is it possible that some people are relying on this option combination? I do not necessarily think we must say it happens not to work already for such and such reasons, lucky us!, but it is indeed a good idea to think things through, justifying why this cannot be a regression, and record the fact that we did that thinking, in the log message. 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] clone: forbid --bare --separate-git-dir dir
On Mon, Jan 7, 2013 at 6:13 AM, Junio C Hamano gits...@pobox.com wrote: I don't know what would go in the explanation here blank above, though. Is it possible that some people are relying on this option combination? I do not necessarily think we must say it happens not to work already for such and such reasons, lucky us!, but it is indeed a good idea to think things through, justifying why this cannot be a regression, and record the fact that we did that thinking, in the log message. Thanks. I wanted to give a day or two or think about the explanation here. Does Thanks. mean you have picked up the patch and adjusted the commit message appropriately, or should I go with my original plan and resend it later with explanantion there? -- 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] clone: forbid --bare --separate-git-dir dir
Duy Nguyen pclo...@gmail.com writes: On Mon, Jan 7, 2013 at 6:13 AM, Junio C Hamano gits...@pobox.com wrote: I don't know what would go in the explanation here blank above, though. Is it possible that some people are relying on this option combination? I do not necessarily think we must say it happens not to work already for such and such reasons, lucky us!, but it is indeed a good idea to think things through, justifying why this cannot be a regression, and record the fact that we did that thinking, in the log message. Thanks. I wanted to give a day or two or think about the explanation here. Does Thanks. mean you have picked up the patch and adjusted the commit message appropriately, or should I go with my original plan and resend it later with explanantion there? The latter, Thanks for a review. I may have picked it up in 'pu', but that merely means, as usual, that I wanted to add a reminder in What's cooking to expect a reroll, and nothing more. 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
