[PATCH 0/2] Slightly prettier reflog message from checkout

[1/2] is important.  [2/2] is a minor prettification, that wouldn't
have been possible without [1/2].

Thanks.

Ramkumar Ramachandra (2):
  sha1_name: stop hard-coding 40-character hex checks
  checkout: do not write full sha1 to reflog

 builtin/checkout.c | 2 +-
 sha1_name.c        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.8.3.1.438.g96d34e8

In two places, get_sha1_basic() assumes that strings are possibly sha1
hexes if they are 40 characters long, and calls get_sha1_hex() in these
two cases.  This 40-character check is ugly and wrong: there is nothing
preventing a revision or branch name from being exactly 40 characters.
Replace it with a call to the more robust get_short_sha1().

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 sha1_name.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 90419ef..d862af3 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -451,7 +451,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	int refs_found = 0;
 	int at, reflog_len, nth_prior = 0;
 
-	if (len == 40 && !get_sha1_hex(str, sha1)) {
+	if (!get_short_sha1(str, strlen(str), sha1, GET_SHA1_QUIETLY)) {
 		refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
 		if (refs_found > 0 && warn_ambiguous_refs) {
 			warning(warn_msg, len, str);

@@ -492,9 +492,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 		int detached;
 
 		if (interpret_nth_prior_checkout(str, &buf) > 0) {
-			detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
+			detached = get_short_sha1(buf.buf, buf.len, sha1, GET_SHA1_QUIETLY);
 			strbuf_release(&buf);
-			if (detached)
+			if (detached != SHORT_NAME_NOT_FOUND)
 				return 0;
 		}
 	}

-- 
1.8.3.1.438.g96d34e8

A snippet from a typical git session may look like this:

  $ git checkout @~3
  ...
  $ git checkout -

The reflog entries corresponding to the two checkouts look like:

  f855138: checkout: moving from bdff0e3a374617dce784f801b97500d9ba2e4705 to co-reflog
  bdff0e3: checkout: moving from co-reflog to HEAD~3

There is no need to write the full SHA-1 to the user-visible reflog; use
find_unique_abbrev() to shorten the first line like:

  f855138: checkout: moving from bdff0e3 to co-reflog

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f5b50e5..41b1929 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -619,7 +619,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 
 	old_desc = old->name;
 	if (!old_desc && old->commit)
-		old_desc = sha1_to_hex(old->commit->object.sha1);
+		old_desc = find_unique_abbrev(old->commit->object.sha1, DEFAULT_ABBREV);
 	strbuf_addf(&msg, "checkout: moving from %s to %s",
 		    old_desc ? old_desc : "(invalid)", new->name);
 

-- 
1.8.3.1.438.g96d34e8

Ramkumar Ramachandra wrote:

[1/2] is important.  [2/2] is a minor prettification, that wouldn't
have been possible without [1/2].

I forgot to mention: some tests fail, and I'm investigating.  This is
an early preview.

Ramkumar Ramachandra [off-list ref] writes:

[1/2] is important.  [2/2] is a minor prettification, that wouldn't
have been possible without [1/2].

Thanks.

Ramkumar Ramachandra (2):
  sha1_name: stop hard-coding 40-character hex checks
  checkout: do not write full sha1 to reflog

 builtin/checkout.c | 2 +-
 sha1_name.c        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

I view the two codepaths touched by these patches the other way
around.

An abbreviated unique SHA-1 you have today may not be unique
tomorrow.  There is no reason to deliberately lose information
(e.g. by using "Then, instead of the absolute minimum, let's record
a bit more bytes" heuristics) when we record. The reflog recording
code in checkout writes full 40-characters on purpose and there is
no reason not to do so (i.e. the codepath that is the topic of 2/2).

That is a more important design decision between the two codepaths.

Once we accept that design principle of not losing information when
we do not have to, it naturally follows that the writing side should
write full 40-hex, and also the reading side (i.e. the codepath that
is the topic of 1/2) should make sure that it reads 40-hex and
nothing else.  This also reduces the risk of a funny branch name
that consists only of [0-9a-f] getting mistaken as an object name,
but that is not the primary point.

So I am fairly strongly negative on both changes.

Junio C Hamano wrote:

I view the two codepaths touched by these patches the other way
around.

I see.  Thanks for the early feedback.  I have some doubts.

An abbreviated unique SHA-1 you have today may not be unique
tomorrow.  There is no reason to deliberately lose information
(e.g. by using "Then, instead of the absolute minimum, let's record
a bit more bytes" heuristics) when we record. The reflog recording
code in checkout writes full 40-characters on purpose and there is
no reason not to do so (i.e. the codepath that is the topic of 2/2).

When did we guarantee that the messages written by the reflog are invariant?

  $ git checkout @^
  $ git reflog | head -n 1
  b1d94f2 HEAD@{2 seconds ago}: checkout: moving from checkout-dash to HEAD^

What does HEAD^ even mean?  What guarantees that checkout-dash will
not be something else tomorrow?  If you want invariance, isn't that
what the first field is for (b1d94f2)?  As I understand it, the
messages are purely to convey end-user information about the
breadcrumb trail: they were later made semi-semantic (like the @{-N}
parser using them).

Once we accept that design principle of not losing information when
we do not have to, it naturally follows that the writing side should
write full 40-hex, and also the reading side (i.e. the codepath that
is the topic of 1/2) should make sure that it reads 40-hex and
nothing else.  This also reduces the risk of a funny branch name
that consists only of [0-9a-f] getting mistaken as an object name,
but that is not the primary point.

As I already explained, I don't know what information loss you're
talking about.  And yes, I noticed advice.object_name_warning.

So I am fairly strongly negative on both changes.

Okay, but please explain it for me.

On Sat, Jun 15, 2013 at 1:38 PM, Ramkumar Ramachandra
[off-list ref] wrote:

In two places, get_sha1_basic() assumes that strings are possibly sha1
hexes if they are 40 characters long, and calls get_sha1_hex() in these
two cases.  This 40-character check is ugly and wrong: there is nothing
preventing a revision or branch name from being exactly 40 characters.
Replace it with a call to the more robust get_short_sha1().

I share your disdain for the bare '40's in the code.  But I think this
code is less clear than the previous version with the magic number.

Use len instead of strlen(str) here.  It's faster and more correct.

But also get_short_sha1 is much heavier than get_sha1_hex and does not
seem appropriate here.

The semantic meaning of 'detached' seems less clear now if you have to
compare against an enumerated constant to determine the result.  But
also, I do not see why you have to test '!= SHORT_NAME_NOT_FOUND' here
but you did not have to in the other instance.

I think it would be improved if you did this comparison in the
assignment of detached so 'detached' could keep its original boolean
meaning.

But anyway, having looked inside get_short_sha1, it really does seem
to do much more than you want here.

                                return 0;
                }
        }
--
1.8.3.1.438.g96d34e8

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Phil Hord wrote:

I share your disdain for the bare '40's in the code.  But I think this
code is less clear than the previous version with the magic number.

Please read the cover-letter: I was just toying around to see if this
was a good idea, and Junio points out that it's not.