* [PATCH 0/2] Slightly prettier reflog message from checkout @ 2013-06-15 17:38 Ramkumar Ramachandra 2013-06-15 17:38 ` [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks Ramkumar Ramachandra ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Ramkumar Ramachandra @ 2013-06-15 17:38 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano [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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks 2013-06-15 17:38 [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra @ 2013-06-15 17:38 ` Ramkumar Ramachandra 2013-06-16 13:28 ` Phil Hord 2013-06-15 17:38 ` [PATCH 2/2] checkout: do not write full sha1 to reflog Ramkumar Ramachandra ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Ramkumar Ramachandra @ 2013-06-15 17:38 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks 2013-06-15 17:38 ` [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks Ramkumar Ramachandra @ 2013-06-16 13:28 ` Phil Hord 2013-06-18 7:03 ` Ramkumar Ramachandra 0 siblings, 1 reply; 10+ messages in thread From: Phil Hord @ 2013-06-16 13:28 UTC (permalink / raw) To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks 2013-06-16 13:28 ` Phil Hord @ 2013-06-18 7:03 ` Ramkumar Ramachandra 2013-06-18 14:27 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Ramkumar Ramachandra @ 2013-06-18 7:03 UTC (permalink / raw) To: Phil Hord; +Cc: Git List, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks 2013-06-18 7:03 ` Ramkumar Ramachandra @ 2013-06-18 14:27 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2013-06-18 14:27 UTC (permalink / raw) To: Ramkumar Ramachandra; +Cc: Phil Hord, Git List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] checkout: do not write full sha1 to reflog 2013-06-15 17:38 [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra 2013-06-15 17:38 ` [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks Ramkumar Ramachandra @ 2013-06-15 17:38 ` Ramkumar Ramachandra 2013-06-15 17:42 ` [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra 2013-06-16 1:24 ` Junio C Hamano 3 siblings, 0 replies; 10+ messages in thread From: Ramkumar Ramachandra @ 2013-06-15 17:38 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Slightly prettier reflog message from checkout 2013-06-15 17:38 [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra 2013-06-15 17:38 ` [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks Ramkumar Ramachandra 2013-06-15 17:38 ` [PATCH 2/2] checkout: do not write full sha1 to reflog Ramkumar Ramachandra @ 2013-06-15 17:42 ` Ramkumar Ramachandra 2013-06-16 1:24 ` Junio C Hamano 3 siblings, 0 replies; 10+ messages in thread From: Ramkumar Ramachandra @ 2013-06-15 17:42 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Slightly prettier reflog message from checkout 2013-06-15 17:38 [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra ` (2 preceding siblings ...) 2013-06-15 17:42 ` [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra @ 2013-06-16 1:24 ` Junio C Hamano 2013-06-16 9:21 ` Ramkumar Ramachandra 3 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2013-06-16 1:24 UTC (permalink / raw) To: Ramkumar Ramachandra; +Cc: Git List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Slightly prettier reflog message from checkout 2013-06-16 1:24 ` Junio C Hamano @ 2013-06-16 9:21 ` Ramkumar Ramachandra 2013-06-17 2:59 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Ramkumar Ramachandra @ 2013-06-16 9:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Slightly prettier reflog message from checkout 2013-06-16 9:21 ` Ramkumar Ramachandra @ 2013-06-17 2:59 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2013-06-17 2:59 UTC (permalink / raw) To: Ramkumar Ramachandra; +Cc: Git List 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-06-18 14:28 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-15 17:38 [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra 2013-06-15 17:38 ` [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks Ramkumar Ramachandra 2013-06-16 13:28 ` Phil Hord 2013-06-18 7:03 ` Ramkumar Ramachandra 2013-06-18 14:27 ` Junio C Hamano 2013-06-15 17:38 ` [PATCH 2/2] checkout: do not write full sha1 to reflog Ramkumar Ramachandra 2013-06-15 17:42 ` [PATCH 0/2] Slightly prettier reflog message from checkout Ramkumar Ramachandra 2013-06-16 1:24 ` Junio C Hamano 2013-06-16 9:21 ` Ramkumar Ramachandra 2013-06-17 2:59 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).