[PATCH 0/5] fetch & co: misc output cleanup

[PATCH 0/5] fetch & co: misc output cleanup

[Santi Béjar]

Hi *,

this patchset includes:

      fetch: Reset remote refs list each time fetch_main is called
      fetch & co: Use "hash1..hash2" instead of "from hash1 to hash2"
      fetch & co: Use short sha1 in the output
      fetch: Add output for the not fast forward case
      fetch: Clean output

The first one is actually a bugfix (at least for me), but needed for the
last one.

 git-fetch.sh   |   19 ++++++++++++++-----
 git-merge.sh   |    2 +-
 git-resolve.sh |    2 +-
 3 files changed, 16 insertions(+), 7 deletions(-)

[PATCH 1/5] fetch: Reset remote refs list each time fetch_main is called

[Santi Béjar]

This prevents the fetch of the heads again in the second call of fetch_main.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 git-fetch.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-fetch.sh b/git-fetch.sh
index bcc67ab..f1522bd 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -257,6 +257,7 @@ fi
 fetch_main () {
   reflist="$1"
   refs=
+  rref=
 
   for ref in $reflist
   do
-- 
1.4.2.1.g38049

[PATCH 2/5] fetch & co: Use "hash1..hash2" instead of "from hash1 to hash2"

[Santi Béjar]

I find it shorter, easier to copy&paste and cleaner.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 git-fetch.sh   |    2 +-
 git-merge.sh   |    2 +-
 git-resolve.sh |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-fetch.sh b/git-fetch.sh
index f1522bd..08d86cd 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -182,7 +182,7 @@ fast_forward_local () {
 		;;
 	    *,$local)
 		echo >&2 "* $1: fast forward to $3"
-		echo >&2 "  from $local to $2"
+		echo >&2 "  $local..$2"
 		git-update-ref -m "$rloga: fast-forward" "$1" "$2" "$local"
 		;;
 	    *)
diff --git a/git-merge.sh b/git-merge.sh
index 5b34b4d..fd587c5 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -197,7 +197,7 @@ f,*)
 	;;
 ?,1,"$head",*)
 	# Again the most common case of merging one remote.
-	echo "Updating from $head to $1"
+	echo "Updating $head..$1"
 	git-update-index --refresh 2>/dev/null
 	new_head=$(git-rev-parse --verify "$1^0") &&
 	git-read-tree -u -v -m $head "$new_head" &&
diff --git a/git-resolve.sh b/git-resolve.sh
index 729ec65..6e4fb02 100755
--- a/git-resolve.sh
+++ b/git-resolve.sh
@@ -46,7 +46,7 @@ case "$common" in
 	exit 0
 	;;
 "$head")
-	echo "Updating from $head to $merge"
+	echo "Updating $head..$merge"
 	git-read-tree -u -m $head $merge || exit 1
 	git-update-ref -m "resolve $merge_name: Fast forward" \
 		HEAD "$merge" "$head"
-- 
1.4.2.1.g38049

[PATCH 3/5] fetch & co: Use short sha1 in the output

[Santi Béjar]

... for brevity.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 git-fetch.sh   |    6 +++---
 git-merge.sh   |    2 +-
 git-resolve.sh |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-fetch.sh b/git-fetch.sh
index 08d86cd..32553f8 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -129,11 +129,11 @@ append_fetch_head () {
     then
 	headc_=$(git-rev-parse --verify "$head_^0") || exit
 	echo "$headc_	$not_for_merge_	$note_" >>"$GIT_DIR/FETCH_HEAD"
-	[ "$verbose" ] && echo >&2 "* committish: $head_"
+	[ "$verbose" ] && echo >&2 "* committish: $(git-rev-parse --short $head_)"
 	[ "$verbose" ] && echo >&2 "  $note_"
     else
 	echo "$head_	not-for-merge	$note_" >>"$GIT_DIR/FETCH_HEAD"
-	[ "$verbose" ] && echo >&2 "* non-commit: $head_"
+	[ "$verbose" ] && echo >&2 "* non-commit: $(git-rev-parse --short $head_)"
 	[ "$verbose" ] && echo >&2 "  $note_"
     fi
     if test "$local_name_" != ""
@@ -182,7 +182,7 @@ fast_forward_local () {
 		;;
 	    *,$local)
 		echo >&2 "* $1: fast forward to $3"
-		echo >&2 "  $local..$2"
+		echo >&2 "  $(git-rev-parse --short $local)..$(git-rev-parse --short $2)"
 		git-update-ref -m "$rloga: fast-forward" "$1" "$2" "$local"
 		;;
 	    *)
diff --git a/git-merge.sh b/git-merge.sh
index fd587c5..49c46d5 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -197,7 +197,7 @@ f,*)
 	;;
 ?,1,"$head",*)
 	# Again the most common case of merging one remote.
-	echo "Updating $head..$1"
+	echo "Updating $(git-rev-parse --short $head)..$(git-rev-parse --short $1)"
 	git-update-index --refresh 2>/dev/null
 	new_head=$(git-rev-parse --verify "$1^0") &&
 	git-read-tree -u -v -m $head "$new_head" &&
diff --git a/git-resolve.sh b/git-resolve.sh
index 6e4fb02..36b90e3 100755
--- a/git-resolve.sh
+++ b/git-resolve.sh
@@ -46,7 +46,7 @@ case "$common" in
 	exit 0
 	;;
 "$head")
-	echo "Updating $head..$merge"
+	echo "Updating $(git-rev-parse --short $head)..$(git-rev-parse --short $merge)"
 	git-read-tree -u -m $head $merge || exit 1
 	git-update-ref -m "resolve $merge_name: Fast forward" \
 		HEAD "$merge" "$head"
-- 
1.4.2.1.g38049

[PATCH 4/5] fetch: Add output for the not fast forward case

[Santi Béjar]

Use the ... notation to be able to use this directly in "git log" to see
how the two non-fast-forward heads have diverged.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 git-fetch.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/git-fetch.sh b/git-fetch.sh
index 32553f8..ee4f5bd 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -190,6 +190,7 @@ fast_forward_local () {
 		;;
 	    esac || {
 		echo >&2 "* $1: does not fast forward to $3;"
+		echo >&2 "  $(git-rev-parse --short $local)...$(git-rev-parse --short $2)"
 		case ",$force,$single_force," in
 		*,t,*)
 			echo >&2 "  forcing update."
-- 
1.4.2.1.g38049

[PATCH 5/5] fetch: Clean output

[Santi Béjar]

Do not show duplicated remote branch information and reformat the output as:

$ git fetch -v   # the committish lines for the -v.
* refs/heads/origin: fast forward to remote branch 'master' of ../git/
  1ad7a06..bc1a580
  committish: bc1a580
* refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/;
  7c733a8...5faa935
  not updating.
  forcing update.
  committish: 5faa935
* refs/heads/next: same as remote branch 'origin/next' of ../git/
  committish: ce47b9f
...
* refs/tags/v1.4.2-rc4: storing tag 'v1.4.2-rc4' of ../git/
  committish: 8c7a107

$ git fetch -v origin refs/heads/master
* committish: 695dffe
  branch 'master' of ../git/

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 git-fetch.sh |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/git-fetch.sh b/git-fetch.sh
index ee4f5bd..cc71612 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -129,19 +129,26 @@ append_fetch_head () {
     then
 	headc_=$(git-rev-parse --verify "$head_^0") || exit
 	echo "$headc_	$not_for_merge_	$note_" >>"$GIT_DIR/FETCH_HEAD"
-	[ "$verbose" ] && echo >&2 "* committish: $(git-rev-parse --short $head_)"
-	[ "$verbose" ] && echo >&2 "  $note_"
+	committish=committish
     else
 	echo "$head_	not-for-merge	$note_" >>"$GIT_DIR/FETCH_HEAD"
-	[ "$verbose" ] && echo >&2 "* non-commit: $(git-rev-parse --short $head_)"
-	[ "$verbose" ] && echo >&2 "  $note_"
+	committish=non-commit
     fi
+    star_prefix="*"
     if test "$local_name_" != ""
     then
 	# We are storing the head locally.  Make sure that it is
 	# a fast forward (aka "reverse push").
 	fast_forward_local "$local_name_" "$head_" "$note_"
+	exit=$?
+	star_prefix=" "
     fi
+    if test "$verbose"
+    then
+	echo >&2 "$star_prefix $committish: $(git-rev-parse --short $head_)"
+	[ -z "$local_name_" ] && echo >&2 "  $note_"
+    fi
+    return ${exit:-0}
 }
 
 fast_forward_local () {
-- 
1.4.2.1.g38049

Re: [PATCH 0/5] fetch & co: misc output cleanup

[A Large Angry SCM]

Santi Béjar wrote:
> this patchset includes:
> 
>       fetch: Reset remote refs list each time fetch_main is called
>       fetch & co: Use "hash1..hash2" instead of "from hash1 to hash2"
>       fetch & co: Use short sha1 in the output
>       fetch: Add output for the not fast forward case
>       fetch: Clean output

Please do not make short ID output the default. That is, unless you can 
somehow guarantee that the short ID reported today will _always_ be 
unambiguous even with future additions to the repository.

Re: [PATCH 0/5] fetch & co: misc output cleanup

[Santi]

2006/9/29, A Large Angry SCM <gitzilla@gmail.com>:
> Santi Béjar wrote:
> > this patchset includes:
> >
> >       fetch: Reset remote refs list each time fetch_main is called
> >       fetch & co: Use "hash1..hash2" instead of "from hash1 to hash2"
> >       fetch & co: Use short sha1 in the output
> >       fetch: Add output for the not fast forward case
> >       fetch: Clean output
>
> Please do not make short ID output the default. That is, unless you can
> somehow guarantee that the short ID reported today will _always_ be
> unambiguous even with future additions to the repository.
>

There is not such guarantee. But the fetch output is to be use
immediately in git-log, etc. If you want these "guarantees" you must
know that you have to keep the long version.

By the way, is there a way to show all the commits,tags,... that
abbreviate to a short hash? This way instead of just saying "bad
revision" we could show the list of possible revisions.

Also, git-log & co also output abbreviated hashes by default. So why
not for git-fetch?

Santi

Re: [PATCH 0/5] fetch & co: misc output cleanup

[Junio C Hamano]

A Large Angry SCM <gitzilla@gmail.com> writes:

> Santi Béjar wrote:
>> this patchset includes:
>>
>>       fetch: Reset remote refs list each time fetch_main is called
>>       fetch & co: Use "hash1..hash2" instead of "from hash1 to hash2"
>>       fetch & co: Use short sha1 in the output
>>       fetch: Add output for the not fast forward case
>>       fetch: Clean output
>
> Please do not make short ID output the default. That is, unless you
> can somehow guarantee that the short ID reported today will _always_
> be unambiguous even with future additions to the repository.

I would understand your worries of spitting out only abbreviated
object names if they appear in a document to be preserved for a
long term.  But we are talking about output to stderr to help
people see what happened in the operation they initiated a few
moments ago, and to help them use the output for cut&paste while
it is still in the scrollbuffer of the terminal.

Shortening them would help A..B avoid crossing the line
boundaries, which terminal emulators have trouble with cutting
sometimes.  That benefit may outweigh the longer term ambiguity
concerns.

The only real-life situation I can imagine the program output is
preserved in any longer term than immediate consumption for
cut&paste is to paste it into a piece of e-mail (which is
archived), to show exactly what the program said to the user.
But then it carries the timestamp and you should be able to
figure out what the abbreviation uniquely stood for back then if
you really cared.  If you are producing a document for longer
term storage, the program that produces such a document should
be doing its own rev-parse anyway (or maybe pass whatever it is
piping through git-name-rev --stdin).

I think it is Ok to always use the abbreviated form in the
places Santi's patch touched, as long as people understand these
messages are not for long term storage.  We might want to use a
bit longer abbreviation than the default (7 hexdigits).

Re: [PATCH 5/5] fetch: Clean output

[Junio C Hamano]

Santi Béjar <sbejar@gmail.com> writes:

> Do not show duplicated remote branch information and reformat the output as:
>
> $ git fetch -v   # the committish lines for the -v.
> * refs/heads/origin: fast forward to remote branch 'master' of ../git/
>   1ad7a06..bc1a580
>   committish: bc1a580

I am not quite sure about this --- it is not obvious what these
two numbers represent anymore.  Also I think the last line
outlived its usefulness (99% of the time refs are committish, so
noting exception is good but otherwise it is not interesting).

I know you opted for minimum patch, but it might be a good time
to polish the wording a bit while we are touching the general
vicinity of the code.

How about saying something like:

 * refs/heads/origin: fast forward to remote branch 'master' of ../git/
   old..new = 1ad7a06..bc1a580

> * refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/;
>   7c733a8...5faa935
>   not updating.
>   forcing update.
>   committish: 5faa935

This is even more confusing.  Perhaps we would want to have two
cases, depending on --force (and +).

 * refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/;
   but forcing update anyway.  old...new = 7c733a8...5faa935

 * refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/;
   not updating.  old...new = 7c733a8...5faa935

Re: [PATCH 5/5] fetch: Clean output

[Santi]

2006/9/30, Junio C Hamano <junkio@cox.net>:
> Santi Béjar <sbejar@gmail.com> writes:
>
> > Do not show duplicated remote branch information and reformat the output as:
> >
> > $ git fetch -v   # the committish lines for the -v.
> > * refs/heads/origin: fast forward to remote branch 'master' of ../git/
> >   1ad7a06..bc1a580
> >   committish: bc1a580
>
> I am not quite sure about this --- it is not obvious what these
> two numbers represent anymore.  Also I think the last line
> outlived its usefulness (99% of the time refs are committish, so
> noting exception is good but otherwise it is not interesting).

I agree in the case of updating a ref, but I would keep it otherwise
(and even in this case only when -v is given).

>
> I know you opted for minimum patch, but it might be a good time
> to polish the wording a bit while we are touching the general
> vicinity of the code.
>
> How about saying something like:
>
>  * refs/heads/origin: fast forward to remote branch 'master' of ../git/
>    old..new = 1ad7a06..bc1a580

I like it.

>
> > * refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/;
> >   7c733a8...5faa935
> >   not updating.
> >   forcing update.
> >   committish: 5faa935
>
> This is even more confusing.  Perhaps we would want to have two
> cases, depending on --force (and +).

Sorry, it was from two copy'n'paste. I did it in a new repository
(without the +), then it stoped at this point (I suppose to force the
user to solve the problem). Then I put the + and did the copy'n'paste
again without deleting this extra line.

>
>  * refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/;
>    but forcing update anyway.  old...new = 7c733a8...5faa935

With the old...new in a new line?

So at the end, something like this output?

$ git fetch -v   # the committish lines for the -v.
* refs/heads/origin: fast forward to remote branch 'master' of ../git/
 old..new: 1ad7a06..bc1a580
* refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/
 old...new: 7c733a8...5faa935
 forcing update.
* refs/heads/next: same as remote branch 'origin/next' of ../git/
 committish: ce47b9f
...
* refs/tags/v1.4.2-rc4: storing tag 'v1.4.2-rc4' of ../git/
 committish: 8c7a107

$ git fetch -v origin refs/heads/master
* committish: 695dffe
 branch 'master' of ../git/

Santi

Re: [PATCH 5/5] fetch: Clean output

[Junio C Hamano]

Santi <sbejar@gmail.com> writes:

> So at the end, something like this output?
>
> $ git fetch -v   # the committish lines for the -v.
> * refs/heads/origin: fast forward to remote branch 'master' of ../git/
> old..new: 1ad7a06..bc1a580
> * refs/heads/pu: does not fast forward to remote branch 'pu' of ../git/
> old...new: 7c733a8...5faa935
> forcing update.
> * refs/heads/next: same as remote branch 'origin/next' of ../git/
> committish: ce47b9f
> ...
> * refs/tags/v1.4.2-rc4: storing tag 'v1.4.2-rc4' of ../git/
> committish: 8c7a107
>
> $ git fetch -v origin refs/heads/master
> * committish: 695dffe
> branch 'master' of ../git/

For the last one, the second and third line feel backwards.

Most cases in your summary (i.e. storing on a tracking branch or
tag) in general look like:

        * <local ref name>: <what happened and where it came from>
	<label>: <object names>

where <label> (e.g. old..new) describes what <object names>
mean.  So to be consistent, the <label> for "same as" case
(i.e. storing what is fetched in a local ref, but it was already
the same) and "storing anew" case, we might want to say their
true object types, not "committish".

Also we probably would want to "s/remote branch/branch" in the
"where from" part, since we always say "remote" in fetch anyway.

Then the last case where we do not use local tracking branch
would simply be something like:

	* fetched branch 'master' of ../git
        commit: 695dffe

How about this?  Essentially the same as the above but with
minor tweaks.

1. the ones we store in our local ref (either branches or tags):

 1a) fast-forward

  * refs/heads/origin: fast forward to branch 'master' of ../git/
    old..new: 1ad7a06..bc1a580

 1b) same (only shown under -v)

  * refs/heads/next: same as branch 'origin/next' of ../git/
    commit: ce47b9f

 1c) non-fast-forward, forced

  * refs/heads/pu: forcing update to branch 'pu' of ../git/
    old...new: 7c733a8...5faa935

 1d) non-fast-forward, did not update because not forced

  * refs/heads/po: does not fast-forward to branch 'po' of ../git/
    old...new: 7c733a8...5faa935

 1e) creating a new local ref to store

  * refs/tags/v1.4.2-rc4: storing tag 'v1.4.2-rc4' of ../git/
    tag: 8c7a107

2. the ones we do not store in our local ref (only shown under -v):

  * fetched branch 'master' of ../git
    commit: 695dffe

I think we should reword the "what happened" for 1d to clarify
that we did not touch refs/heads/po, but other than that I am
reasonably happy about how they look consistent.

But please don't take the above too seriously.  As everybody
should be very well aware by now, I am very bad at UI designs.