threads / patch / 34151
Subject: [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
Ramkumar Ramachandra[PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks
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
Phil HordRe: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks
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
Ramkumar RamachandraRe: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks
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.
Junio C HamanoRe: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks
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.
Ramkumar Ramachandra[PATCH 2/2] checkout: do not write full sha1 to reflog
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
Ramkumar RamachandraRe: [PATCH 0/2] Slightly prettier reflog message from checkout
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.
Junio C HamanoRe: [PATCH 0/2] Slightly prettier reflog message from checkout
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.
Ramkumar RamachandraRe: [PATCH 0/2] Slightly prettier reflog message from checkout
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.
Junio C HamanoRe: [PATCH 0/2] Slightly prettier reflog message from checkout
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.