Proposal for git stash rename

Proposal for git stash rename

[Greg Hewgill]

I'd like to teach git-stash to rename stashes. Sometimes I'll create a
stash and then later think of a better or more descriptive name for it.
Or, I'll create a stash with "git stash -p" and forget to give it a
useful name, and there's no easy way to undo and redo that without
doing the interactive part again.

I noticed that although the stash message is used as the commit
description for the stash commit(s), it is the message in the reflog
that is actually displayed on "git stash list". So to rename a stash,
changing the message in the reflog is sufficient (this can be
demonstrated with a text editor).

My general idea is:

1. Implement a new "git reflog update" command that updates the message
associated with a specific reflog entry. To do this, a new
update_reflog_ent() function (in reflog.c) would change the message
associated with the specific reflog entry to update. An update_reflog()
function would use for_each_reflog_ent() with update_reflog_ent to
actually do the change.

2. A "git stash rename" command would then only need to call "git
reflog update" with the appropriate ref and new message.

Since this is my first time modifying the Git source, I thought I'd
solicit some feedback on this idea.

- Is this an appropriate approach to implementing this?
- Is there a better way to do it?
- I have a mostly working patch now, but it needs cleanup. What's the
  next step?

Greg Hewgill
http://hewgill.com

Re: Proposal for git stash rename

[Ævar Arnfjörð Bjarmason]

On Sun, Jun 20, 2010 at 09:31, Greg Hewgill <greg@hewgill.com> wrote:
> - Is this an appropriate approach to implementing this?
> - Is there a better way to do it?

No idea.

> - I have a mostly working patch now, but it needs cleanup. What's the
>  next step?

It's good to post a WIP PATCH even if it needs cleanup, just as a
point for further discussion.

Re: Proposal for git stash rename

[Greg Hewgill]

On Sun, Jun 20, 2010 at 10:54:43AM +0000, ??var Arnfj??r?? Bjarmason wrote:
> It's good to post a WIP PATCH even if it needs cleanup, just as a
> point for further discussion.

Thanks, point taken. WIP patch follows.

This patch implements a "git stash rename" using a new
"git reflog update" command that updates the message associated
with a reflog entry.
---
 builtin/reflog.c |  149 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh     |    5 ++
 t/t3903-stash.sh |   10 ++++
 3 files changed, 164 insertions(+), 0 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index ebf610e..35eae1f 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -7,6 +7,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "reachable.h"
+#include "strbuf.h"
 
 /*
  * reflog expire
@@ -16,6 +17,8 @@ static const char reflog_expire_usage[] =
 "git reflog expire [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
 static const char reflog_delete_usage[] =
 "git reflog delete [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...";
+static const char reflog_update_usage[] =
+"git reflog update [--verbose] [--dry-run] [--rewrite] <ref> <newdescr>";
 
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
@@ -30,6 +33,7 @@ struct cmd_reflog_expire_cb {
 	unsigned long expire_total;
 	unsigned long expire_unreachable;
 	int recno;
+	struct strbuf newmsg;
 };
 
 struct expire_reflog_cb {
@@ -335,6 +339,30 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 	return 0;
 }
 
+static int update_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
+{
+	struct expire_reflog_cb *cb = cb_data;
+
+	if (cb->cmd->rewrite)
+		osha1 = cb->last_kept_sha1;
+
+	if (cb->cmd->recno && --(cb->cmd->recno) == 0)
+		message = cb->cmd->newmsg.buf;
+
+	if (cb->newlog) {
+		char sign = (tz < 0) ? '-' : '+';
+		int zone = (tz < 0) ? (-tz) : tz;
+		fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
+			sha1_to_hex(osha1), sha1_to_hex(nsha1),
+			email, timestamp, sign, zone,
+			message);
+		hashcpy(cb->last_kept_sha1, nsha1);
+	}
+	return 0;
+}
+
 static int push_tip_to_list(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct commit_list **list = cb_data;
@@ -448,6 +476,65 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,
 	return status;
 }
 
+static int update_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
+{
+	struct cmd_reflog_expire_cb *cmd = cb_data;
+	struct expire_reflog_cb cb;
+	struct ref_lock *lock;
+	char *log_file, *newlog_path = NULL;
+	int status = 0;
+
+	memset(&cb, 0, sizeof(cb));
+
+	/*
+	 * we take the lock for the ref itself to prevent it from
+	 * getting updated.
+	 */
+	lock = lock_any_ref_for_update(ref, sha1, 0);
+	if (!lock)
+		return error("cannot lock ref '%s'", ref);
+	log_file = git_pathdup("logs/%s", ref);
+	if (!file_exists(log_file))
+		goto finish;
+	if (!cmd->dry_run) {
+		newlog_path = git_pathdup("logs/%s.lock", ref);
+		cb.newlog = fopen(newlog_path, "w");
+	}
+
+	cb.cmd = cmd;
+
+	for_each_reflog_ent(ref, update_reflog_ent, &cb);
+
+ finish:
+	if (cb.newlog) {
+		if (fclose(cb.newlog)) {
+			status |= error("%s: %s", strerror(errno),
+					newlog_path);
+			unlink(newlog_path);
+		} else if (cmd->updateref &&
+			(write_in_full(lock->lock_fd,
+				sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
+			 write_str_in_full(lock->lock_fd, "\n") != 1 ||
+			 close_ref(lock) < 0)) {
+			status |= error("Couldn't write %s",
+				lock->lk->filename);
+			unlink(newlog_path);
+		} else if (rename(newlog_path, log_file)) {
+			status |= error("cannot rename %s to %s",
+					newlog_path, log_file);
+			unlink(newlog_path);
+		} else if (cmd->updateref && commit_ref(lock)) {
+			status |= error("Couldn't set %s", lock->ref_name);
+		} else {
+			adjust_shared_perm(log_file);
+		}
+	}
+	free(newlog_path);
+	free(log_file);
+	unlock_ref(lock);
+	return status;
+}
+
 static int collect_reflog(const char *ref, const unsigned char *sha1, int unused, void *cb_data)
 {
 	struct collected_reflog *e;
@@ -752,6 +839,65 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
+static int cmd_reflog_update(int argc, const char **argv, const char *prefix)
+{
+	struct cmd_reflog_expire_cb cb;
+	int i, status = 0;
+
+	memset(&cb, 0, sizeof(cb));
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
+			cb.dry_run = 1;
+		else if (!strcmp(arg, "--rewrite"))
+			cb.rewrite = 1;
+		else if (!strcmp(arg, "--verbose"))
+			cb.verbose = 1;
+		else if (!strcmp(arg, "--")) {
+			i++;
+			break;
+		}
+		else if (arg[0] == '-')
+			usage(reflog_update_usage);
+		else
+			break;
+	}
+
+	if (argc - i < 2)
+		usage(reflog_update_usage);
+
+	strbuf_init(&cb.newmsg, strlen(argv[i+1])+1);
+	strbuf_addstr(&cb.newmsg, argv[i+1]);
+	strbuf_addstr(&cb.newmsg, "\n");
+
+	const char *spec = strstr(argv[i], "@{");
+	unsigned char sha1[20];
+	char *ep, *ref;
+	int recno;
+
+	if (!spec) {
+		return error("Not a reflog: %s", argv[i]);
+	}
+
+	if (!dwim_log(argv[i], spec - argv[i], sha1, &ref)) {
+		return error("no reflog for '%s'", argv[i]);
+	}
+
+	recno = strtoul(spec + 2, &ep, 10);
+	if (*ep == '}') {
+		cb.recno = -recno;
+		for_each_reflog_ent(ref, count_reflog_ent, &cb);
+	} else {
+		return error("specific ref please");
+	}
+
+	status |= update_reflog(ref, sha1, 0, &cb);
+	free(ref);
+
+	return status;
+}
+
 /*
  * main "reflog"
  */
@@ -777,6 +923,9 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
 	if (!strcmp(argv[1], "delete"))
 		return cmd_reflog_delete(argc - 1, argv + 1, prefix);
 
+	if (!strcmp(argv[1], "update"))
+		return cmd_reflog_update(argc - 1, argv + 1, prefix);
+
 	/* Not a recognized reflog command..*/
 	usage(reflog_usage);
 }
diff --git a/git-stash.sh b/git-stash.sh
index 1d95447..aa80897 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -8,6 +8,7 @@ USAGE="list [<options>]
    or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
    or: $dashless branch <branchname> [<stash>]
    or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [<message>]]
+   or: $dashless rename <stash> <message>
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -431,6 +432,10 @@ branch)
 	shift
 	apply_to_branch "$@"
 	;;
+rename)
+	shift
+	git reflog update $1 "$2"
+	;;
 *)
 	case $# in
 	0)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 8fe14cc..c0de00c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -378,4 +378,14 @@ test_expect_failure 'stash file to directory' '
 	test foo = "$(cat file/file)"
 '
 
+test_expect_success 'update stash message' '
+	git reset --hard &&
+	git checkout master &&
+	echo foo >file &&
+	git stash save "first message" &&
+	git stash list | grep "^stash@{0}: On master: first message$" &&
+	git stash rename stash@{0} "second message" &&
+	git stash list | grep "^stash@{0}: second message$"
+'
+
 test_done
-- 
1.6.6

Re: Proposal for git stash rename

[Micheil Smith]

Greg Hewgill <greg <at> hewgill.com> writes:

> 
> On Sun, Jun 20, 2010 at 10:54:43AM +0000, ??var Arnfj??r?? Bjarmason wrote:
> > It's good to post a WIP PATCH even if it needs cleanup, just as a
> > point for further discussion.
> 
> Thanks, point taken. WIP patch follows.
> 
> This patch implements a "git stash rename" using a new
> "git reflog update" command that updates the message associated
> with a reflog entry.
> ---
> [--snip--]

Hi, 

I note that this proposal is now two years old. A work in progress patch was 
requested, however, after one was given this thread ended. I'm also finding 
a need for this feature;

Not to try and bump an old thread, but what's the best way to land this?

– Micheil Smith
@miksago

Re: Proposal for git stash rename

[Junio C Hamano]

Micheil Smith <micheil@brandedcode.com> writes:

>> This patch implements a "git stash rename" using a new
>> "git reflog update" command that updates the message associated
>> with a reflog entry.
> ...
> I note that this proposal is now two years old. A work in progress patch was 
> requested, however, after one was given this thread ended. I'm also finding 
> a need for this feature;

The whole point of reflog is that it is a mechanism to let users to
go safely back to the previous state, by using a file that is pretty
much append-only.  It feels that a mechanism to "rewrite" one goes
completely against that principle, at least to me.

I have a feeling that "need" in "need for this feature" is a
misspelt "want", that occasional misspelling of the stash message
may give users awkward feelings when viewing "git stash list" output
but not severe enough to make them unable to identify which stash
entry holds which change, and that it is sufficient to pop and then
restash if a user *really* cares.

Re: Proposal for git stash rename

[Michael Haggerty]

On 01/04/2013 10:40 PM, Junio C Hamano wrote:
> Micheil Smith <micheil@brandedcode.com> writes:
> 
>>> This patch implements a "git stash rename" using a new
>>> "git reflog update" command that updates the message associated
>>> with a reflog entry.
>> ...
>> I note that this proposal is now two years old. A work in progress patch was 
>> requested, however, after one was given this thread ended. I'm also finding 
>> a need for this feature;
> 
> The whole point of reflog is that it is a mechanism to let users to
> go safely back to the previous state, by using a file that is pretty
> much append-only.  It feels that a mechanism to "rewrite" one goes
> completely against that principle, at least to me.

The implementation of "git stash" itself seems to violate your
principle, by storing its branches-that-are-not-branches within a
mutable reflog.

Just an observation...

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/