Re: [PATCH v3] branch: show rebase/bisect info when possible instead of "(no branch)"
On Sat, Feb 9, 2013 at 1:35 AM, Junio C Hamano wrote: > We may want to refactor wt_status_print_state() and its callee a bit > so that it and this part can share the logic without duplication and > risk implementing subtly different decision. wt_status used to have > clean separation between collection phase and presentation phase, > but the wall between the phases seems deteriorated over time as more > "in progress" crufts have been piled on top. > > Such a refactoring may look larger than necessary, but on the other > hand, I do not see this feature very useful if it can over time > drift away from what we will see in "git status", so... OK. I'll wait until nd/status-show-in-progress is merged to master, then rework this patch on top. -- 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] branch: show rebase/bisect info when possible instead of "(no branch)"
Nguyễn Thái Ngọc Duy writes:
> +static char *get_head_description()
> +{
> + struct stat st;
> + struct strbuf sb = STRBUF_INIT;
> + struct strbuf result = STRBUF_INIT;
> + int bisect = 0;
> + int ret;
> + if (!stat(git_path("rebase-merge"), &st) && S_ISDIR(st.st_mode))
> + ret = strbuf_read_file(&sb, git_path("rebase-merge/head-name"),
> 0);
Hrmph. Why isn't this checking if the file exists and then read it,
i.e.
if (access(git_path("rebase-merge/head-name"), F_OK))
ret = strbuf_read_file(&sb, git_path("rebase-merge/head-name"),
0);
It is not like you are creating this file and making sure leading
directories exist, so the sequence looks a bit strange.
> + else if (!access(git_path("rebase-apply/rebasing"), F_OK))
> + ret = strbuf_read_file(&sb, git_path("rebase-apply/head-name"),
> 0);
> + else if (!access(git_path("BISECT_LOG"), F_OK)) {
> + ret = strbuf_read_file(&sb, git_path("BISECT_START"), 0);
> + bisect = 1;
And if the answer to the above question is "because if rebase-merge/
exists, with or without head-name, we know we are not bisecting",
then that may suggest that the structure of if/elseif cascade is
misdesigned. Shouldn't the "bisect" boolean be an enum "what are we
doing" that is initialized to "I do not know" and each of these
if/elseif cascade set the state to it when they know what we are
doing, in order for this function to be longer-term maintainable?
--
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] branch: show rebase/bisect info when possible instead of "(no branch)"
Nguyễn Thái Ngọc Duy writes:
> This prints more helpful info when HEAD is detached: is it detached
> because of bisect or rebase? What is the original branch name in those
> cases?
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Keep "no branch" in all cases. Just append "rebasing/bisecting [%s]"
> when applicable.
>
> builtin/branch.c| 44 +++-
> t/t6030-bisect-porcelain.sh | 2 +-
> 2 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6371bf9..26c0c3d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -550,6 +550,48 @@ static int calc_maxwidth(struct ref_list *refs)
> return w;
> }
>
> +static char *get_head_description()
> +{
> + struct stat st;
> + struct strbuf sb = STRBUF_INIT;
> + struct strbuf result = STRBUF_INIT;
> + int bisect = 0;
> + int ret;
> + if (!stat(git_path("rebase-merge"), &st) && S_ISDIR(st.st_mode))
> + ret = strbuf_read_file(&sb, git_path("rebase-merge/head-name"),
> 0);
> + else if (!access(git_path("rebase-apply/rebasing"), F_OK))
> + ret = strbuf_read_file(&sb, git_path("rebase-apply/head-name"),
> 0);
> + else if (!access(git_path("BISECT_LOG"), F_OK)) {
> + ret = strbuf_read_file(&sb, git_path("BISECT_START"), 0);
> + bisect = 1;
> + } else
> + return xstrdup(_("(no branch)"));
> +
> + if (ret <= 0) {
Doesn't the general "negative signals an error" apply here?
The end result may be the same, as the later part of this function
that uses sb with len==0 ends up showing the same "bisecting" (or
"rebasing") without any other information, but the logic to reach
that outcome looks wrong.
> + if (bisect)
> + return xstrdup(_("(no branch, bisecting)"));
> + else
> + return xstrdup(_("_(no branch, rebasing)"));
> + }
> +
> + while (sb.len && sb.buf[sb.len - 1] == '\n')
> + strbuf_setlen(&sb, sb.len - 1);
> +
> + if (bisect) {
> + unsigned char sha1[20];
> + if (!get_sha1_hex(sb.buf, sha1))
> + strbuf_addstr(&result, _("(no branch, bisecting)"));
> + else
> + strbuf_addf(&result, _("(no branch, bisecting %s)"),
> sb.buf);
> + } else {
> + if (!prefixcmp(sb.buf, "refs/heads/"))
> + strbuf_addf(&result, _("(no branch, rebasing %s)"),
> sb.buf + 11);
> + else
> + strbuf_addstr(&result, _("(no branch, rebasing)"));
> + }
> + strbuf_release(&sb);
> + return strbuf_detach(&result, NULL);
> +}
We may want to refactor wt_status_print_state() and its callee a bit
so that it and this part can share the logic without duplication and
risk implementing subtly different decision. wt_status used to have
clean separation between collection phase and presentation phase,
but the wall between the phases seems deteriorated over time as more
"in progress" crufts have been piled on top.
Such a refactoring may look larger than necessary, but on the other
hand, I do not see this feature very useful if it can over time
drift away from what we will see in "git status", so...
>
> static void show_detached(struct ref_list *ref_list)
> {
> @@ -557,7 +599,7 @@ static void show_detached(struct ref_list *ref_list)
>
> if (head_commit && is_descendant_of(head_commit,
> ref_list->with_commit)) {
> struct ref_item item;
> - item.name = xstrdup(_("(no branch)"));
> + item.name = get_head_description();
> item.width = utf8_strwidth(item.name);
> item.kind = REF_LOCAL_BRANCH;
> item.dest = NULL;
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 3e0e15f..9b6f0d0 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -164,7 +164,7 @@ test_expect_success 'bisect start: existing
> ".git/BISECT_START" not modified if
> cp .git/BISECT_START saved &&
> test_must_fail git bisect start $HASH4 foo -- &&
> git branch > branch.output &&
> - test_i18ngrep "* (no branch)" branch.output > /dev/null &&
> + test_i18ngrep "* (no branch, bisecting other)" branch.output >
> /dev/null &&
> test_cmp saved .git/BISECT_START
> '
> test_expect_success 'bisect start: no ".git/BISECT_START" if mistaken rev' '
--
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] branch: show rebase/bisect info when possible instead of "(no branch)"
This prints more helpful info when HEAD is detached: is it detached
because of bisect or rebase? What is the original branch name in those
cases?
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Keep "no branch" in all cases. Just append "rebasing/bisecting [%s]"
when applicable.
builtin/branch.c| 44 +++-
t/t6030-bisect-porcelain.sh | 2 +-
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6371bf9..26c0c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -550,6 +550,48 @@ static int calc_maxwidth(struct ref_list *refs)
return w;
}
+static char *get_head_description()
+{
+ struct stat st;
+ struct strbuf sb = STRBUF_INIT;
+ struct strbuf result = STRBUF_INIT;
+ int bisect = 0;
+ int ret;
+ if (!stat(git_path("rebase-merge"), &st) && S_ISDIR(st.st_mode))
+ ret = strbuf_read_file(&sb, git_path("rebase-merge/head-name"),
0);
+ else if (!access(git_path("rebase-apply/rebasing"), F_OK))
+ ret = strbuf_read_file(&sb, git_path("rebase-apply/head-name"),
0);
+ else if (!access(git_path("BISECT_LOG"), F_OK)) {
+ ret = strbuf_read_file(&sb, git_path("BISECT_START"), 0);
+ bisect = 1;
+ } else
+ return xstrdup(_("(no branch)"));
+
+ if (ret <= 0) {
+ if (bisect)
+ return xstrdup(_("(no branch, bisecting)"));
+ else
+ return xstrdup(_("_(no branch, rebasing)"));
+ }
+
+ while (sb.len && sb.buf[sb.len - 1] == '\n')
+ strbuf_setlen(&sb, sb.len - 1);
+
+ if (bisect) {
+ unsigned char sha1[20];
+ if (!get_sha1_hex(sb.buf, sha1))
+ strbuf_addstr(&result, _("(no branch, bisecting)"));
+ else
+ strbuf_addf(&result, _("(no branch, bisecting %s)"),
sb.buf);
+ } else {
+ if (!prefixcmp(sb.buf, "refs/heads/"))
+ strbuf_addf(&result, _("(no branch, rebasing %s)"),
sb.buf + 11);
+ else
+ strbuf_addstr(&result, _("(no branch, rebasing)"));
+ }
+ strbuf_release(&sb);
+ return strbuf_detach(&result, NULL);
+}
static void show_detached(struct ref_list *ref_list)
{
@@ -557,7 +599,7 @@ static void show_detached(struct ref_list *ref_list)
if (head_commit && is_descendant_of(head_commit,
ref_list->with_commit)) {
struct ref_item item;
- item.name = xstrdup(_("(no branch)"));
+ item.name = get_head_description();
item.width = utf8_strwidth(item.name);
item.kind = REF_LOCAL_BRANCH;
item.dest = NULL;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 3e0e15f..9b6f0d0 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -164,7 +164,7 @@ test_expect_success 'bisect start: existing
".git/BISECT_START" not modified if
cp .git/BISECT_START saved &&
test_must_fail git bisect start $HASH4 foo -- &&
git branch > branch.output &&
- test_i18ngrep "* (no branch)" branch.output > /dev/null &&
+ test_i18ngrep "* (no branch, bisecting other)" branch.output >
/dev/null &&
test_cmp saved .git/BISECT_START
'
test_expect_success 'bisect start: no ".git/BISECT_START" if mistaken rev' '
--
1.8.1.2.536.gf441e6d
--
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
