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

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

[Jeff King]

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(-)

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

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

[Junio C Hamano]

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.
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.

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

[Jeff King]

On Fri, Feb 11, 2011 at 11:42:47PM -0800, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Sat, Feb 12, 2011 at 12:17:16AM -0800, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Want to do a proof-of-concept patch? Then we can get some real timings.

This was about 

> >   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(-)

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);
 

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

[Junio C Hamano]

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.

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

[Sverre Rabbelier]

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

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

[Johannes Sixt]

On Sonntag, 13. Februar 2011, Sverre Rabbelier wrote:
> 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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> 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.

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

[Heiko Voigt]

Hi,

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:

---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(-)

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

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

[Jeff King]

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.

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

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

[Heiko Voigt]

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

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

[Pat Thoyts]

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:
>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(-)

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

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

[Jeff King]

On Tue, Feb 15, 2011 at 07:48:33PM +0000, Pat Thoyts wrote:

> 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...

> +	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

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

[Heiko Voigt]

Hi,

On Tue, Feb 15, 2011 at 07:48:33PM +0000, Pat Thoyts wrote:
> 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>
> ---
[...]
> 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?

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

[Jeff King]

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.

> 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.

> 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.

> > 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

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

[Heiko Voigt]

Hi,

On Tue, Feb 15, 2011 at 10:46:06PM -0500, Jeff King wrote:
> 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?

> > > 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

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

[Victor Engmark]

On 02/15/2011 08:16 PM, Heiko Voigt wrote:
> 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