Re: [PATCH v2 6/7] rebase: write better reflog messages
Johannes Sixt wrote:
I haven't followed the topic closely, but I wonder why there are so many
explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action
(defined in git-sh-setup) the wrong thing to do?
Does this answer your question?
set_reflog_action() {
if [ -z ${GIT_REFLOG_ACTION:+set} ]
then
GIT_REFLOG_ACTION=$*
export GIT_REFLOG_ACTION
fi
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] rebase: write better reflog messages
Am 6/19/2013 8:09, schrieb Ramkumar Ramachandra:
Johannes Sixt wrote:
I haven't followed the topic closely, but I wonder why there are so many
explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action
(defined in git-sh-setup) the wrong thing to do?
Does this answer your question?
set_reflog_action() {
if [ -z ${GIT_REFLOG_ACTION:+set} ]
then
GIT_REFLOG_ACTION=$*
export GIT_REFLOG_ACTION
fi
}
Please don't state the obvious, that does not help. Of course, this does
not answer my question. I was rather hinting that it may be wrong to set
GIT_REFLOG_ACTION explicitly.
I thought that the convention is not to modify GIT_REFLOG_ACTION if it is
already set. set_reflog_action does just that.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] rebase: write better reflog messages
Johannes Sixt j.s...@viscovery.net writes: Am 6/18/2013 20:55, schrieb Ramkumar Ramachandra: Now that the checkout invoked internally from rebase knows to honor GIT_REFLOG_ACTION, we can start to use it to write a better reflog message when rebase anotherbranch, rebase --onto branch, etc. internally checks out the new fork point. We will write: rebase: checkout master instead of the old rebase diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 34e3102..69fae7a 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -5,11 +5,13 @@ case $action in continue) +GIT_REFLOG_ACTION=$base_reflog_action I haven't followed the topic closely, but I wonder why there are so many explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action (defined in git-sh-setup) the wrong thing to do? Excellent question, and I think this illustrates why the recent reroll that uses an approach to use base_reflog_action is not complete and needs further work (to put it mildly). set_reflog_action is designed to be used once at the very top of a program, like this in git am, for example: set_reflog_action am The helper function sets the given string to GIT_REFLOG_ACTION only when GIT_REFLOG_ACTION is not yet set. Thanks to this, git am, when run as the top-level program, will use am in GIT_REFLOG_ACTION and the reflog entries made by whatever it does will say am did this. Because of the conditional assignment, when git am is run as a subprogram (i.e. an implementation detail) of git rebase, the call to the helper function at the beginning will *not* have any effect. So git rebase can do this: set_reflog_action rebase ... do its own preparation, like checking out onto commit ... decide to do format-patch to am pipeline git format-patch --stdout mbox git am mbox and the reflog entries made inside git am invocation will say rebase, not am. The approach to introduce base_reflog_action may be valid, but if we were to go that route, set_reflog_action needs to learn the new convention. Perhaps by doing something like this: 1. set_reflog_action to set GIT_REFLOG_NAME and GIT_REFLOG_ACTION; Program names like am, rebase will be set to this value. 2. If the program does not want to say anything more than its program name in the reflog, it does not have to do anything. GIT_REFLOG_ACTION that is set upfront (or inherited from the calling program) will be written in the reflog. 3. If the program wants to say more than just its name, it needs to arrange GIT_REFLOG_ACTION not to be clobbered. It can do so in various ways: a) A one-shot invocation of any program that writes to reflog can do: GIT_REFLOG_ACTION=$GIT_REFLOG_NAME: custom message \ git cmd An important thing is that GIT_REFLOG_ACTION is left as the original, i.e. am or rebase, so that calls to programs that write reflog using the default (i.e. program name only) will not be affected. b) Or it can temporarily change it and revert it back (necessary to use shell function like output that cannot use the above single shot export technique): GIT_REFLOG_ACTION=$GIT_REFLOG_NAME: custom message output git cmd GIT_REFLOG_ACTION=$GIT_REFLOG_NAME But after writing it down this way, I realize that introduction of base_reflog_action (or GIT_REFLOG_NAME which is a moral equivalent) is not helping us at all. As long as calls to git command in the second category exists in these scripts, GIT_REFLOG_ACTION *must* be kept pristine after set_reflog_action sets it, so we can get rid of this new variable, and rewrite 3.a and 3.b like so: 3-a) GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: custom message \ git cmd 3-b) SAVED=$GIT_REFLOG_ACTION GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: custom message output git cmd GIT_REFLOG_ACTION=$SAVED or ( GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: custom message output git cmd ) That essentially boils down to the very original suggestion I made before Ram introduced the base_reflog_action. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] rebase: write better reflog messages
Junio C Hamano gits...@pobox.com writes:
Excellent question, and I think this illustrates why the recent
reroll that uses an approach to use base_reflog_action is not
complete and needs further work (to put it mildly).
...
That essentially boils down to the very original suggestion I made
before Ram introduced the base_reflog_action.
So how about doing something like this?
Incidentally, I noticed that GIT_LITERAL_PATHSPECS:: heading in the
enumeration of environment variables is marked-up differently from
others, which is a low-hanging fruit somebody may want to fix.
Documentation/git-sh-setup.txt | 8 +---
Documentation/git.txt | 10 ++
git-sh-setup.sh| 34 ++
3 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 5d709d0..4f67c4c 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt
@@ -41,9 +41,11 @@ usage::
die with the usage message.
set_reflog_action::
- set the message that will be recorded to describe the
- end-user action in the reflog, when the script updates a
- ref.
+ Set GIT_REFLOG_ACTION environment to a given string (typically
+ the name of the program) unless it is already set. Whenever
+ the script runs a `git` command that updates refs, a reflog
+ entry is created using the value of this string to leave the
+ record of what command updated the ref.
git_editor::
runs an editor of user's choice (GIT_EDITOR, core.editor, VISUAL or
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2e23cbb..e2bdcc9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -846,6 +846,16 @@ GIT_LITERAL_PATHSPECS::
literal paths to Git (e.g., paths previously given to you by
`git ls-tree`, `--raw` diff output, etc).
+'GIT_REFLOG_ACTION'::
+ When a ref is updated, reflog entries are created to keep
+ track of the reason why the ref was updated (which is
+ typically the name of the high-level command that updated
+ the ref), in addition to the old and new values of the ref.
+ A scripted Porcelain command can use set_reflog_action
+ helper function in `git-sh-setup` to set its name to this
+ variable when it is invoked as the top level command by the
+ end user, to be recorded in the body of the reflog.
+
Discussion[[Discussion]]
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 2f78359..e5379bc 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -103,6 +103,40 @@ $LONG_USAGE
esac
fi
+# Set the name of the end-user facing command in the reflog when the
+# script may update refs. When GIT_REFLOG_ACTION is already set, this
+# will not overwrite it, so that a scripted Porcelain (e.g. git
+# rebase) can set it to its own name (e.g. rebase) and then call
+# another scripted Porcelain (e.g. git am) and a call to this
+# function in the latter will keep the name of the end-user facing
+# program (e.g. rebase) in GIT_REFLOG_ACTION, ensuring whatever it
+# does will be record as actions done as part of the end-user facing
+# operation (e.g. rebase).
+#
+# NOTE NOTE NOTE: consequently, after assigning a specific message to
+# GIT_REFLOG_ACTION when calling a git command to record a custom
+# reflog message, do not leave that custom value in GIT_REFLOG_ACTION,
+# after you are done. Other callers of git commands that rely on
+# writing the default program name in reflog expect the variable to
+# contain the value set by this function.
+#
+# To use a custom reflog message, do either one of these three:
+#
+# (a) use a single-shot export form:
+# GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: preparing frotz \
+# git command-that-updates-a-ref
+#
+# (b) save the original away and restore:
+# SAVED_ACTION=$GIT_REFLOG_ACTION
+# GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: preparing frotz
+# git command-that-updates-a-ref
+# GIT_REFLOG_ACITON=$SAVED_ACTION
+#
+# (c) assign the variable in a subshell:
+# (
+# GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: preparing frotz
+# git command-that-updates-a-ref
+# )
set_reflog_action() {
if [ -z ${GIT_REFLOG_ACTION:+set} ]
then
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] rebase: write better reflog messages
Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: Excellent question, and I think this illustrates why the recent reroll that uses an approach to use base_reflog_action is not complete and needs further work (to put it mildly). ... That essentially boils down to the very original suggestion I made before Ram introduced the base_reflog_action. So how about doing something like this? Having said all that, although I think the something like this patch is an improvement in that it spells out the rules regarding the use of GIT_REFLOG_ACTION environment variable, which was not documented so far, I think the environment variable is showing its flaws. It was a good mechanism in simpler times, back when git commit, git fetch and git merge were its primary users. They didn't do many ref updates, and having a way to record that this update was done by a 'merge' command initiated by the end user was perfectly adequate. For a command like rebase that can do many ref updates, having to set a custom message and export the variable in each and every step is cumbersome, and keeping the same prefix across becomes even more so. The $orig_reflog_action used inside git-rebase--interactive is a reasonable local solution for the keeping the same prefix problem, but it is a _local_ solution that does not scale. In the end, it updates the GIT_REFLOG_ACTION variable, so the script has to be very careful to make sure the variable has a sensible value before calling any ref-updating git command. It will have to set it back to $orig_reflog_action if it ever wants to call another scripted Porcelain. Among the C infrastructure, commit, fetch, merge and reset are the only ones that pay attention to GIT_REFLOG_ACTION, and we will be adding checkout to the mix. If we originally did not make the mistake of using GIT_REFLOG_ACTION as a whole message, and instead used it to convey _only_ the prefix (i.e. rebase, am, etc.) to subprocesses in order to remember what the end-user initiated command was, and used a command line argument to give the actual messages, we would have been in much better shape. E.g. a checkout call inside git rebase may become git checkout \ --reflog-message=$GIT_REFLOG_ACTION: detaching \ $onto^0 and nobody other than set_reflog_action shell function would be setting GIT_REFLOG_ACTION variable. Oh well. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 6/7] rebase: write better reflog messages
Now that the checkout invoked internally from rebase knows to honor
GIT_REFLOG_ACTION, we can start to use it to write a better reflog
message when rebase anotherbranch, rebase --onto branch,
etc. internally checks out the new fork point. We will write:
rebase: checkout master
instead of the old
rebase
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
git-am.sh | 4 +++-
git-rebase--am.sh | 5 +
git-rebase.sh | 13 +++--
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/git-am.sh b/git-am.sh
index 1cf3d1d..74ef9ca 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -46,6 +46,8 @@ set_reflog_action am
require_work_tree
cd_to_toplevel
+base_reflog_action=$GIT_REFLOG_ACTION
+
git var GIT_COMMITTER_IDENT /dev/null ||
die $(gettext You need to set your committer info first)
@@ -884,7 +886,7 @@ did you forget to use 'git add'?
fi
git commit-tree $tree ${parent:+-p} $parent
$dotest/final-commit
)
- git update-ref -m $GIT_REFLOG_ACTION: $FIRSTLINE HEAD $commit $parent
||
+ git update-ref -m $base_reflog_action: $FIRSTLINE HEAD $commit
$parent ||
stop_here $this
if test -f $dotest/original-commit; then
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 34e3102..69fae7a 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -5,11 +5,13 @@
case $action in
continue)
+ GIT_REFLOG_ACTION=$base_reflog_action
git am --resolved --resolvemsg=$resolvemsg
move_to_original_branch
return
;;
skip)
+ GIT_REFLOG_ACTION=$base_reflog_action
git am --skip --resolvemsg=$resolvemsg
move_to_original_branch
return
@@ -40,9 +42,11 @@ else
rm -f $GIT_DIR/rebased-patches
case $head_name in
refs/heads/*)
+ GIT_REFLOG_ACTION=$base_reflog_action: checkout
$head_name
git checkout -q $head_name
;;
*)
+ GIT_REFLOG_ACTION=$base_reflog_action: checkout
$orig_head
git checkout -q $orig_head
;;
esac
@@ -59,6 +63,7 @@ else
return $?
fi
+ GIT_REFLOG_ACTION=$base_reflog_action
git am $git_am_opt --rebasing --resolvemsg=$resolvemsg
$GIT_DIR/rebased-patches
ret=$?
diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..1c72120 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -47,6 +47,8 @@ set_reflog_action rebase
require_work_tree_exists
cd_to_toplevel
+base_reflog_action=$GIT_REFLOG_ACTION
+
LF='
'
ok_to_skip_pre_rebase=
@@ -336,7 +338,8 @@ then
# Only interactive rebase uses detailed reflog messages
if test $type = interactive test $GIT_REFLOG_ACTION = rebase
then
- GIT_REFLOG_ACTION=rebase -i ($action)
+ GIT_REFLOG_ACTION=rebase -i ($1)
+ base_reflog_action=$GIT_REFLOG_ACTION
export GIT_REFLOG_ACTION
fi
fi
@@ -543,7 +546,11 @@ then
if test -z $force_rebase
then
# Lazily switch to the target branch if needed...
- test -z $switch_to || git checkout $switch_to --
+ if test -n $switch_to
+ then
+ GIT_REFLOG_ACTION=$base_reflog_action: checkout
$switch_to
+ git checkout $switch_to --
+ fi
say $(eval_gettext Current branch \$branch_name is up to
date.)
exit 0
else
@@ -568,6 +575,8 @@ test $type = interactive run_specific_rebase
# Detach HEAD and reset the tree
say $(gettext First, rewinding head to replay your work on top of it...)
+
+GIT_REFLOG_ACTION=$base_reflog_action: checkout $onto_name
git checkout -q $onto^0 || die could not detach HEAD
git update-ref ORIG_HEAD $orig_head
--
1.8.3.1.455.g5932b31
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] rebase: write better reflog messages
Ramkumar Ramachandra artag...@gmail.com writes: Now that the checkout invoked internally from rebase knows to honor GIT_REFLOG_ACTION, we can start to use it to write a better reflog message when rebase anotherbranch, rebase --onto branch, etc. internally checks out the new fork point. We will write: rebase: checkout master instead of the old rebase Signed-off-by: Ramkumar Ramachandra artag...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com --- So the approach taken by this round is to change everybody's assumption so far that they can start from a clean and usable GIT_REFLOG_ACTION when creating their own message, and stop depending on what is left in GIT_REFLOG_ACTION. That would certainly allow this patch to leave whatever cruft in GIT_REFLOG_ACTION, because everybody else will now create the value to be assigned to that variable from scratch based on a new and different variable. All existing everybody else is converted to adjust to the new reality. A one-shot assignment VAR=VAL git checkout is sometimes cumbersome to arrange, especially when what calls the git checkout is wrapped in a shell function like output, I think this is an OK approach. If we are adopting that convention, however, the new variable that holds the name of the overall program, base_reflog_action, needs to be a bit more prominently documented in the code, to let the other people know that is the new convention to follow. Something like... # Use this as the prefix when setting and exporting # GIT_REFLOG_ACTION variable. base_reflog_action=am And the fact that we are declaring the new convention and expecting everybody to stick to it should be in the log message. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/7] rebase: write better reflog messages
Am 6/18/2013 20:55, schrieb Ramkumar Ramachandra: Now that the checkout invoked internally from rebase knows to honor GIT_REFLOG_ACTION, we can start to use it to write a better reflog message when rebase anotherbranch, rebase --onto branch, etc. internally checks out the new fork point. We will write: rebase: checkout master instead of the old rebase diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 34e3102..69fae7a 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -5,11 +5,13 @@ case $action in continue) + GIT_REFLOG_ACTION=$base_reflog_action I haven't followed the topic closely, but I wonder why there are so many explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action (defined in git-sh-setup) the wrong thing to do? -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
