* [PATCH] branch: let '--edit-description' default to rebased branch during rebase @ 2020-01-11 12:35 marcandre.lureau 2020-01-11 13:26 ` Eric Sunshine 0 siblings, 1 reply; 20+ messages in thread From: marcandre.lureau @ 2020-01-11 12:35 UTC (permalink / raw) To: git; +Cc: szeder.dev, Marc-André Lureau From: Marc-André Lureau <marcandre.lureau@redhat.com> Defaulting to editing the description of the rebased branch without an explicit branchname argument would be useful. Even the git bash prompt shows the name of the rebased branch, and then ~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description fatal: Cannot give description to detached HEAD looks quite unhelpful. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- Changed in v2: - add a test - fix commit message & summary - simplify code builtin/branch.c | 24 +++++++++++++++++++----- t/t3200-branch.sh | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index d8297f80ff..ee82dc828e 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -745,15 +745,27 @@ int cmd_branch(int argc, const char **argv, const char *prefix) string_list_clear(&output, 0); return 0; } else if (edit_description) { - const char *branch_name; + char *branch_name = NULL; struct strbuf branch_ref = STRBUF_INIT; if (!argc) { - if (filter.detached) - die(_("Cannot give description to detached HEAD")); - branch_name = head; + if (filter.detached) { + struct wt_status_state state; + + memset(&state, 0, sizeof(state)); + + if (wt_status_check_rebase(NULL, &state)) { + branch_name = state.branch; + } + + if (!branch_name) + die(_("Cannot give description to detached HEAD")); + + free(state.onto); + } else + branch_name = xstrdup(head); } else if (argc == 1) - branch_name = argv[0]; + branch_name = xstrdup(argv[0]); else die(_("cannot edit description of more than one branch")); @@ -772,6 +784,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (edit_branch_description(branch_name)) return 1; + + free(branch_name); } else if (copy) { if (!argc) die(_("branch name required")); diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 411a70b0ce..a20ebedea0 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -1260,6 +1260,25 @@ test_expect_success 'use --edit-description' ' test_cmp expect EDITOR_OUTPUT ' +test_expect_success 'use --edit-description during rebase' ' + write_script editor <<-\EOF && + echo "Rebase contents" >"$1" + EOF + ( + set_fake_editor && + FAKE_LINES="break 1" git rebase -i HEAD^ && + EDITOR=./editor git branch --edit-description && + git rebase --continue + ) && + write_script editor <<-\EOF && + git stripspace -s <"$1" >"EDITOR_OUTPUT" + EOF + EDITOR=./editor git branch --edit-description && + echo "Rebase contents" >expect && + test_cmp expect EDITOR_OUTPUT +' +test_done + test_expect_success 'detect typo in branch name when using --edit-description' ' write_script editor <<-\EOF && echo "New contents" >"$1" base-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0 -- 2.25.0.rc2.2.g5aece98438 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-11 12:35 [PATCH] branch: let '--edit-description' default to rebased branch during rebase marcandre.lureau @ 2020-01-11 13:26 ` Eric Sunshine 2020-01-11 14:54 ` Marc-André Lureau 0 siblings, 1 reply; 20+ messages in thread From: Eric Sunshine @ 2020-01-11 13:26 UTC (permalink / raw) To: marcandre.lureau; +Cc: Git List, SZEDER Gábor On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote: > Defaulting to editing the description of the rebased branch without an > explicit branchname argument would be useful. Even the git bash prompt > shows the name of the rebased branch, and then > > ~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description > fatal: Cannot give description to detached HEAD > > looks quite unhelpful. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > diff --git a/builtin/branch.c b/builtin/branch.c > @@ -745,15 +745,27 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > if (!argc) { > - if (filter.detached) > - die(_("Cannot give description to detached HEAD")); > - branch_name = head; > + if (filter.detached) { > + struct wt_status_state state; > + > + memset(&state, 0, sizeof(state)); > + > + if (wt_status_check_rebase(NULL, &state)) { > + branch_name = state.branch; > + } Style: drop unneeded braces. > + > + if (!branch_name) > + die(_("Cannot give description to detached HEAD")); > + > + free(state.onto); Also, no need for all the blank lines which eat up valuable vertical screen real-estate without making the code clearer. > + } else > + branch_name = xstrdup(head); It would be easier to see what happens in the common case (when not rebasing) if you invert the condition to `if (!filter.detached)` and turn this one-line 'else' branch into the 'if' branch. > @@ -772,6 +784,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > if (edit_branch_description(branch_name)) > return 1; > + > + free(branch_name); That `return 1` just above this free() is leaking 'branch_name', isn't it? > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > @@ -1260,6 +1260,25 @@ test_expect_success 'use --edit-description' ' > +test_expect_success 'use --edit-description during rebase' ' > + write_script editor <<-\EOF && > + echo "Rebase contents" >"$1" > + EOF > + ( > + set_fake_editor && > + FAKE_LINES="break 1" git rebase -i HEAD^ && > + EDITOR=./editor git branch --edit-description && > + git rebase --continue > + ) && > + write_script editor <<-\EOF && > + git stripspace -s <"$1" >"EDITOR_OUTPUT" > + EOF > + EDITOR=./editor git branch --edit-description && > + echo "Rebase contents" >expect && > + test_cmp expect EDITOR_OUTPUT > +' > +test_done Strange place for a test_done() invocation considering that existing tests follow the new one added by this patch. > test_expect_success 'detect typo in branch name when using --edit-description' ' > write_script editor <<-\EOF && > echo "New contents" >"$1" ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-11 13:26 ` Eric Sunshine @ 2020-01-11 14:54 ` Marc-André Lureau 2020-01-12 1:27 ` Eric Sunshine 0 siblings, 1 reply; 20+ messages in thread From: Marc-André Lureau @ 2020-01-11 14:54 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, SZEDER Gábor Hi On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote: > > Defaulting to editing the description of the rebased branch without an > > explicit branchname argument would be useful. Even the git bash prompt > > shows the name of the rebased branch, and then > > > > ~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description > > fatal: Cannot give description to detached HEAD > > > > looks quite unhelpful. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > diff --git a/builtin/branch.c b/builtin/branch.c > > @@ -745,15 +745,27 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > > if (!argc) { > > - if (filter.detached) > > - die(_("Cannot give description to detached HEAD")); > > - branch_name = head; > > + if (filter.detached) { > > + struct wt_status_state state; > > + > > + memset(&state, 0, sizeof(state)); > > + > > + if (wt_status_check_rebase(NULL, &state)) { > > + branch_name = state.branch; > > + } > > Style: drop unneeded braces. ok > > > + > > + if (!branch_name) > > + die(_("Cannot give description to detached HEAD")); > > + > > + free(state.onto); > > Also, no need for all the blank lines which eat up valuable vertical > screen real-estate without making the code clearer. ok > > > + } else > > + branch_name = xstrdup(head); > > It would be easier to see what happens in the common case (when not > rebasing) if you invert the condition to `if (!filter.detached)` and > turn this one-line 'else' branch into the 'if' branch. indeed > > > @@ -772,6 +784,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > > if (edit_branch_description(branch_name)) > > return 1; > > + > > + free(branch_name); > > That `return 1` just above this free() is leaking 'branch_name', isn't it? right, let's fix that too > > > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > > @@ -1260,6 +1260,25 @@ test_expect_success 'use --edit-description' ' > > +test_expect_success 'use --edit-description during rebase' ' > > + write_script editor <<-\EOF && > > + echo "Rebase contents" >"$1" > > + EOF > > + ( > > + set_fake_editor && > > + FAKE_LINES="break 1" git rebase -i HEAD^ && > > + EDITOR=./editor git branch --edit-description && > > + git rebase --continue > > + ) && > > + write_script editor <<-\EOF && > > + git stripspace -s <"$1" >"EDITOR_OUTPUT" > > + EOF > > + EDITOR=./editor git branch --edit-description && > > + echo "Rebase contents" >expect && > > + test_cmp expect EDITOR_OUTPUT > > +' > > +test_done > > Strange place for a test_done() invocation considering that existing > tests follow the new one added by this patch. doh, sorry thanks for the review! > > > test_expect_success 'detect typo in branch name when using --edit-description' ' > > write_script editor <<-\EOF && > > echo "New contents" >"$1" -- Marc-André Lureau ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-11 14:54 ` Marc-André Lureau @ 2020-01-12 1:27 ` Eric Sunshine 2020-01-12 6:44 ` Marc-André Lureau 2020-01-12 12:14 ` SZEDER Gábor 0 siblings, 2 replies; 20+ messages in thread From: Eric Sunshine @ 2020-01-12 1:27 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Git List, SZEDER Gábor On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote: > > > + if (wt_status_check_rebase(NULL, &state)) { > > > + branch_name = state.branch; > > > + } Taking a deeper look at the code, I'm wondering it would make more sense to call wt_status_get_state(), which handles 'rebase' and 'bisect'. Is there a reason that you limited this check to only 'rebase'? > > > if (edit_branch_description(branch_name)) > > > return 1; > > > + > > > + free(branch_name); > > > > That `return 1` just above this free() is leaking 'branch_name', isn't it? > > right, let's fix that too Looking at the code itself (rather than consulting only the patch), I see that there are a couple more early returns leaking 'branch_name', so they need to be handled, as well. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-12 1:27 ` Eric Sunshine @ 2020-01-12 6:44 ` Marc-André Lureau 2020-01-12 12:14 ` SZEDER Gábor 1 sibling, 0 replies; 20+ messages in thread From: Marc-André Lureau @ 2020-01-12 6:44 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, SZEDER Gábor Hi Eric On Sun, Jan 12, 2020 at 5:27 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote: > > > > + if (wt_status_check_rebase(NULL, &state)) { > > > > + branch_name = state.branch; > > > > + } > > Taking a deeper look at the code, I'm wondering it would make more > sense to call wt_status_get_state(), which handles 'rebase' and > 'bisect'. Is there a reason that you limited this check to only > 'rebase'? No reason, I just didn't try it yet. Done, thanks > > > > > if (edit_branch_description(branch_name)) > > > > return 1; > > > > + > > > > + free(branch_name); > > > > > > That `return 1` just above this free() is leaking 'branch_name', isn't it? > > > > right, let's fix that too > > Looking at the code itself (rather than consulting only the patch), I > see that there are a couple more early returns leaking 'branch_name', > so they need to be handled, as well. I think I covered them now, sending v4. thanks -- Marc-André Lureau ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-12 1:27 ` Eric Sunshine 2020-01-12 6:44 ` Marc-André Lureau @ 2020-01-12 12:14 ` SZEDER Gábor 2020-01-13 1:59 ` Eric Sunshine 1 sibling, 1 reply; 20+ messages in thread From: SZEDER Gábor @ 2020-01-12 12:14 UTC (permalink / raw) To: Eric Sunshine; +Cc: Marc-André Lureau, Git List On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > On Sat, Jan 11, 2020 at 9:55 AM Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > On Sat, Jan 11, 2020 at 5:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > > On Sat, Jan 11, 2020 at 7:36 AM <marcandre.lureau@redhat.com> wrote: > > > > + if (wt_status_check_rebase(NULL, &state)) { > > > > + branch_name = state.branch; > > > > + } > > Taking a deeper look at the code, I'm wondering it would make more > sense to call wt_status_get_state(), which handles 'rebase' and > 'bisect'. Is there a reason that you limited this check to only > 'rebase'? While I do think that defaulting to edit the description of the rebased branch makes sense, I'm not sure how that would work with bisect. What branch name does wt_status_get_state() return while bisecting? The branch where I started from? Because that's what 'git status' shows: ~/src/git (mybranch)$ git bisect start v2.21.0 v2.20.0 Bisecting: 334 revisions left to test after this (roughly 8 steps) [b99a579f8e434a7757f90895945b5711b3f159d5] Merge branch 'sb/more-repo-in-api' ~/src/git ((b99a579f8e...)|BISECTING)$ git status HEAD detached at b99a579f8e You are currently bisecting, started from branch 'mybranch'. (use "git bisect reset" to get back to the original branch) nothing to commit, working tree clean But am I really on that branch? Does it really makes sense to edit the description of 'mybranch' by default while bisecting through an old revision range? I do not think so. > > > > if (edit_branch_description(branch_name)) > > > > return 1; > > > > + > > > > + free(branch_name); > > > > > > That `return 1` just above this free() is leaking 'branch_name', isn't it? > > > > right, let's fix that too > > Looking at the code itself (rather than consulting only the patch), I > see that there are a couple more early returns leaking 'branch_name', > so they need to be handled, as well. 'git branch --edit-description' is a one-shot operation: it allows to edit only one branch description per invocation, and then the process exits right away, whether the operation was successful or some error occurred. I'm not sure free()ing 'branch_name' is worth the effort (and even if it does, I think it should be a separate preparatory patch). ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-12 12:14 ` SZEDER Gábor @ 2020-01-13 1:59 ` Eric Sunshine 2020-01-24 22:41 ` SZEDER Gábor 0 siblings, 1 reply; 20+ messages in thread From: Eric Sunshine @ 2020-01-13 1:59 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Marc-André Lureau, Git List On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > Taking a deeper look at the code, I'm wondering it would make more > > sense to call wt_status_get_state(), which handles 'rebase' and > > 'bisect'. Is there a reason that you limited this check to only > > 'rebase'? > > What branch name does wt_status_get_state() return while bisecting? > The branch where I started from? Because that's what 'git status' > shows: > But am I really on that branch? Does it really makes sense to edit > the description of 'mybranch' by default while bisecting through an > old revision range? I do not think so. It's not clear what downside you are pointing out; i.e. why would it be a bad thing to be able to set the branch description even while bisecting -- especially since `git status` affirms that it knows the branch? > > Looking at the code itself (rather than consulting only the patch), I > > see that there are a couple more early returns leaking 'branch_name', > > so they need to be handled, as well. > > 'git branch --edit-description' is a one-shot operation: it allows to > edit only one branch description per invocation, and then the process > exits right away, whether the operation was successful or some error > occurred. It is one-shot, but the existing `--edit-description` code already cleans up after itself by releasing resources it allocated (as do other one-shot parts of cmd_branch()), so it would be odd and inconsistent for this new code to not clean up after itself, as well (or, more accurately, to only clean up after itself in some branches but not others). > I'm not sure free()ing 'branch_name' is worth the effort > (and even if it does, I think it should be a separate preparatory > patch). A separate preparatory patch doesn't make sense in this case since 'branch_name' becomes "freeable" with this patch itself (prior to that, it was `const char *`). Anyhow, a different approach was later proposed[1] which eliminates some of the ugliness. [1]: https://lore.kernel.org/git/20200112101735.GA19676@flurp.local/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-13 1:59 ` Eric Sunshine @ 2020-01-24 22:41 ` SZEDER Gábor 2020-01-30 21:37 ` Marc-André Lureau 0 siblings, 1 reply; 20+ messages in thread From: SZEDER Gábor @ 2020-01-24 22:41 UTC (permalink / raw) To: Eric Sunshine; +Cc: Marc-André Lureau, Git List On Sun, Jan 12, 2020 at 08:59:04PM -0500, Eric Sunshine wrote: > On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > Taking a deeper look at the code, I'm wondering it would make more > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > 'bisect'. Is there a reason that you limited this check to only > > > 'rebase'? > > > > What branch name does wt_status_get_state() return while bisecting? > > The branch where I started from? Because that's what 'git status' > > shows: > > But am I really on that branch? Does it really makes sense to edit > > the description of 'mybranch' by default while bisecting through an > > old revision range? I do not think so. > > It's not clear what downside you are pointing out; i.e. why would it > be a bad thing to be able to set the branch description even while > bisecting -- especially since `git status` affirms that it knows the > branch? No, during a bisect operation 'git status' knows the branch where I _was_ when I started bisecting, and where a 'git bisect reset' will eventually bring me back when I'm finished, and that has no relation whatsoever to the revision range that I'm bisecting. Consider this case: $ git checkout --orphan unrelated-history Switched to a new branch 'unrelated-history' $ git commit -m "test" [unrelated-history (root-commit) 639b9d1047] test <...> $ git bisect start v2.25.0 v2.24.0 Bisecting: 361 revisions left to test after this (roughly 9 steps) [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 $ git status HEAD detached at 7034cd094b You are currently bisecting, started from branch 'unrelated-history'. (use "git bisect reset" to get back to the original branch) nothing to commit, working tree clean I can't possible be on branch 'unrelated-history' during that bisection. OTOH, while during a rebase we are technically on a detached HEAD as well, that rebase operation is all about constructing the new history of the rebased branch, and once finished that branch will be updated to point to the tip of the new history, thus it will include all the commits created while on the detached HEAD. Therefore, it makes sense conceptually to treat it as if we were on the rebased branch. That's why it makes sense to display the name of the rebased branch in the Bash prompt, and that's why I think it makes sense to default to edit the description of the rebased branch without explicitly naming it. With bisect that just doesn't make sense. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-24 22:41 ` SZEDER Gábor @ 2020-01-30 21:37 ` Marc-André Lureau 2020-01-31 15:52 ` SZEDER Gábor 0 siblings, 1 reply; 20+ messages in thread From: Marc-André Lureau @ 2020-01-30 21:37 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Eric Sunshine, Git List Hi On Fri, Jan 24, 2020 at 11:41 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Sun, Jan 12, 2020 at 08:59:04PM -0500, Eric Sunshine wrote: > > On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > > Taking a deeper look at the code, I'm wondering it would make more > > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > > 'bisect'. Is there a reason that you limited this check to only > > > > 'rebase'? > > > > > > What branch name does wt_status_get_state() return while bisecting? > > > The branch where I started from? Because that's what 'git status' > > > shows: > > > But am I really on that branch? Does it really makes sense to edit > > > the description of 'mybranch' by default while bisecting through an > > > old revision range? I do not think so. > > > > It's not clear what downside you are pointing out; i.e. why would it > > be a bad thing to be able to set the branch description even while > > bisecting -- especially since `git status` affirms that it knows the > > branch? > > No, during a bisect operation 'git status' knows the branch where I > _was_ when I started bisecting, and where a 'git bisect reset' will > eventually bring me back when I'm finished, and that has no relation > whatsoever to the revision range that I'm bisecting. > > Consider this case: > > $ git checkout --orphan unrelated-history > Switched to a new branch 'unrelated-history' > $ git commit -m "test" > [unrelated-history (root-commit) 639b9d1047] test > <...> > $ git bisect start v2.25.0 v2.24.0 > Bisecting: 361 revisions left to test after this (roughly 9 steps) > [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 > $ git status > HEAD detached at 7034cd094b > You are currently bisecting, started from branch 'unrelated-history'. > (use "git bisect reset" to get back to the original branch) > > nothing to commit, working tree clean > > I can't possible be on branch 'unrelated-history' during that > bisection. > > > OTOH, while during a rebase we are technically on a detached HEAD as > well, that rebase operation is all about constructing the new history > of the rebased branch, and once finished that branch will be updated > to point to the tip of the new history, thus it will include all the > commits created while on the detached HEAD. Therefore, it makes sense > conceptually to treat it as if we were on the rebased branch. That's > why it makes sense to display the name of the rebased branch in the > Bash prompt, and that's why I think it makes sense to default to edit > the description of the rebased branch without explicitly naming it. > > With bisect that just doesn't make sense. If the range you are bisecting belongs or lead to the current branch, that still makes sense. And it's probably most of the time. So, I am not sure your objection is valid enough here. -- Marc-André Lureau ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-30 21:37 ` Marc-André Lureau @ 2020-01-31 15:52 ` SZEDER Gábor 2020-01-31 15:59 ` Marc-André Lureau 0 siblings, 1 reply; 20+ messages in thread From: SZEDER Gábor @ 2020-01-31 15:52 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Eric Sunshine, Git List On Thu, Jan 30, 2020 at 10:37:38PM +0100, Marc-André Lureau wrote: > Hi > > On Fri, Jan 24, 2020 at 11:41 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > On Sun, Jan 12, 2020 at 08:59:04PM -0500, Eric Sunshine wrote: > > > On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > > > Taking a deeper look at the code, I'm wondering it would make more > > > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > > > 'bisect'. Is there a reason that you limited this check to only > > > > > 'rebase'? > > > > > > > > What branch name does wt_status_get_state() return while bisecting? > > > > The branch where I started from? Because that's what 'git status' > > > > shows: > > > > But am I really on that branch? Does it really makes sense to edit > > > > the description of 'mybranch' by default while bisecting through an > > > > old revision range? I do not think so. > > > > > > It's not clear what downside you are pointing out; i.e. why would it > > > be a bad thing to be able to set the branch description even while > > > bisecting -- especially since `git status` affirms that it knows the > > > branch? > > > > No, during a bisect operation 'git status' knows the branch where I > > _was_ when I started bisecting, and where a 'git bisect reset' will > > eventually bring me back when I'm finished, and that has no relation > > whatsoever to the revision range that I'm bisecting. > > > > Consider this case: > > > > $ git checkout --orphan unrelated-history > > Switched to a new branch 'unrelated-history' > > $ git commit -m "test" > > [unrelated-history (root-commit) 639b9d1047] test > > <...> > > $ git bisect start v2.25.0 v2.24.0 > > Bisecting: 361 revisions left to test after this (roughly 9 steps) > > [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 > > $ git status > > HEAD detached at 7034cd094b > > You are currently bisecting, started from branch 'unrelated-history'. > > (use "git bisect reset" to get back to the original branch) > > > > nothing to commit, working tree clean > > > > I can't possible be on branch 'unrelated-history' during that > > bisection. > > > > > > OTOH, while during a rebase we are technically on a detached HEAD as > > well, that rebase operation is all about constructing the new history > > of the rebased branch, and once finished that branch will be updated > > to point to the tip of the new history, thus it will include all the > > commits created while on the detached HEAD. Therefore, it makes sense > > conceptually to treat it as if we were on the rebased branch. That's > > why it makes sense to display the name of the rebased branch in the > > Bash prompt, and that's why I think it makes sense to default to edit > > the description of the rebased branch without explicitly naming it. > > > > With bisect that just doesn't make sense. > > If the range you are bisecting belongs or lead to the current branch, > that still makes sense. And it's probably most of the time. So, I am > not sure your objection is valid enough here. I'm not sure what you mean with "belongs or lead to" a branch. Do you mean that the range is reachable from the branch that just so happened to be checked out when the bisection was started? Well, I have over 30 branches from where v2.25.0 is reachable, and all of them are obviously bad candidates for editing their descriptions by default while bisecting a totally unrelated issue. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-31 15:52 ` SZEDER Gábor @ 2020-01-31 15:59 ` Marc-André Lureau 2020-01-31 16:16 ` SZEDER Gábor 0 siblings, 1 reply; 20+ messages in thread From: Marc-André Lureau @ 2020-01-31 15:59 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Eric Sunshine, Git List Hi On Fri, Jan 31, 2020 at 4:52 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Thu, Jan 30, 2020 at 10:37:38PM +0100, Marc-André Lureau wrote: > > Hi > > > > On Fri, Jan 24, 2020 at 11:41 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > > On Sun, Jan 12, 2020 at 08:59:04PM -0500, Eric Sunshine wrote: > > > > On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > > > > Taking a deeper look at the code, I'm wondering it would make more > > > > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > > > > 'bisect'. Is there a reason that you limited this check to only > > > > > > 'rebase'? > > > > > > > > > > What branch name does wt_status_get_state() return while bisecting? > > > > > The branch where I started from? Because that's what 'git status' > > > > > shows: > > > > > But am I really on that branch? Does it really makes sense to edit > > > > > the description of 'mybranch' by default while bisecting through an > > > > > old revision range? I do not think so. > > > > > > > > It's not clear what downside you are pointing out; i.e. why would it > > > > be a bad thing to be able to set the branch description even while > > > > bisecting -- especially since `git status` affirms that it knows the > > > > branch? > > > > > > No, during a bisect operation 'git status' knows the branch where I > > > _was_ when I started bisecting, and where a 'git bisect reset' will > > > eventually bring me back when I'm finished, and that has no relation > > > whatsoever to the revision range that I'm bisecting. > > > > > > Consider this case: > > > > > > $ git checkout --orphan unrelated-history > > > Switched to a new branch 'unrelated-history' > > > $ git commit -m "test" > > > [unrelated-history (root-commit) 639b9d1047] test > > > <...> > > > $ git bisect start v2.25.0 v2.24.0 > > > Bisecting: 361 revisions left to test after this (roughly 9 steps) > > > [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 > > > $ git status > > > HEAD detached at 7034cd094b > > > You are currently bisecting, started from branch 'unrelated-history'. > > > (use "git bisect reset" to get back to the original branch) > > > > > > nothing to commit, working tree clean > > > > > > I can't possible be on branch 'unrelated-history' during that > > > bisection. > > > > > > > > > OTOH, while during a rebase we are technically on a detached HEAD as > > > well, that rebase operation is all about constructing the new history > > > of the rebased branch, and once finished that branch will be updated > > > to point to the tip of the new history, thus it will include all the > > > commits created while on the detached HEAD. Therefore, it makes sense > > > conceptually to treat it as if we were on the rebased branch. That's > > > why it makes sense to display the name of the rebased branch in the > > > Bash prompt, and that's why I think it makes sense to default to edit > > > the description of the rebased branch without explicitly naming it. > > > > > > With bisect that just doesn't make sense. > > > > If the range you are bisecting belongs or lead to the current branch, > > that still makes sense. And it's probably most of the time. So, I am > > not sure your objection is valid enough here. > > I'm not sure what you mean with "belongs or lead to" a branch. > > Do you mean that the range is reachable from the branch that just so > happened to be checked out when the bisection was started? Well, I > have over 30 branches from where v2.25.0 is reachable, and all of them > are obviously bad candidates for editing their descriptions by default > while bisecting a totally unrelated issue. > If we take that simple example: * (my-branch) * * bisect bad * * (HEAD) * bisect good * It makes a lot of sense to me to edit my-branch description by default, even if the range good-bad happen to exist in other branches. -- Marc-André Lureau ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-31 15:59 ` Marc-André Lureau @ 2020-01-31 16:16 ` SZEDER Gábor 2020-02-06 22:26 ` Marc-André Lureau 0 siblings, 1 reply; 20+ messages in thread From: SZEDER Gábor @ 2020-01-31 16:16 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Eric Sunshine, Git List On Fri, Jan 31, 2020 at 04:59:15PM +0100, Marc-André Lureau wrote: > Hi > > On Fri, Jan 31, 2020 at 4:52 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > On Thu, Jan 30, 2020 at 10:37:38PM +0100, Marc-André Lureau wrote: > > > Hi > > > > > > On Fri, Jan 24, 2020 at 11:41 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > > > > On Sun, Jan 12, 2020 at 08:59:04PM -0500, Eric Sunshine wrote: > > > > > On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > > > > > Taking a deeper look at the code, I'm wondering it would make more > > > > > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > > > > > 'bisect'. Is there a reason that you limited this check to only > > > > > > > 'rebase'? > > > > > > > > > > > > What branch name does wt_status_get_state() return while bisecting? > > > > > > The branch where I started from? Because that's what 'git status' > > > > > > shows: > > > > > > But am I really on that branch? Does it really makes sense to edit > > > > > > the description of 'mybranch' by default while bisecting through an > > > > > > old revision range? I do not think so. > > > > > > > > > > It's not clear what downside you are pointing out; i.e. why would it > > > > > be a bad thing to be able to set the branch description even while > > > > > bisecting -- especially since `git status` affirms that it knows the > > > > > branch? > > > > > > > > No, during a bisect operation 'git status' knows the branch where I > > > > _was_ when I started bisecting, and where a 'git bisect reset' will > > > > eventually bring me back when I'm finished, and that has no relation > > > > whatsoever to the revision range that I'm bisecting. > > > > > > > > Consider this case: > > > > > > > > $ git checkout --orphan unrelated-history > > > > Switched to a new branch 'unrelated-history' > > > > $ git commit -m "test" > > > > [unrelated-history (root-commit) 639b9d1047] test > > > > <...> > > > > $ git bisect start v2.25.0 v2.24.0 > > > > Bisecting: 361 revisions left to test after this (roughly 9 steps) > > > > [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 > > > > $ git status > > > > HEAD detached at 7034cd094b > > > > You are currently bisecting, started from branch 'unrelated-history'. > > > > (use "git bisect reset" to get back to the original branch) > > > > > > > > nothing to commit, working tree clean > > > > > > > > I can't possible be on branch 'unrelated-history' during that > > > > bisection. > > > > > > > > > > > > OTOH, while during a rebase we are technically on a detached HEAD as > > > > well, that rebase operation is all about constructing the new history > > > > of the rebased branch, and once finished that branch will be updated > > > > to point to the tip of the new history, thus it will include all the > > > > commits created while on the detached HEAD. Therefore, it makes sense > > > > conceptually to treat it as if we were on the rebased branch. That's > > > > why it makes sense to display the name of the rebased branch in the > > > > Bash prompt, and that's why I think it makes sense to default to edit > > > > the description of the rebased branch without explicitly naming it. > > > > > > > > With bisect that just doesn't make sense. > > > > > > If the range you are bisecting belongs or lead to the current branch, > > > that still makes sense. And it's probably most of the time. So, I am > > > not sure your objection is valid enough here. > > > > I'm not sure what you mean with "belongs or lead to" a branch. > > > > Do you mean that the range is reachable from the branch that just so > > happened to be checked out when the bisection was started? Well, I > > have over 30 branches from where v2.25.0 is reachable, and all of them > > are obviously bad candidates for editing their descriptions by default > > while bisecting a totally unrelated issue. > > > > > If we take that simple example: > > * (my-branch) > * > * bisect bad > * > * (HEAD) > * bisect good > * > > It makes a lot of sense to me to edit my-branch description by > default, even if the range good-bad happen to exist in other branches. I still don't understand why it would make sense. Furthermore, how do you think you could avoid choosing an obviously bad branch to default to? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-01-31 16:16 ` SZEDER Gábor @ 2020-02-06 22:26 ` Marc-André Lureau 2020-02-07 10:02 ` SZEDER Gábor 0 siblings, 1 reply; 20+ messages in thread From: Marc-André Lureau @ 2020-02-06 22:26 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Eric Sunshine, Git List Hi On Fri, Jan 31, 2020 at 5:16 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Fri, Jan 31, 2020 at 04:59:15PM +0100, Marc-André Lureau wrote: > > Hi > > > > On Fri, Jan 31, 2020 at 4:52 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > > On Thu, Jan 30, 2020 at 10:37:38PM +0100, Marc-André Lureau wrote: > > > > Hi > > > > > > > > On Fri, Jan 24, 2020 at 11:41 PM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > > > > > > On Sun, Jan 12, 2020 at 08:59:04PM -0500, Eric Sunshine wrote: > > > > > > On Sun, Jan 12, 2020 at 7:14 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > > > > > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > > > > > > Taking a deeper look at the code, I'm wondering it would make more > > > > > > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > > > > > > 'bisect'. Is there a reason that you limited this check to only > > > > > > > > 'rebase'? > > > > > > > > > > > > > > What branch name does wt_status_get_state() return while bisecting? > > > > > > > The branch where I started from? Because that's what 'git status' > > > > > > > shows: > > > > > > > But am I really on that branch? Does it really makes sense to edit > > > > > > > the description of 'mybranch' by default while bisecting through an > > > > > > > old revision range? I do not think so. > > > > > > > > > > > > It's not clear what downside you are pointing out; i.e. why would it > > > > > > be a bad thing to be able to set the branch description even while > > > > > > bisecting -- especially since `git status` affirms that it knows the > > > > > > branch? > > > > > > > > > > No, during a bisect operation 'git status' knows the branch where I > > > > > _was_ when I started bisecting, and where a 'git bisect reset' will > > > > > eventually bring me back when I'm finished, and that has no relation > > > > > whatsoever to the revision range that I'm bisecting. > > > > > > > > > > Consider this case: > > > > > > > > > > $ git checkout --orphan unrelated-history > > > > > Switched to a new branch 'unrelated-history' > > > > > $ git commit -m "test" > > > > > [unrelated-history (root-commit) 639b9d1047] test > > > > > <...> > > > > > $ git bisect start v2.25.0 v2.24.0 > > > > > Bisecting: 361 revisions left to test after this (roughly 9 steps) > > > > > [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 > > > > > $ git status > > > > > HEAD detached at 7034cd094b > > > > > You are currently bisecting, started from branch 'unrelated-history'. > > > > > (use "git bisect reset" to get back to the original branch) > > > > > > > > > > nothing to commit, working tree clean > > > > > > > > > > I can't possible be on branch 'unrelated-history' during that > > > > > bisection. > > > > > > > > > > > > > > > OTOH, while during a rebase we are technically on a detached HEAD as > > > > > well, that rebase operation is all about constructing the new history > > > > > of the rebased branch, and once finished that branch will be updated > > > > > to point to the tip of the new history, thus it will include all the > > > > > commits created while on the detached HEAD. Therefore, it makes sense > > > > > conceptually to treat it as if we were on the rebased branch. That's > > > > > why it makes sense to display the name of the rebased branch in the > > > > > Bash prompt, and that's why I think it makes sense to default to edit > > > > > the description of the rebased branch without explicitly naming it. > > > > > > > > > > With bisect that just doesn't make sense. > > > > > > > > If the range you are bisecting belongs or lead to the current branch, > > > > that still makes sense. And it's probably most of the time. So, I am > > > > not sure your objection is valid enough here. > > > > > > I'm not sure what you mean with "belongs or lead to" a branch. > > > > > > Do you mean that the range is reachable from the branch that just so > > > happened to be checked out when the bisection was started? Well, I > > > have over 30 branches from where v2.25.0 is reachable, and all of them > > > are obviously bad candidates for editing their descriptions by default > > > while bisecting a totally unrelated issue. > > > > > > > > > If we take that simple example: > > > > * (my-branch) > > * > > * bisect bad > > * > > * (HEAD) > > * bisect good > > * > > > > It makes a lot of sense to me to edit my-branch description by > > default, even if the range good-bad happen to exist in other branches. > > I still don't understand why it would make sense. > > Furthermore, how do you think you could avoid choosing an obviously > bad branch to default to? It uses the same branch that git status displays. In your example: You are currently bisecting, started from branch 'unrelated-history'. So it's not completely off to pick that branch by default for --edit-description. But again, I think you are focusing on a rather rare case, please consider the most common case. -- Marc-André Lureau ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-02-06 22:26 ` Marc-André Lureau @ 2020-02-07 10:02 ` SZEDER Gábor 2020-02-07 14:16 ` Marc-André Lureau 0 siblings, 1 reply; 20+ messages in thread From: SZEDER Gábor @ 2020-02-07 10:02 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Eric Sunshine, Git List On Thu, Feb 06, 2020 at 11:26:56PM +0100, Marc-André Lureau wrote: > > > > > > > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > > > > > > > Taking a deeper look at the code, I'm wondering it would make more > > > > > > > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > > > > > > > 'bisect'. Is there a reason that you limited this check to only > > > > > > > > > 'rebase'? > > > > > > > > > > > > > > > > What branch name does wt_status_get_state() return while bisecting? > > > > > > > > The branch where I started from? Because that's what 'git status' > > > > > > > > shows: > > > > > > > > But am I really on that branch? Does it really makes sense to edit > > > > > > > > the description of 'mybranch' by default while bisecting through an > > > > > > > > old revision range? I do not think so. > > > > > > > > > > > > > > It's not clear what downside you are pointing out; i.e. why would it > > > > > > > be a bad thing to be able to set the branch description even while > > > > > > > bisecting -- especially since `git status` affirms that it knows the > > > > > > > branch? > > > > > > > > > > > > No, during a bisect operation 'git status' knows the branch where I > > > > > > _was_ when I started bisecting, and where a 'git bisect reset' will > > > > > > eventually bring me back when I'm finished, and that has no relation > > > > > > whatsoever to the revision range that I'm bisecting. > > > > > > > > > > > > Consider this case: > > > > > > > > > > > > $ git checkout --orphan unrelated-history > > > > > > Switched to a new branch 'unrelated-history' > > > > > > $ git commit -m "test" > > > > > > [unrelated-history (root-commit) 639b9d1047] test > > > > > > <...> > > > > > > $ git bisect start v2.25.0 v2.24.0 > > > > > > Bisecting: 361 revisions left to test after this (roughly 9 steps) > > > > > > [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 > > > > > > $ git status > > > > > > HEAD detached at 7034cd094b > > > > > > You are currently bisecting, started from branch 'unrelated-history'. > > > > > > (use "git bisect reset" to get back to the original branch) > > > > > > > > > > > > nothing to commit, working tree clean > > > > > > > > > > > > I can't possible be on branch 'unrelated-history' during that > > > > > > bisection. > > > > > > > > > > > > > > > > > > OTOH, while during a rebase we are technically on a detached HEAD as > > > > > > well, that rebase operation is all about constructing the new history > > > > > > of the rebased branch, and once finished that branch will be updated > > > > > > to point to the tip of the new history, thus it will include all the > > > > > > commits created while on the detached HEAD. Therefore, it makes sense > > > > > > conceptually to treat it as if we were on the rebased branch. That's > > > > > > why it makes sense to display the name of the rebased branch in the > > > > > > Bash prompt, and that's why I think it makes sense to default to edit > > > > > > the description of the rebased branch without explicitly naming it. > > > > > > > > > > > > With bisect that just doesn't make sense. > > > > > > > > > > If the range you are bisecting belongs or lead to the current branch, > > > > > that still makes sense. And it's probably most of the time. So, I am > > > > > not sure your objection is valid enough here. > > > > > > > > I'm not sure what you mean with "belongs or lead to" a branch. > > > > > > > > Do you mean that the range is reachable from the branch that just so > > > > happened to be checked out when the bisection was started? Well, I > > > > have over 30 branches from where v2.25.0 is reachable, and all of them > > > > are obviously bad candidates for editing their descriptions by default > > > > while bisecting a totally unrelated issue. > > > > > > > > > > > > > If we take that simple example: > > > > > > * (my-branch) > > > * > > > * bisect bad > > > * > > > * (HEAD) > > > * bisect good > > > * > > > > > > It makes a lot of sense to me to edit my-branch description by > > > default, even if the range good-bad happen to exist in other branches. > > > > I still don't understand why it would make sense. > > > > Furthermore, how do you think you could avoid choosing an obviously > > bad branch to default to? > > It uses the same branch that git status displays. In your example: > > You are currently bisecting, started from branch 'unrelated-history'. Yes, that's the problem: it shows "started from branch", not "On branch". Conceptually a huge difference. > So it's not completely off to pick that branch by default for > --edit-description. > > But again, I think you are focusing on a rather rare case, please > consider the most common case. I do focus on the most common case: I'm on branch 'foo' built on top of current master, when a bugreport comes in, and I start bisecting on the range v2.12.0..v2.16.0. It can not possibly be considered that during the bisect I'm on the branch 'foo' during the bisection, they are totally unrelated. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-02-07 10:02 ` SZEDER Gábor @ 2020-02-07 14:16 ` Marc-André Lureau 2020-02-07 18:57 ` Junio C Hamano 0 siblings, 1 reply; 20+ messages in thread From: Marc-André Lureau @ 2020-02-07 14:16 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Eric Sunshine, Git List Hi On Fri, Feb 7, 2020 at 11:02 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > > On Thu, Feb 06, 2020 at 11:26:56PM +0100, Marc-André Lureau wrote: > > > > > > > > > On Sat, Jan 11, 2020 at 08:27:11PM -0500, Eric Sunshine wrote: > > > > > > > > > > Taking a deeper look at the code, I'm wondering it would make more > > > > > > > > > > sense to call wt_status_get_state(), which handles 'rebase' and > > > > > > > > > > 'bisect'. Is there a reason that you limited this check to only > > > > > > > > > > 'rebase'? > > > > > > > > > > > > > > > > > > What branch name does wt_status_get_state() return while bisecting? > > > > > > > > > The branch where I started from? Because that's what 'git status' > > > > > > > > > shows: > > > > > > > > > But am I really on that branch? Does it really makes sense to edit > > > > > > > > > the description of 'mybranch' by default while bisecting through an > > > > > > > > > old revision range? I do not think so. > > > > > > > > > > > > > > > > It's not clear what downside you are pointing out; i.e. why would it > > > > > > > > be a bad thing to be able to set the branch description even while > > > > > > > > bisecting -- especially since `git status` affirms that it knows the > > > > > > > > branch? > > > > > > > > > > > > > > No, during a bisect operation 'git status' knows the branch where I > > > > > > > _was_ when I started bisecting, and where a 'git bisect reset' will > > > > > > > eventually bring me back when I'm finished, and that has no relation > > > > > > > whatsoever to the revision range that I'm bisecting. > > > > > > > > > > > > > > Consider this case: > > > > > > > > > > > > > > $ git checkout --orphan unrelated-history > > > > > > > Switched to a new branch 'unrelated-history' > > > > > > > $ git commit -m "test" > > > > > > > [unrelated-history (root-commit) 639b9d1047] test > > > > > > > <...> > > > > > > > $ git bisect start v2.25.0 v2.24.0 > > > > > > > Bisecting: 361 revisions left to test after this (roughly 9 steps) > > > > > > > [7034cd094bda4edbcdff7fad1a28fcaaf9b9a040] Sync with Git 2.24.1 > > > > > > > $ git status > > > > > > > HEAD detached at 7034cd094b > > > > > > > You are currently bisecting, started from branch 'unrelated-history'. > > > > > > > (use "git bisect reset" to get back to the original branch) > > > > > > > > > > > > > > nothing to commit, working tree clean > > > > > > > > > > > > > > I can't possible be on branch 'unrelated-history' during that > > > > > > > bisection. > > > > > > > > > > > > > > > > > > > > > OTOH, while during a rebase we are technically on a detached HEAD as > > > > > > > well, that rebase operation is all about constructing the new history > > > > > > > of the rebased branch, and once finished that branch will be updated > > > > > > > to point to the tip of the new history, thus it will include all the > > > > > > > commits created while on the detached HEAD. Therefore, it makes sense > > > > > > > conceptually to treat it as if we were on the rebased branch. That's > > > > > > > why it makes sense to display the name of the rebased branch in the > > > > > > > Bash prompt, and that's why I think it makes sense to default to edit > > > > > > > the description of the rebased branch without explicitly naming it. > > > > > > > > > > > > > > With bisect that just doesn't make sense. > > > > > > > > > > > > If the range you are bisecting belongs or lead to the current branch, > > > > > > that still makes sense. And it's probably most of the time. So, I am > > > > > > not sure your objection is valid enough here. > > > > > > > > > > I'm not sure what you mean with "belongs or lead to" a branch. > > > > > > > > > > Do you mean that the range is reachable from the branch that just so > > > > > happened to be checked out when the bisection was started? Well, I > > > > > have over 30 branches from where v2.25.0 is reachable, and all of them > > > > > are obviously bad candidates for editing their descriptions by default > > > > > while bisecting a totally unrelated issue. > > > > > > > > > > > > > > > > > If we take that simple example: > > > > > > > > * (my-branch) > > > > * > > > > * bisect bad > > > > * > > > > * (HEAD) > > > > * bisect good > > > > * > > > > > > > > It makes a lot of sense to me to edit my-branch description by > > > > default, even if the range good-bad happen to exist in other branches. > > > > > > I still don't understand why it would make sense. > > > > > > Furthermore, how do you think you could avoid choosing an obviously > > > bad branch to default to? > > > > It uses the same branch that git status displays. In your example: > > > > You are currently bisecting, started from branch 'unrelated-history'. > > Yes, that's the problem: it shows "started from branch", not "On > branch". Conceptually a huge difference. > > > So it's not completely off to pick that branch by default for > > --edit-description. > > > > But again, I think you are focusing on a rather rare case, please > > consider the most common case. > > I do focus on the most common case: I'm on branch 'foo' built on top > of current master, when a bugreport comes in, and I start bisecting on > the range v2.12.0..v2.16.0. It can not possibly be considered that > during the bisect I'm on the branch 'foo' during the bisection, they > are totally unrelated. And usually that bisection is ancestry of master, rarely unrelated. Also, when doing --edit-description there are comments like: # Please edit the description for the branch # unrelated-history What else do you suggest? -- Marc-André Lureau ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-02-07 14:16 ` Marc-André Lureau @ 2020-02-07 18:57 ` Junio C Hamano 2020-02-07 19:09 ` Marc-André Lureau 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2020-02-07 18:57 UTC (permalink / raw) To: Marc-André Lureau; +Cc: SZEDER Gábor, Eric Sunshine, Git List Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Also, when doing --edit-description there are comments like: > > # Please edit the description for the branch > # unrelated-history > > What else do you suggest? How about teaching "git branch --edit-description [HEAD]" notice when/if HEAD is detached and always error out, no matter what operation is in progress? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-02-07 18:57 ` Junio C Hamano @ 2020-02-07 19:09 ` Marc-André Lureau 2020-02-07 19:12 ` Junio C Hamano 0 siblings, 1 reply; 20+ messages in thread From: Marc-André Lureau @ 2020-02-07 19:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: SZEDER Gábor, Eric Sunshine, Git List Hi On Fri, Feb 7, 2020 at 7:57 PM Junio C Hamano <gitster@pobox.com> wrote: > > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Also, when doing --edit-description there are comments like: > > > > # Please edit the description for the branch > > # unrelated-history > > > > What else do you suggest? > > How about teaching "git branch --edit-description [HEAD]" notice > when/if HEAD is detached and always error out, no matter what > operation is in progress? Then --edit-description during won't default to the branch you started from the bisect. So we are back to my original proposal only, having branch default during rebase. Eric, do you mind? -- Marc-André Lureau ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-02-07 19:09 ` Marc-André Lureau @ 2020-02-07 19:12 ` Junio C Hamano 2020-02-07 19:29 ` Eric Sunshine 0 siblings, 1 reply; 20+ messages in thread From: Junio C Hamano @ 2020-02-07 19:12 UTC (permalink / raw) To: Marc-André Lureau; +Cc: SZEDER Gábor, Eric Sunshine, Git List Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Fri, Feb 7, 2020 at 7:57 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes: >> >> > Also, when doing --edit-description there are comments like: >> > >> > # Please edit the description for the branch >> > # unrelated-history >> > >> > What else do you suggest? >> >> How about teaching "git branch --edit-description [HEAD]" notice >> when/if HEAD is detached and always error out, no matter what >> operation is in progress? > > Then --edit-description during won't default to the branch you started > from the bisect. So we are back to my original proposal only, having > branch default during rebase. Eric, do you mind? What I meant by "no matter what is in progress" is not to special case "during rebase", either. Sorry for the confusion. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-02-07 19:12 ` Junio C Hamano @ 2020-02-07 19:29 ` Eric Sunshine 2020-02-07 20:14 ` Junio C Hamano 0 siblings, 1 reply; 20+ messages in thread From: Eric Sunshine @ 2020-02-07 19:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: Marc-André Lureau, SZEDER Gábor, Git List On Fri, Feb 7, 2020 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > Then --edit-description during won't default to the branch you started > > from the bisect. So we are back to my original proposal only, having > > branch default during rebase. Eric, do you mind? I don't feel strongly one way or the other. > > > How about teaching "git branch --edit-description [HEAD]" notice > > > when/if HEAD is detached and always error out, no matter what > > > operation is in progress? > > What I meant by "no matter what is in progress" is not to special > case "during rebase", either. That would defeat the original purpose[1] of this submission, I think. As I understand it, the idea all along was to make this operation work during a rebase. [1]: https://lore.kernel.org/git/20200110071929.119000-1-marcandre.lureau@redhat.com/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] branch: let '--edit-description' default to rebased branch during rebase 2020-02-07 19:29 ` Eric Sunshine @ 2020-02-07 20:14 ` Junio C Hamano 0 siblings, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2020-02-07 20:14 UTC (permalink / raw) To: Eric Sunshine; +Cc: Marc-André Lureau, SZEDER Gábor, Git List Eric Sunshine <sunshine@sunshineco.com> writes: >> > > How about teaching "git branch --edit-description [HEAD]" notice >> > > when/if HEAD is detached and always error out, no matter what >> > > operation is in progress? >> >> What I meant by "no matter what is in progress" is not to special >> case "during rebase", either. > > That would defeat the original purpose[1] of this submission, I think. > As I understand it, the idea all along was to make this operation work > during a rebase. I know. But I do not think it is a good thing to begin with. While you are rebasing the branch X and get control back before rebase finishes, you are *not* on branch X. You are *preparing* a new version of the history leading to the tip of branch X, in the hope that once you are done, you would make that new version of the history the history of branch X. Until that happens, you are not on branch X. If you were on branch X, then "git checkout -m another-branch" followed by some other operations, and then finally coming back with "git checkout -m X" would work. But it would not, because you are not on branch X. After all, you may well say "git rebase --abort" before you are done. Would "edit description" you do in the middle be reverted if you did so? It is bad for the user to blur the distinction between "detached and not on X but preparing to update X" and "working on X to advance X", and I think the original patch that started the thread takes us in that direction. Thanks. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-02-07 20:14 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-11 12:35 [PATCH] branch: let '--edit-description' default to rebased branch during rebase marcandre.lureau 2020-01-11 13:26 ` Eric Sunshine 2020-01-11 14:54 ` Marc-André Lureau 2020-01-12 1:27 ` Eric Sunshine 2020-01-12 6:44 ` Marc-André Lureau 2020-01-12 12:14 ` SZEDER Gábor 2020-01-13 1:59 ` Eric Sunshine 2020-01-24 22:41 ` SZEDER Gábor 2020-01-30 21:37 ` Marc-André Lureau 2020-01-31 15:52 ` SZEDER Gábor 2020-01-31 15:59 ` Marc-André Lureau 2020-01-31 16:16 ` SZEDER Gábor 2020-02-06 22:26 ` Marc-André Lureau 2020-02-07 10:02 ` SZEDER Gábor 2020-02-07 14:16 ` Marc-André Lureau 2020-02-07 18:57 ` Junio C Hamano 2020-02-07 19:09 ` Marc-André Lureau 2020-02-07 19:12 ` Junio C Hamano 2020-02-07 19:29 ` Eric Sunshine 2020-02-07 20:14 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.