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

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

[Ramkumar Ramachandra]

[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

[PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks

[Ramkumar Ramachandra]

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 <artagnon@gmail.com>
---
 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

Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks

[Phil Hord]

On Sat, Jun 15, 2013 at 1:38 PM, Ramkumar Ramachandra
<artagnon@gmail.com> 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.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  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)) {

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.

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

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

Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks

[Ramkumar Ramachandra]

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.

Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Which was...

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

...and I do not find how the above is connected with

> I was just toying around to see if this
> was a good idea,...

...this in any way.  Puzzled.

[PATCH 2/2] checkout: do not write full sha1 to reflog

[Ramkumar Ramachandra]

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 <artagnon@gmail.com>
---
 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

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

[Ramkumar Ramachandra]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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.

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

[Ramkumar Ramachandra]

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.

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

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

That is not the point.  From the proposed log message for your 2/2:

  f855138: checkout: moving from bdff0e3a374617dce784f801b97500d9ba2e4705 to co-reflog
  f855138: checkout: moving from bdff0e3 to co-reflog

The "bdff0e3" may be a unique abbreviation to identify the commit
bdff0e3a374617dce784f801b97500d9ba2e4705 when the reflog entry was
written.  But the latter can become meaningless once you have an
unrelated commit in your history that shares the prefix.

That is the "deliberate loss of information" I saw in the proposal.
Recording full 40-hex does not have to worry about that issue; you
do not even have to argue "but in this case we do not even have to
have unique SHA-1, nobody uses it" vs. "some other codepaths you are
not aware of may want to take advantage and start using it".  In
other words, we will have one less thing we have to worry about.