threads / patch / 26479

patchgit-gui: give more advice when detaching HEAD

Subject: [PATCH] git-gui: give more advice when detaching HEAD

## tl;dr

19 messages between Feb 12, 2011 and Feb 17, 2011. Diffs are folded; open one to read it.

replies: 18people: 7as markdown or json

Jeff King· Feb 12, 2011, 07:05 UTC · lore

The current advice is a little sparse and dates back to d41b43e (git-gui: Refactor branch switch to support detached head, 2007-07-08). In the meantime, command-line git grew much more detailed advice for this situation, especially in 13be3e3 (Reword "detached HEAD" notification, 2010-01-29).

Let's use that more detailed advice here.
Signed-off-by: Jeff King <peff@peff.net>
---
I recently helped somebody who had detached HEAD via git-gui, made a
bunch of commits, switched to another branch, and then became confused
about where his work went.

After working through what happened with him, I think this is one place where we could have prevented the problem. And given that we saw the need for more advice in the CLI, I think this change is a no-brainer.

I also think we could have saved him by doing one or more of:
  1. Give some indication or warning during commit that you're in a
     detached state. The CLI template says "You are not on any branch"
     when editing the commit message, and mentions "detached HEAD" as
     the branch in the post-commit summary. As far as I can tell,
     git-gui says nothing at all.
  2. When leaving the detached state, notice that we have commits not
     contained in any other ref and pop up an "are you sure you want to
     lose these commits" dialog, with an option to create a branch. This
     is something we considered and rejected for the CLI, but I wonder
     if it makes more sense for git-gui.
  3. Make it easier to create a new branch from the checkout dialog.
     Obviously I can go to "Branch->Create" and make a new branch from a
     remote one. But if my mental model is "Checkout", then I pick a
     remote branch, we may want to present the user with the decision
     _then_ about detaching or creating. Something as simple as a "make
     local branch from remote" checkbox would help. Or perhaps the
     "you're going to a detached HEAD" dialog should actually have a
     button to create a local branch right then and there instead. I
     dunno.

All of those things are too far beyond my scope of caring about git-gui and wanting to write tcl to actually implement myself. But I thought I would share them as thoughts that came from a real confused-user interaction. Feel free to ignore.

 lib/checkout_op.tcl |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)
Show changes to lib/checkout_op.tcl +10 −3
diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
index 9e7412c..9c95208 100644
--- a/lib/checkout_op.tcl
+++ b/lib/checkout_op.tcl
@@ -449,9 +449,16 @@ method _after_readtree {} {
 	}
 
 	if {$is_detached} {
-		info_popup [mc "You are no longer on a local branch.
-
-If you wanted to be on a branch, create one now starting from 'This Detached Checkout'."]
+		info_popup [mc \
+"You are no longer on a local branch. You can look
+around, make experimental changes and commit,
+and you can discard any commits you make in this
+state without impacting any branches by
+performing another checkout.
+
+If you want to create a new branch to retain
+commits you create, you may do so (now or later)
+by starting from 'This Detached Checkout'."]
 	}
 
 	# -- Run the post-checkout hook.

-- 
1.7.4
Junio C Hamano· Feb 12, 2011, 07:42 UTC · re: Jeff King · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
>   2. When leaving the detached state, notice that we have commits not
>      contained in any other ref and pop up an "are you sure you want to
>      lose these commits" dialog, with an option to create a branch. This
>      is something we considered and rejected for the CLI, but I wonder
>      if it makes more sense for git-gui.

Hmm, I don't recall the discussion on this for the CLI, but it intuitively feels like a good thing to do, unless it incurs an unacceptable cost. Temporarily detaching HEAD by scripts like rebase and am that know what they are doing should never have to pay the penalty, but an expert user who worked interactively on the detached HEAD can be made to wait for 0.2 second more.

Your 1 and 3 both sound like sensible things to do, but I am not a good judge on them as I rarely if ever work in GUI.

Jeff King· Feb 12, 2011, 08:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

On Fri, Feb 11, 2011 at 11:42:47PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> >   2. When leaving the detached state, notice that we have commits not
> >      contained in any other ref and pop up an "are you sure you want to
> >      lose these commits" dialog, with an option to create a branch. This
> >      is something we considered and rejected for the CLI, but I wonder
> >      if it makes more sense for git-gui.
> 
> Hmm, I don't recall the discussion on this for the CLI, but it intuitively
> feels like a good thing to do, unless it incurs an unacceptable cost.

I think one of the main concerns was cost, but I'm having trouble coming up with the exact thread that I recall.

There is some discussion here, including Linus endorsing an exact-ref check:

  http://article.gmane.org/gmane.comp.version-control.git/36428

There is a lot of back and forth, but I didn't have the patience to read it all.

There is also this thread:
  http://thread.gmane.org/gmane.comp.version-control.git/94695

where one of the arguments against a leaving-detached safety valve seems to be "well, we reflog the HEAD these days, so it's no big deal" (and indeed, in this confused user case, the reflog did end up being the recovery method).

I have a feeling there is another thread somewhere, but I can't find it.
> Temporarily detaching HEAD by scripts like rebase and am that know what
> they are doing should never have to pay the penalty, but an expert user
> who worked interactively on the detached HEAD can be made to wait for 0.2
> second more.

Is it that cheap? A full reachability check for something that is not in any ref would involve going to the roots, wouldn't it? On linux-2.6, that is something like 3s on my fast-ish machine. Though I guess using commit-time cutoffs could make it really short (which reminds me, I really need to clean up and post my patches to deal with clock skew).

> Your 1 and 3 both sound like sensible things to do, but I am not a good
> judge on them as I rarely if ever work in GUI.

That's kind of how I feel. I've never actually used git-gui beyond trying to help users.

-Peff
Junio C Hamano· Feb 12, 2011, 08:17 UTC · re: Jeff King · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

Jeff King <peff@peff.net> writes:
> Is it that cheap? A full reachability check for something that is not in
> any ref would involve going to the roots, wouldn't it?
You only need to dig until you hit a merge base, no?

In this case, you would need to compute just one merge base, between the commit you are about to leave, and the (imaginary) commit that is a merge across all the tips of your refs. If the merge base is the commit you are about to leave, you were sightseeing in the past without creating anything new, otherwise you will lose commits between the computed base and the commit you are about to leave.

And merge-base has an interface to compute exactly that, I think.
Jeff King· Feb 12, 2011, 08:21 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

On Sat, Feb 12, 2011 at 12:17:16AM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Is it that cheap? A full reachability check for something that is not in
> > any ref would involve going to the roots, wouldn't it?
> 
> You only need to dig until you hit a merge base, no?

Hmm, yeah, you're right. In the worst case of reachability checks, you would share no ancestry and go to the roots searching for the merge base, but of course that is very unlikely to be the case here. So it should be much cheaper.

> And merge-base has an interface to compute exactly that, I think.
Want to do a proof-of-concept patch? Then we can get some real timings.
-Peff
Junio C Hamano· Feb 17, 2011, 23:13 UTC · re: Jeff King · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

Jeff King <peff@peff.net> writes:
> Want to do a proof-of-concept patch? Then we can get some real timings.
This was about
Show 5 quoted lines
> >   2. When leaving the detached state, notice that we have commits not
> >      contained in any other ref and pop up an "are you sure you want to
> >      lose these commits" dialog, with an option to create a branch. This
> >      is something we considered and rejected for the CLI, but I wonder
> >      if it makes more sense for git-gui.

I thought about counting remaining commits, but decided against it. The cost has already paid (the "limited" traversal already has happend), so it may not be too bad to show each of them in oneline format if somebody really wanted to to tell the user "here are the stuff you are about to lose".

Also it might make sense to have a training wheel option of forcing "checkout -f branch-I-wanted-to-go" in this case as an extra safety valve, and it would be even Ok to enable the training wheel by default, as long as annoyed experts can turn it off via configuration.

 builtin/checkout.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 60 insertions(+), 6 deletions(-)
Show changes to builtin/checkout.c +60 −6
diff --git a/builtin/checkout.c b/builtin/checkout.c
index cd7f56e..1f5376f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -503,6 +503,17 @@ static void detach_advice(const char *old_path, const char *new_name)
 	fprintf(stderr, fmt, new_name);
 }
 
+static void suggest_reattach(struct commit *commit)
+{
+	const char fmt[] =
+	"Note: you are about to abandon commit %1$s\n\n"
+	"None of your branches nor tags refer to this commit. If you want to\n"
+	"keep this commit, you can do so by creating a new branch. Example:\n\n"
+	" git branch new_branch_name %1$s\n\n";
+
+	fprintf(stderr, fmt, sha1_to_hex(commit->object.sha1));
+}
+
 static void update_refs_for_switch(struct checkout_opts *opts,
 				   struct branch_info *old,
 				   struct branch_info *new)
@@ -578,6 +589,54 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 		report_tracking(new);
 }
 
+struct rev_list_args {
+	int argc;
+	int alloc;
+	const char **argv;
+};
+
+static void add_one_rev_list_arg(struct rev_list_args *args, const char *s)
+{
+	ALLOC_GROW(args->argv, args->argc + 1, args->alloc);
+	args->argv[args->argc++] = s;
+}
+
+static int add_one_ref_to_rev_list_arg(const char *refname,
+				       const unsigned char *sha1,
+				       int flags,
+				       void *cb_data)
+{
+	add_one_rev_list_arg(cb_data, refname);
+	return 0;
+}
+
+/*
+ * We are about to leave commit that was at the tip of a detached
+ * HEAD.  If it is not reachable from any ref, this is the last chance
+ * for the user to do so without resorting to reflog.
+ */
+static void orphaned_commit_warning(struct commit *commit)
+{
+	struct rev_list_args args = { 0, 0, NULL };
+	struct rev_info revs;
+
+	add_one_rev_list_arg(&args, "(internal)");
+	add_one_rev_list_arg(&args, sha1_to_hex(commit->object.sha1));
+	add_one_rev_list_arg(&args, "--not");
+	for_each_ref(add_one_ref_to_rev_list_arg, &args);
+	add_one_rev_list_arg(&args, "--");
+	add_one_rev_list_arg(&args, NULL);
+
+	init_revisions(&revs, NULL);
+	if (setup_revisions(args.argc - 1, args.argv, &revs, NULL) != 1)
+		die("internal error: only -- alone should have been left");
+	if (prepare_revision_walk(&revs))
+		die("internal error in revision walk");
+	if (!(commit->object.flags & UNINTERESTING))
+		suggest_reattach(commit);
+	describe_detached_head("Previous HEAD position was", commit);
+}
+
 static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 {
 	int ret = 0;
@@ -605,13 +664,8 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	if (ret)
 		return ret;
 
-	/*
-	 * If we were on a detached HEAD, but have now moved to
-	 * a new commit, we want to mention the old commit once more
-	 * to remind the user that it might be lost.
-	 */
 	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
-		describe_detached_head("Previous HEAD position was", old.commit);
+		orphaned_commit_warning(old.commit);
 
 	update_refs_for_switch(opts, &old, new);
Junio C Hamano· Feb 12, 2011, 08:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

Junio C Hamano <gitster@pobox.com> writes:
> You only need to dig until you hit a merge base, no?
> ...
> And merge-base has an interface to compute exactly that, I think.

Ah, forget "merge-base". In the kernel repository, the very old "v2.6.12" will participate in the (imaginary) merge across all the refs, and computing merge-base means we need to traverse down to it.

We only need to prime a "struct revisions" with the detached HEAD as the sole positive, and the refs as negatives (i.e. UNINTERESTING), and walk the history the usual way, until we either

 (1) see HEAD painted uninteresting; or
 (2) the queue becomes all uninteresting.

As soon as (1) happens, we know the HEAD is reachable from some ref, and we can immediately stop. When (2) happens, we inspect the HEAD again and if it is painted uninteresting then we know HEAD is reachable from some ref. Otherwise HEAD will become dangling when you leave it.

That way, the traversal will terminate much sooner than computing the true merge base.

Sverre Rabbelier· Feb 13, 2011, 00:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

Heya,
On Sat, Feb 12, 2011 at 09:42, Junio C Hamano <gitster@pobox.com> wrote:
> That way, the traversal will terminate much sooner than computing the true
> merge base.

Since we want to use this in git-gui, do you intend to expose this as a command somehow (e.g. 'git rev-parse --reachable HEAD --all' or somesuch)?

-- 
Cheers,

Sverre Rabbelier
Johannes Sixt· Feb 13, 2011, 09:22 UTC · re: Sverre Rabbelier · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

On Sonntag, 13. Februar 2011, Sverre Rabbelier wrote:
Show 9 quoted lines
> Heya,
>
> On Sat, Feb 12, 2011 at 09:42, Junio C Hamano <gitster@pobox.com> wrote:
> > That way, the traversal will terminate much sooner than computing the
> > true merge base.
>
> Since we want to use this in git-gui, do you intend to expose this as
> a command somehow (e.g. 'git rev-parse --reachable HEAD --all' or
> somesuch)?
What's wrong with checking the output of
  git rev-list -1 HEAD --not --branches --tags --
for zero length?
-- Hannes
Junio C Hamano· Feb 13, 2011, 23:10 UTC · re: Johannes Sixt · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

Johannes Sixt <j6t@kdbg.org> writes:
Show 13 quoted lines
>> On Sat, Feb 12, 2011 at 09:42, Junio C Hamano <gitster@pobox.com> wrote:
>> > That way, the traversal will terminate much sooner than computing the
>> > true merge base.
>>
>> Since we want to use this in git-gui, do you intend to expose this as
>> a command somehow (e.g. 'git rev-parse --reachable HEAD --all' or
>> somesuch)?
>
> What's wrong with checking the output of
>
>   git rev-list -1 HEAD --not --branches --tags --
>
> for zero length?

Nothing, except that (1) that approach won't catch refs outside heads and tags (like the ones I have in refs/merge-fixes and refs/hold), and (2) it won't have an early termination optimization I mentioned either.

I don't know how much the early termination optimization matter in practice, though. It would probably depend heavily on where you begin.

Heiko Voigt· Feb 13, 2011, 12:31 UTC · re: Jeff King · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

Hi,
On Sat, Feb 12, 2011 at 02:05:38AM -0500, Jeff King wrote:
Show 5 quoted lines
>   1. Give some indication or warning during commit that you're in a
>      detached state. The CLI template says "You are not on any branch"
>      when editing the commit message, and mentions "detached HEAD" as
>      the branch in the post-commit summary. As far as I can tell,
>      git-gui says nothing at all.
How about something like this:
---8<----
From 8e2b61cd5e8d85f43ed6f00935a757f0dfa56b3b Mon Sep 17 00:00:00 2001
From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Sun, 13 Feb 2011 13:25:04 +0100
Subject: [PATCH] git-gui: warn when trying to commit on a detached head

The commandline is already warning when checking out a detached head. Since the only thing thats potentially dangerous is to create commits on a detached head lets warn in case the user is about to do that.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
The wording of the warning might need some cleanup and documentation of
the configuration variable is still missing but if you like it I will
add it.
 git-gui/git-gui.sh     |    1 +
 git-gui/lib/commit.tcl |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)
Show changes to 2 files +15 −0

git-gui/git-gui.sh, git-gui/lib/commit.tcl

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index d3acf0d..5314a3f 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -831,6 +831,7 @@ set default_config(gui.fontdiff) [font configure font_diff]
 # TODO: this option should be added to the git-config documentation
 set default_config(gui.maxfilesdisplayed) 5000
 set default_config(gui.usettk) 1
+set default_config(gui.warndetachedcommit) 1
 set font_descs {
 	{fontui   font_ui   {mc "Main Font"}}
 	{fontdiff font_diff {mc "Diff/Console Font"}}
diff --git a/git-gui/lib/commit.tcl b/git-gui/lib/commit.tcl
index 7f459cd..9bef8ee 100644
--- a/git-gui/lib/commit.tcl
+++ b/git-gui/lib/commit.tcl
@@ -259,8 +259,22 @@ proc commit_prehook_wait {fd_ph curHEAD msg_p} {
 }
 
 proc commit_commitmsg {curHEAD msg_p} {
+	global is_detached repo_config
 	global pch_error
 
+	if {$is_detached && $repo_config(gui.warndetachedcommit)} {
+		set msg [mc "You are about to commit on a detached head.
+This is a potentially dangerous thing to do because
+if you switch to another branch you will loose your
+changes and it can be difficult to get them back.
+
+Do you really want to proceed?"]
+		if {[ask_popup $msg] ne yes} {
+			unlock_index
+			return
+		}
+	}
+
 	# -- Run the commit-msg hook.
 	#
 	set fd_ph [githook_read commit-msg $msg_p]

-- 
1.7.4.rc3.4.g155c4
Jeff King· Feb 15, 2011, 06:39 UTC · re: Heiko Voigt · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

On Sun, Feb 13, 2011 at 01:31:52PM +0100, Heiko Voigt wrote:
Show 14 quoted lines
> On Sat, Feb 12, 2011 at 02:05:38AM -0500, Jeff King wrote:
> >   1. Give some indication or warning during commit that you're in a
> >      detached state. The CLI template says "You are not on any branch"
> >      when editing the commit message, and mentions "detached HEAD" as
> >      the branch in the post-commit summary. As far as I can tell,
> >      git-gui says nothing at all.
> 
> How about something like this:
> [...]
> Subject: [PATCH] git-gui: warn when trying to commit on a detached head
> 
> The commandline is already warning when checking out a detached head.
> Since the only thing thats potentially dangerous is to create commits
> on a detached head lets warn in case the user is about to do that.

It seems a little heavy-handed to have a dialog pop up for each commit. It's not actually dangerous to create a commit on a detached HEAD; it's just dangerous to _leave_ without referencing your new commits.

So I think for making commits, something informational that doesn't require a click-through would be the more appropriate level (similar to what the CLI does; it just mentions it in the commit template). I guess there isn't a commit template in the same way for git gui; instead, it is always showing you the current state. And indeed, it does switch from "Current Branch: master" to "Current Branch: HEAD" when you are on a detached head. Maybe we should beef that up a bit to "You are not on any branch." or something that is more self-explanatory. I dunno. I am just guessing here about what users would want.

I do think a pop-up is appropriate when you try to check something else out, and commits you have made on the detached HEAD are about to become unreferenced. But this is something even the CLI doesn't do, so it would make sense to see how the check is implemented there first before doing anything in git-gui.

-Peff
Heiko Voigt· Feb 15, 2011, 19:16 UTC · re: Jeff King · lore

Re: Re: [PATCH] git-gui: give more advice when detaching HEAD

Hi,
On Tue, Feb 15, 2011 at 01:39:03AM -0500, Jeff King wrote:
Show 20 quoted lines
> On Sun, Feb 13, 2011 at 01:31:52PM +0100, Heiko Voigt wrote:
> 
> > On Sat, Feb 12, 2011 at 02:05:38AM -0500, Jeff King wrote:
> > >   1. Give some indication or warning during commit that you're in a
> > >      detached state. The CLI template says "You are not on any branch"
> > >      when editing the commit message, and mentions "detached HEAD" as
> > >      the branch in the post-commit summary. As far as I can tell,
> > >      git-gui says nothing at all.
> > 
> > How about something like this:
> > [...]
> > Subject: [PATCH] git-gui: warn when trying to commit on a detached head
> > 
> > The commandline is already warning when checking out a detached head.
> > Since the only thing thats potentially dangerous is to create commits
> > on a detached head lets warn in case the user is about to do that.
> 
> It seems a little heavy-handed to have a dialog pop up for each commit.
> It's not actually dangerous to create a commit on a detached HEAD; it's
> just dangerous to _leave_ without referencing your new commits.
Hmm, how about adding a checkbox:
  [ ] Do not ask again

In my experience anything other than a popup will be overseen so I would suggest doing it at least once to prepare the user for the possible consequences.

IMO such a message is a good thing for the GUI regardless whether we implement the leaving detached HEAD state warning. First I think a typical GUI user does not commit on a detached head that often since there is currently no way to use these commits from the GUI (e.g. format-patch, rebase, ...). Second because a detached head is very practical for testing work on a remote branch the message box would remind most users to switch to their development branch first. If they only get that message after a series of commits it might become a hassle for them to get these commits onto another branch (remember no format-patch or rebase currently).

Show 15 quoted lines
> So I think for making commits, something informational that doesn't
> require a click-through would be the more appropriate level (similar to
> what the CLI does; it just mentions it in the commit template). I guess
> there isn't a commit template in the same way for git gui; instead, it
> is always showing you the current state. And indeed, it does switch from
> "Current Branch: master" to "Current Branch: HEAD" when you are on a
> detached head. Maybe we should beef that up a bit to "You are not on any
> branch." or something that is more self-explanatory. I dunno. I am just
> guessing here about what users would want.
> 
> I do think a pop-up is appropriate when you try to check something else
> out, and commits you have made on the detached HEAD are about to become
> unreferenced. But this is something even the CLI doesn't do, so it would
> make sense to see how the check is implemented there first before doing
> anything in git-gui.
>From what I read in this thread it currently seems to be not so easy to

precisely find out whether some commit is referenced. (If we care about stuff outside of remotes, heads and tags). But maybe we do not need that for the GUI.

If a commit is referenced from non typical refs the worst we do is issue a false warning. Meaning we warn the user even though the commit is referenced. For a GUI I think being a little more restrictive is the right thing to do since it should guide the user much more into a safe workflow. If he wants to do special things than there still is the CLI to fall back on. And its just a warning so we are not preventing anything.

Now it depends on what we would want for the CLI if we are going to implement a thorough check over everything in refs/ than there is no reason for not applying the same thing to git-gui. In case the current behavior is deemed sufficient we should go with the check mention

Just to give you a practical example:

At $dayjob we are currently even more restrictive and completely forbid commits on a detached head by a pre-commit hook. This was mainly done due to the lack of warnings but I do not recall a single incident where a user actually complained about this restriction (~90% GUI users).

Cheers Heiko
Pat Thoyts· Feb 15, 2011, 19:48 UTC · re: Heiko Voigt · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Tue, 15 Feb 2011 19:43:54 +0000
Subject: [PATCH] git-gui: warn when trying to commit on a detached head

The commandline is already warning when checking out a detached head. Since the only thing thats potentially dangerous is to create commits on a detached head lets warn in case the user is about to do that.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
---
Heiko Voigt <hvoigt@hvoigt.net> writes:
Show 85 quoted lines
>Hi,
>
>On Tue, Feb 15, 2011 at 01:39:03AM -0500, Jeff King wrote:
>> On Sun, Feb 13, 2011 at 01:31:52PM +0100, Heiko Voigt wrote:
>> 
>> > On Sat, Feb 12, 2011 at 02:05:38AM -0500, Jeff King wrote:
>> > >   1. Give some indication or warning during commit that you're in a
>> > >      detached state. The CLI template says "You are not on any branch"
>> > >      when editing the commit message, and mentions "detached HEAD" as
>> > >      the branch in the post-commit summary. As far as I can tell,
>> > >      git-gui says nothing at all.
>> > 
>> > How about something like this:
>> > [...]
>> > Subject: [PATCH] git-gui: warn when trying to commit on a detached head
>> > 
>> > The commandline is already warning when checking out a detached head.
>> > Since the only thing thats potentially dangerous is to create commits
>> > on a detached head lets warn in case the user is about to do that.
>> 
>> It seems a little heavy-handed to have a dialog pop up for each commit.
>> It's not actually dangerous to create a commit on a detached HEAD; it's
>> just dangerous to _leave_ without referencing your new commits.
>
>Hmm, how about adding a checkbox:
>
>  [ ] Do not ask again
>
>In my experience anything other than a popup will be overseen so I would
>suggest doing it at least once to prepare the user for the possible
>consequences.
>
>IMO such a message is a good thing for the GUI regardless whether we
>implement the leaving detached HEAD state warning. First I think a
>typical GUI user does not commit on a detached head that often since
>there is currently no way to use these commits from the GUI (e.g.
>format-patch, rebase, ...). Second because a detached head is very
>practical for testing work on a remote branch the message box would
>remind most users to switch to their development branch first. If they
>only get that message after a series of commits it might become a hassle
>for them to get these commits onto another branch (remember no
>format-patch or rebase currently).
>
>> So I think for making commits, something informational that doesn't
>> require a click-through would be the more appropriate level (similar to
>> what the CLI does; it just mentions it in the commit template). I guess
>> there isn't a commit template in the same way for git gui; instead, it
>> is always showing you the current state. And indeed, it does switch from
>> "Current Branch: master" to "Current Branch: HEAD" when you are on a
>> detached head. Maybe we should beef that up a bit to "You are not on any
>> branch." or something that is more self-explanatory. I dunno. I am just
>> guessing here about what users would want.
>> 
>> I do think a pop-up is appropriate when you try to check something else
>> out, and commits you have made on the detached HEAD are about to become
>> unreferenced. But this is something even the CLI doesn't do, so it would
>> make sense to see how the check is implemented there first before doing
>> anything in git-gui.
>
>From what I read in this thread it currently seems to be not so easy to
>precisely find out whether some commit is referenced. (If we care about
>stuff outside of remotes, heads and tags). But maybe we do not need
>that for the GUI.
>
>If a commit is referenced from non typical refs the worst we do is issue
>a false warning. Meaning we warn the user even though the commit is
>referenced. For a GUI I think being a little more restrictive is the
>right thing to do since it should guide the user much more into a safe
>workflow. If he wants to do special things than there still is the CLI
>to fall back on. And its just a warning so we are not preventing
>anything.
>
>Now it depends on what we would want for the CLI if we are going to
>implement a thorough check over everything in refs/ than there is no
>reason for not applying the same thing to git-gui. In case the current
>behavior is deemed sufficient we should go with the check mention
>
>Just to give you a practical example:
>
>At $dayjob we are currently even more restrictive and completely forbid
>commits on a detached head by a pre-commit hook. This was mainly done
>due to the lack of warnings but I do not recall a single incident where a
>user actually complained about this restriction (~90% GUI users).
>
>Cheers Heiko

My feeling is that the user should be making a branch to hold his commits. So I suggest adding some text to suggest that a branch be created and keep annoying the user every time they commit to a detached head. This errs on the side of not dropping commits into the reflog which seems the most useful strategy to me.

So here is a modded version of Heiko's patch.
 git-gui.sh     |    1 +
 lib/commit.tcl |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)
Show changes to 2 files +16 −0

git-gui.sh, lib/commit.tcl

diff --git a/git-gui.sh b/git-gui.sh
index d96df63..9f2e9ae 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -835,6 +835,7 @@ set default_config(gui.fontdiff) [font configure font_diff]
 # TODO: this option should be added to the git-config documentation
 set default_config(gui.maxfilesdisplayed) 5000
 set default_config(gui.usettk) 1
+set default_config(gui.warndetachedcommit) 1
 set font_descs {
 	{fontui   font_ui   {mc "Main Font"}}
 	{fontdiff font_diff {mc "Diff/Console Font"}}
diff --git a/lib/commit.tcl b/lib/commit.tcl
index 5ce4687..372bed9 100644
--- a/lib/commit.tcl
+++ b/lib/commit.tcl
@@ -260,8 +260,23 @@ proc commit_prehook_wait {fd_ph curHEAD msg_p} {
 }
 
 proc commit_commitmsg {curHEAD msg_p} {
+	global is_detached repo_config
 	global pch_error
 
+	if {$is_detached && $repo_config(gui.warndetachedcommit)} {
+		set msg [mc "You are about to commit on a detached head.\
+This is a potentially dangerous thing to do because if you switch\
+to another branch you will loose your changes and it can be difficult\
+to retrieve them later from the reflog. You should probably cancel this\
+commit and create a new branch to continue.\n\
+\n\
+Do you really want to proceed with your commit?"]
+		if {[ask_popup $msg] ne yes} {
+			unlock_index
+			return
+		}
+	}
+
 	# -- Run the commit-msg hook.
 	#
 	set fd_ph [githook_read commit-msg $msg_p]

-- 
1.7.4.1
Jeff King· Feb 16, 2011, 03:50 UTC · re: Pat Thoyts · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

On Tue, Feb 15, 2011 at 07:48:33PM +0000, Pat Thoyts wrote:
Show 5 quoted lines
> My feeling is that the user should be making a branch to hold his
> commits. So I suggest adding some text to suggest that a branch be
> created and keep annoying the user every time they commit to a detached
> head. This errs on the side of not dropping commits into the reflog
> which seems the most useful strategy to me.

I like Heiko's suggestion of a check-box to turn off gui.warndetachedcommit, though I personally don't care much as a non-gui user. I also think if you are going to suggest making a branch that there should be an "OK, make the branch now" button, or at least a button to take you to the right dialog to create it. But again, I know nothing about gui design and as a non-user I don't have a strong feeling.

But...
Show 6 quoted lines
> +	if {$is_detached && $repo_config(gui.warndetachedcommit)} {
> +		set msg [mc "You are about to commit on a detached head.\
> +This is a potentially dangerous thing to do because if you switch\
> +to another branch you will loose your changes and it can be difficult\
> +to retrieve them later from the reflog. You should probably cancel this\
> +commit and create a new branch to continue.\n\

I do have a strong feeling about English grammar, and this should be s/loose/lose/. :)

-Peff
Heiko Voigt· Feb 17, 2011, 17:38 UTC · re: Pat Thoyts · lore

Re: Re: [PATCH] git-gui: give more advice when detaching HEAD

Hi,
On Tue, Feb 15, 2011 at 07:48:33PM +0000, Pat Thoyts wrote:
Show 11 quoted lines
> From: Heiko Voigt <hvoigt@hvoigt.net>
> Date: Tue, 15 Feb 2011 19:43:54 +0000
> Subject: [PATCH] git-gui: warn when trying to commit on a detached head
> 
> The commandline is already warning when checking out a detached head.
> Since the only thing thats potentially dangerous is to create commits
> on a detached head lets warn in case the user is about to do that.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> ---
[...]
Show 7 quoted lines
> My feeling is that the user should be making a branch to hold his
> commits. So I suggest adding some text to suggest that a branch be
> created and keep annoying the user every time they commit to a detached
> head. This errs on the side of not dropping commits into the reflog
> which seems the most useful strategy to me.
> 
> So here is a modded version of Heiko's patch.

Thanks for cleaning up my wording. I would be fine with this patch. I played with creating a dialog including a checkbox yesterday. Please see my other answer to this thread whether we work on this further. Otherwise I would be fine with just issuing the warning every time. Its better than not warning at all and using the configuration option a clueful person can still disable the warning.

Cheers Heiko
P.S.: Should I prepare a separate patch to document the variable for
      Junio once he pulls again from you?
Jeff King· Feb 16, 2011, 03:46 UTC · re: Heiko Voigt · lore

Re: Re: [PATCH] git-gui: give more advice when detaching HEAD

On Tue, Feb 15, 2011 at 08:16:21PM +0100, Heiko Voigt wrote:
Show 11 quoted lines
> > It seems a little heavy-handed to have a dialog pop up for each commit.
> > It's not actually dangerous to create a commit on a detached HEAD; it's
> > just dangerous to _leave_ without referencing your new commits.
> 
> Hmm, how about adding a checkbox:
> 
>   [ ] Do not ask again
> 
> In my experience anything other than a popup will be overseen so I would
> suggest doing it at least once to prepare the user for the possible
> consequences.

Yeah, that's much better IMHO because at least clueful people can dismiss it after the first time.

Show 5 quoted lines
> IMO such a message is a good thing for the GUI regardless whether we
> implement the leaving detached HEAD state warning. First I think a
> typical GUI user does not commit on a detached head that often since
> there is currently no way to use these commits from the GUI (e.g.
> format-patch, rebase, ...).

Fair enough. I really have no idea what sorts of things gui users do, or how they perceive the system.

Show 6 quoted lines
> Second because a detached head is very practical for testing work on a
> remote branch the message box would remind most users to switch to
> their development branch first. If they only get that message after a
> series of commits it might become a hassle for them to get these
> commits onto another branch (remember no format-patch or rebase
> currently).
Good point.
Show 10 quoted lines
> > I do think a pop-up is appropriate when you try to check something else
> > out, and commits you have made on the detached HEAD are about to become
> > unreferenced. But this is something even the CLI doesn't do, so it would
> > make sense to see how the check is implemented there first before doing
> > anything in git-gui.
> 
> From what I read in this thread it currently seems to be not so easy to
> precisely find out whether some commit is referenced. (If we care about
> stuff outside of remotes, heads and tags). But maybe we do not need
> that for the GUI.

Yeah, I think there is still some question about how it should happen, and any check in the gui should probably be the same as in the cli. But from the rest of what you say, that shouldn't impact whether a per-commit warning is worth doing.

-Peff
Heiko Voigt· Feb 17, 2011, 17:27 UTC · re: Jeff King · lore

Re: Re: Re: [PATCH] git-gui: give more advice when detaching HEAD

Hi,
On Tue, Feb 15, 2011 at 10:46:06PM -0500, Jeff King wrote:
Show 16 quoted lines
> On Tue, Feb 15, 2011 at 08:16:21PM +0100, Heiko Voigt wrote:
> 
> > > It seems a little heavy-handed to have a dialog pop up for each commit.
> > > It's not actually dangerous to create a commit on a detached HEAD; it's
> > > just dangerous to _leave_ without referencing your new commits.
> > 
> > Hmm, how about adding a checkbox:
> > 
> >   [ ] Do not ask again
> > 
> > In my experience anything other than a popup will be overseen so I would
> > suggest doing it at least once to prepare the user for the possible
> > consequences.
> 
> Yeah, that's much better IMHO because at least clueful people can
> dismiss it after the first time.

I tried to implement such a dialog yesterday. Unfortunately my tcl/tk fu seems not sufficient enough. I managed to create the dialog but did not get the results which button was clicked. It has something to do with the scope of variables in tcl. Pat you would probably be able to fix this. I can send you the code if you are interested?

Show 15 quoted lines
> > > I do think a pop-up is appropriate when you try to check something else
> > > out, and commits you have made on the detached HEAD are about to become
> > > unreferenced. But this is something even the CLI doesn't do, so it would
> > > make sense to see how the check is implemented there first before doing
> > > anything in git-gui.
> > 
> > From what I read in this thread it currently seems to be not so easy to
> > precisely find out whether some commit is referenced. (If we care about
> > stuff outside of remotes, heads and tags). But maybe we do not need
> > that for the GUI.
> 
> Yeah, I think there is still some question about how it should happen,
> and any check in the gui should probably be the same as in the cli.  But
> from the rest of what you say, that shouldn't impact whether a
> per-commit warning is worth doing.

Will someone be working on this? Otherwise: Implementing the previously suggested solution is quite straightforward for git-gui. I could implement that check in git-gui first and once the CLI has some mechanism for this check we could switch to that.

Cheers Heiko
Victor Engmark· Feb 16, 2011, 16:11 UTC · lore

Re: [PATCH] git-gui: give more advice when detaching HEAD

On 02/15/2011 08:16 PM, Heiko Voigt wrote:
Show 31 quoted lines
> Hi,
> 
> On Tue, Feb 15, 2011 at 01:39:03AM -0500, Jeff King wrote:
>> On Sun, Feb 13, 2011 at 01:31:52PM +0100, Heiko Voigt wrote:
>>
>>> On Sat, Feb 12, 2011 at 02:05:38AM -0500, Jeff King wrote:
>>>>   1. Give some indication or warning during commit that you're in a
>>>>      detached state. The CLI template says "You are not on any branch"
>>>>      when editing the commit message, and mentions "detached HEAD" as
>>>>      the branch in the post-commit summary. As far as I can tell,
>>>>      git-gui says nothing at all.
>>>
>>> How about something like this:
>>> [...]
>>> Subject: [PATCH] git-gui: warn when trying to commit on a detached head
>>>
>>> The commandline is already warning when checking out a detached head.
>>> Since the only thing thats potentially dangerous is to create commits
>>> on a detached head lets warn in case the user is about to do that.
>>
>> It seems a little heavy-handed to have a dialog pop up for each commit.
>> It's not actually dangerous to create a commit on a detached HEAD; it's
>> just dangerous to _leave_ without referencing your new commits.
> 
> Hmm, how about adding a checkbox:
> 
>   [ ] Do not ask again
> 
> In my experience anything other than a popup will be overseen so I would
> suggest doing it at least once to prepare the user for the possible
> consequences.

That would be useful. However, there is only so much space in a dialog box (and only so much users will read in one), so to make sure users understand what is going on (and perhaps advocate some self-learning) there should be a link to more information.

2c,
-- 
Victor

← back to recent threads