Re: [PATCH 3/5] checkout --to: no auto-detach if the ref is already checked out
On Thu, Jul 31, 2014 at 1:03 AM, Junio C Hamano gits...@pobox.com wrote: Michael J Gruber g...@drmicha.warpmail.net writes: As an error message that is completely sufficient. The advice messages are meant to teach the user about the normal parts of the toolchest to use in a situation of conflict, aren't they? Not really. They are to remind (to those who learned but forgot) and to hint (to those who haven't realized they have things to learn in this area). Wall of text that tries to do more than that, like teaching, risks not getting read by anybody. Last call to all. Keep this 'advice' or drop it? -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Duy Nguyen pclo...@gmail.com writes: On Thu, Jul 31, 2014 at 1:03 AM, Junio C Hamano gits...@pobox.com wrote: Michael J Gruber g...@drmicha.warpmail.net writes: As an error message that is completely sufficient. The advice messages are meant to teach the user about the normal parts of the toolchest to use in a situation of conflict, aren't they? Not really. They are to remind (to those who learned but forgot) and to hint (to those who haven't realized they have things to learn in this area). Wall of text that tries to do more than that, like teaching, risks not getting read by anybody. Last call to all. Keep this 'advice' or drop it? I think you quoted what I needed to say already so I won't repeat myself. Earlier you said: If you have two checkouts associated to the same repo, git checkout foo on one checkout when foo is held by another checkout would cause the same error. I'll need to think of a better name than advice.checkoutTo. and I agree; the patch needs rerolling in any case. -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Michael J Gruber g...@drmicha.warpmail.net writes: As an error message that is completely sufficient. The advice messages are meant to teach the user about the normal parts of the toolchest to use in a situation of conflict, aren't they? Not really. They are to remind (to those who learned but forgot) and to hint (to those who haven't realized they have things to learn in this area). Wall of text that tries to do more than that, like teaching, risks not getting read by anybody. Maybe we should ask someone who hasn't turned them off... Actually, I run without most of the 'advice.*' turned off. -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Junio C Hamano gits...@pobox.com writes: Michael J Gruber g...@drmicha.warpmail.net writes: As an error message that is completely sufficient. The advice messages are meant to teach the user about the normal parts of the toolchest to use in a situation of conflict, aren't they? Not really. They are to remind (to those who learned but forgot) and to hint (to those who haven't realized they have things to learn in this area). Wall of text that tries to do more than that, like teaching, risks not getting read by anybody. Maybe we should ask someone who hasn't turned them off... Actually, I run without most of the 'advice.*' turned off. Ehh, what I meant was that I do not have advice.foo = false for most of 'foo', i.e. I run with most of them still active. -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Junio C Hamano venit, vidit, dixit 24.07.2014 23:30: Duy Nguyen pclo...@gmail.com writes: On Wed, Jul 23, 2014 at 8:48 PM, Michael J Gruber g...@drmicha.warpmail.net wrote: Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43: + if (advice_checkout_to) + die(_(%s is already checked out at %s.\n + Either use --detach or -b together with --to + or switch branch in the the other checkout.), or switch to a different branch in the other checkout. But maybe we can be even more helpful, like: %s is already checked out at %s.\n Either checkout the detached head of branch %s using\n git checkout --detach --to %s %s\n or checkout a new branch based off %s using\n git checkout --to %s -b %s newbranch %s\n or switch to a different branch in the other checkout. if we can figure out the appropriate arguments at this point in the code. We would not be able to construct the exact command that the user has entered, so perhaps git checkout --detach more options %s git checkout -b new branch more options %s ? Note that this does not only occur when --to is given. If you have two checkouts associated to the same repo, git checkout foo on one checkout when foo is held by another checkout would cause the same error. I'll need to think of a better name than advice.checkoutTo. I am not sure if we need to say anything more than the first line of the message you have in your patch. To fork a new branch at the commit the user is interested in to check it out, or not bothering with the branch and detach, are both very normal parts of user's Git toolchest, nothing particularly special. As long as the most important point, i.e. in the new world order, unlike the contrib/workdir hack, you cannot check out the same branch at two different places, is clearly conveyed and understood, everything else should follow naturally, no? As an error message that is completely sufficient. The advice messages are meant to teach the user about the normal parts of the toolchest to use in a situation of conflict, aren't they? Maybe we should ask someone who hasn't turned them off... Michael -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
On Wed, Jul 23, 2014 at 8:48 PM, Michael J Gruber g...@drmicha.warpmail.net wrote: Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43: + if (advice_checkout_to) + die(_(%s is already checked out at %s.\n + Either use --detach or -b together with --to + or switch branch in the the other checkout.), or switch to a different branch in the other checkout. But maybe we can be even more helpful, like: %s is already checked out at %s.\n Either checkout the detached head of branch %s using\n git checkout --detach --to %s %s\n or checkout a new branch based off %s using\n git checkout --to %s -b %s newbranch %s\n or switch to a different branch in the other checkout. if we can figure out the appropriate arguments at this point in the code. We would not be able to construct the exact command that the user has entered, so perhaps git checkout --detach more options %s git checkout -b new branch more options %s ? Note that this does not only occur when --to is given. If you have two checkouts associated to the same repo, git checkout foo on one checkout when foo is held by another checkout would cause the same error. I'll need to think of a better name than advice.checkoutTo. -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano gits...@pobox.com wrote:
+ if (strbuf_read_file(sb, path.buf, 0) = 0 ||
+ !skip_prefix(sb.buf, ref:, start))
+ goto done;
while (isspace(*start))
start++;
end = start;
while (*end !isspace(*end))
end++;
Not new in this round of update, and may not even be an issue, but:
- Earlier, the code returned early on a negative return value from
read-file (i.e., an error), but this round it also does so for
zero. Intended?
Yes. But it does not make any difference. strbuf_read_file returns
sb.len, if it's empty, the next skip_prefix would fail anyway.
- The above duplicates the logic in resolve_ref_unsafe() and
resolve_gitlink_ref_recursive(); three places now knows what a
textual symref looks like (i.e. begins with ref:, zero or more
whitespaces, the target ref and then zero or more trailing
whitespaces). Perhaps we need to consolidate the code further,
so that this knowledge does not leak out of refs.c?
OK. Will do (unless Mike has a different opinion about this).
+ if (strncmp(start, new-path, end - start) ||
+ new-path[end - start] != '\0')
+ goto done;
+ if (id) {
+ strbuf_reset(path);
+ strbuf_addf(path, %s/repos/%s/gitdir,
+ get_git_common_dir(), id);
+ if (strbuf_read_file(gitdir, path.buf, 0) = 0)
+ goto done;
+ while (gitdir.len (gitdir.buf[gitdir.len - 1] == '\n' ||
+ gitdir.buf[gitdir.len - 1] == '\r'))
+ gitdir.buf[--gitdir.len] = '\0';
Accepting arbitrary numbers of '\r' and '\n' sounds as if the code
is allowing it, but text editors would not end their files with a
nonsense sequence like \r\r\n\r unless the end-user works to do
so, and if you are prepared to be lenient to noisy human input, not
trimming trailing whitespaces does not look it is going far enough
to help them.
I do not see a good reason to allow random text editors to edit this
file in the first place, so my preference is:
if (strbuf_read_file(...) 0 ||
gitdir.len == 0 ||
gitdir.buf[gitdir.len - 1] != '\n')
goto error_return;
gitdir.buf[--gitdir.len] = '\0';
Alternatively, if you are trying to be lenient to human input, I
would understand:
if (strbuf_read_file(...) 0)
goto error_return;
strbuf_rtrim(gitdir);
The code in the patch, which is something in between, does not make
much sense to me.
I think more about echo abc $this_file where the echo command may
output '\r\n' on Windows (wild guess though, I don't use Windows
much). I think I'm going with _rtrim.
--
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 3/5] checkout --to: no auto-detach if the ref is already checked out
Duy Nguyen pclo...@gmail.com writes: On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano gits...@pobox.com wrote: + if (strbuf_read_file(sb, path.buf, 0) = 0 || + !skip_prefix(sb.buf, ref:, start)) + goto done; while (isspace(*start)) start++; end = start; while (*end !isspace(*end)) end++; Not new in this round of update, and may not even be an issue, but: - Earlier, the code returned early on a negative return value from read-file (i.e., an error), but this round it also does so for zero. Intended? Yes. But it does not make any difference. strbuf_read_file returns sb.len, if it's empty, the next skip_prefix would fail anyway. Yes but changing 0 to = 0 is inconsistent with that; I would understand if you changed it to = 4, which would be consistent with the reasoning, though. The code in the patch, which is something in between, does not make much sense to me. I think more about echo abc $this_file where the echo command may output '\r\n' on Windows (wild guess though, I don't use Windows much). I think I'm going with _rtrim. To expect 'echo' into the file is to expect and encourage that people muck with the internal implementation details by hand, which we do not generally do for things inside .git [*1*]. If we consider the contents of $this_file not an implementation detail but a part of the published API (i.e. writing this string into the file is a valid way to make Git do this), rtrim would at least be consistent with how the existing code deals with symrefs, so I wouldn't say does not make much sense if you are going in that direction ;-) [Footnote] *1* ... except for .git/config, to which we say It's a simple text file; don't be afraied to edit it away!. -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Junio C Hamano gits...@pobox.com writes: Duy Nguyen pclo...@gmail.com writes: On Thu, Jul 24, 2014 at 4:16 AM, Junio C Hamano gits...@pobox.com wrote: + if (strbuf_read_file(sb, path.buf, 0) = 0 || + !skip_prefix(sb.buf, ref:, start)) + goto done; while (isspace(*start)) start++; end = start; while (*end !isspace(*end)) end++; Not new in this round of update, and may not even be an issue, but: - Earlier, the code returned early on a negative return value from read-file (i.e., an error), but this round it also does so for zero. Intended? Yes. But it does not make any difference. strbuf_read_file returns sb.len, if it's empty, the next skip_prefix would fail anyway. Yes but changing 0 to = 0 is inconsistent with that; I would understand if you changed it to = 4, which would be consistent with the reasoning, though. Just to make sure. I am not saying changing 0 to = 4 is a good idea. I think checking for strbuf_read_file() failure with 0 and then checking for malformatted input (or detached head perhaps?) with !skip_prefix(), i.e. testing two logically separate things with two separate conditions concatenated together with ||, would be the most natural way to express it. -- 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Duy Nguyen pclo...@gmail.com writes: On Wed, Jul 23, 2014 at 8:48 PM, Michael J Gruber g...@drmicha.warpmail.net wrote: Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43: + if (advice_checkout_to) + die(_(%s is already checked out at %s.\n + Either use --detach or -b together with --to + or switch branch in the the other checkout.), or switch to a different branch in the other checkout. But maybe we can be even more helpful, like: %s is already checked out at %s.\n Either checkout the detached head of branch %s using\n git checkout --detach --to %s %s\n or checkout a new branch based off %s using\n git checkout --to %s -b %s newbranch %s\n or switch to a different branch in the other checkout. if we can figure out the appropriate arguments at this point in the code. We would not be able to construct the exact command that the user has entered, so perhaps git checkout --detach more options %s git checkout -b new branch more options %s ? Note that this does not only occur when --to is given. If you have two checkouts associated to the same repo, git checkout foo on one checkout when foo is held by another checkout would cause the same error. I'll need to think of a better name than advice.checkoutTo. I am not sure if we need to say anything more than the first line of the message you have in your patch. To fork a new branch at the commit the user is interested in to check it out, or not bothering with the branch and detach, are both very normal parts of user's Git toolchest, nothing particularly special. As long as the most important point, i.e. in the new world order, unlike the contrib/workdir hack, you cannot check out the same branch at two different places, is clearly conveyed and understood, everything else should follow naturally, 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 3/5] checkout --to: no auto-detach if the ref is already checked out
Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.07.2014 13:43:
Give the user a choice in this case. If they want to detach, they can go
with '--detach --to ...', or they could switch branch of the checkout
that's holding the ref in question. Or they could just create a new
branch with '-b xxx --to yyy'
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/config.txt | 3 ++
advice.c | 2 ++
advice.h | 1 +
builtin/checkout.c | 73
t/t2025-checkout-to.sh | 16 +--
5 files changed, 56 insertions(+), 39 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 57999fa..4a41202 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -201,6 +201,9 @@ advice.*::
rmHints::
In case of failure in the output of linkgit:git-rm[1],
show directions on how to proceed from the current state.
+ checkoutTo::
+ In case of failure in the output of git checkout --to,
+ show directions on how to proceed from the current state.
--
core.fileMode::
diff --git a/advice.c b/advice.c
index 9b42033..b1fb524 100644
--- a/advice.c
+++ b/advice.c
@@ -15,6 +15,7 @@ int advice_detached_head = 1;
int advice_set_upstream_failure = 1;
int advice_object_name_warning = 1;
int advice_rm_hints = 1;
+int advice_checkout_to = 1;
static struct {
const char *name;
@@ -35,6 +36,7 @@ static struct {
{ setupstreamfailure, advice_set_upstream_failure },
{ objectnamewarning, advice_object_name_warning },
{ rmhints, advice_rm_hints },
+ { checkoutto, advice_checkout_to },
/* make this an alias for backward compatibility */
{ pushnonfastforward, advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 5ecc6c1..a288219 100644
--- a/advice.h
+++ b/advice.h
@@ -18,6 +18,7 @@ extern int advice_detached_head;
extern int advice_set_upstream_failure;
extern int advice_object_name_warning;
extern int advice_rm_hints;
+extern int advice_checkout_to;
int git_default_advice_config(const char *var, const char *value);
__attribute__((format (printf, 1, 2)))
diff --git a/builtin/checkout.c b/builtin/checkout.c
index c83f476..d35245a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1006,31 +1006,52 @@ static const char *unique_tracking_name(const char
*name, unsigned char *sha1)
return NULL;
}
-static int check_linked_checkout(struct branch_info *new,
- const char *name, const char *path)
+static void check_linked_checkout(struct branch_info *new, const char *id)
{
struct strbuf sb = STRBUF_INIT;
+ struct strbuf path = STRBUF_INIT;
+ struct strbuf gitdir = STRBUF_INIT;
const char *start, *end;
- if (strbuf_read_file(sb, path, 0) 0 ||
- !skip_prefix(sb.buf, ref:, start)) {
- strbuf_release(sb);
- return 0;
- }
+ if (id)
+ strbuf_addf(path, %s/repos/%s/HEAD, get_git_common_dir(),
id);
+ else
+ strbuf_addf(path, %s/HEAD, get_git_common_dir());
+
+ if (strbuf_read_file(sb, path.buf, 0) = 0 ||
+ !skip_prefix(sb.buf, ref:, start))
+ goto done;
while (isspace(*start))
start++;
end = start;
while (*end !isspace(*end))
end++;
- if (!strncmp(start, new-path, end - start)
- new-path[end - start] == '\0') {
- strbuf_release(sb);
- new-path = NULL; /* detach */
- new-checkout = xstrdup(name); /* reason */
- return 1;
- }
+ if (strncmp(start, new-path, end - start) ||
+ new-path[end - start] != '\0')
+ goto done;
+ if (id) {
+ strbuf_reset(path);
+ strbuf_addf(path, %s/repos/%s/gitdir,
+ get_git_common_dir(), id);
+ if (strbuf_read_file(gitdir, path.buf, 0) = 0)
+ goto done;
+ while (gitdir.len (gitdir.buf[gitdir.len - 1] == '\n' ||
+ gitdir.buf[gitdir.len - 1] == '\r'))
+ gitdir.buf[--gitdir.len] = '\0';
+ } else
+ strbuf_addstr(gitdir, get_git_common_dir());
+ if (advice_checkout_to)
+ die(_(%s is already checked out at %s.\n
+ Either use --detach or -b together with --to
+ or switch branch in the the other checkout.),
or switch to a different branch in the other checkout. But maybe we
can be even more helpful, like:
%s is already checked out at %s.\n
Either checkout the detached head of branch %s using\n
git checkout --detach --to %s %s\n
or checkout a new branch based off %s using\n
git checkout --to %s -b %s newbranch %s\n
or switch to a
Re: [PATCH 3/5] checkout --to: no auto-detach if the ref is already checked out
Michael J Gruber g...@drmicha.warpmail.net writes: +if (advice_checkout_to) +die(_(%s is already checked out at %s.\n + Either use --detach or -b together with --to + or switch branch in the the other checkout.), or switch to a different branch in the other checkout. But maybe we can be even more helpful, like: %s is already checked out at %s.\n Either checkout the detached head of branch %s using\n git checkout --detach --to %s %s\n or checkout a new branch based off %s using\n git checkout --to %s -b %s newbranch %s\n or switch to a different branch in the other checkout. if we can figure out the appropriate arguments at this point in the code. Another possible alternative is go and work there instead of creating yet another checkout, but is that too obvious without saying? -- 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
