Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Philip Oakley]

From: "Junio C Hamano" 
Sent: Wednesday, November 08, 2017 1:59 AM



"Philip Oakley"  writes:


But...

...
This change causes quite a few tests to fall over; however, they
all have truncated-something-longer-ellipses in their
raw-diff-output expected sections, and removing the ellipses
from there makes the tests pass again, :-)


The number of failures you report in the test suit suggests that
someone somewhere will be expecting that notation, and that we may
need a deprecation period, perhaps with an 'ellipsis' config variable
whose default value can later be flipped, though that leaves a config
value needing support forever!


Hmmm, never thought about that.

I have been assuming that tools reading "--raw" output that is
abbreviated would be crazy, because they have to strip the dots and
the number of dots may not always be three [*1*].

But you are right.  It would be very unlikely that there is no such
crazy tools, so it deserves consideration if we would be breaking
such tools.

On the other hand, if such a crazy tool was still written correctly
(it is debatable what the definition of "correct" is, though), it
would be stripping any number dots at the end, not just insisting on
seeing exactly three dots, and splitting these fields at SP.
Otherwise they would already be broken as they cannot handle
occasional object names that have less than three dots because they
happen to be longer than the more common abbreviation length used by
other objects.  So in practice it might not be _too_ bad.

Thinking on this, I'd suggest that the patch series does remove the ellipsis 
dots immediately, but retains a config option that can be set to get back 
the old 'dots' display for those who have badly written scripts that maybe 
haven't failed yet. i.e. no deprecation period, just a fall back option; and 
if nobody shouts then remove the config option after a respectable period.


It would also mean the existing tests can be re-used...



[Footnote]

*1* When we ask for --abbrev=7, we allocate 10 places and fill the
rest with necessary number of dots after the result of
find_unique_abbrev(), so if an object name turns out to require 8
hexdigits to make it unique, we'll append only two dots to it to
make it 10 so that it aligns nicely with others) and they would
always be reading the full, non abbreviated output.  The story does
not change that much when we do not explicitly ask for a specific
abbreviation length in that we add variable number of dots for
aligning in that case, too.


The --abbrev=7 does cater for many smaller repo's, so there is a possiblity 
that the bad script issue hasn't been hit yet by those repos.

--

Philip 

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Junio C Hamano]

"Philip Oakley"  writes:

> But...
>> ...
>> This change causes quite a few tests to fall over; however, they
>> all have truncated-something-longer-ellipses in their
>> raw-diff-output expected sections, and removing the ellipses
>> from there makes the tests pass again, :-)
>
> The number of failures you report in the test suit suggests that
> someone somewhere will be expecting that notation, and that we may
> need a deprecation period, perhaps with an 'ellipsis' config variable
> whose default value can later be flipped, though that leaves a config
> value needing support forever!

Hmmm, never thought about that. 

I have been assuming that tools reading "--raw" output that is
abbreviated would be crazy, because they have to strip the dots and
the number of dots may not always be three [*1*].

But you are right.  It would be very unlikely that there is no such
crazy tools, so it deserves consideration if we would be breaking
such tools.

On the other hand, if such a crazy tool was still written correctly
(it is debatable what the definition of "correct" is, though), it
would be stripping any number dots at the end, not just insisting on
seeing exactly three dots, and splitting these fields at SP.
Otherwise they would already be broken as they cannot handle
occasional object names that have less than three dots because they
happen to be longer than the more common abbreviation length used by
other objects.  So in practice it might not be _too_ bad.


[Footnote]

*1* When we ask for --abbrev=7, we allocate 10 places and fill the
rest with necessary number of dots after the result of
find_unique_abbrev(), so if an object name turns out to require 8
hexdigits to make it unique, we'll append only two dots to it to
make it 10 so that it aligns nicely with others) and they would
always be reading the full, non abbreviated output.  The story does
not change that much when we do not explicitly ask for a specific
abbreviation length in that we add variable number of dots for
aligning in that case, too.

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Philip Oakley]

From: "Ann T Ropea" 

Thanks for all the feedback provided!

I'd like to summarise what consensus we have reached so far and
then propose a way forward:

  * we'll use the term "ellipsis (pl. ellipses)" for what's
been referred to as "3dots", "n-dots", "many dots" and so
forth


Using a consistent  term for the *display* of shortened oid's is good.



  * we would like to use ellipses when attached to SHA-1
values only for the purpose of specifying a symmetric
difference (as per gitrevisions(7))


The symetric difference (three-dots) is a specific Git *cli* notation that 
is distinct from the use of ellipsis for displaying oid's




  * the usage of ellipses as a "here we truncated something
longer" is a relic which should be phased out.


I think that is true.



To get there, preventing describe_detached_head from appending
an ellipsis to the SHA-1 values it prints is one important step.

This change does not cause any test to fall over.


But...


The other important step is dealing with the "git diff --raw"
output which features ellipses in the relic-fashion no longer
desired.

It would appear that simplifying diff.c's diff_aligned_abbrev
routine to something like:

/* Do we want all 40 hex characters?
*/
if (len == GIT_SHA1_HEXSZ)
return oid_to_hex(oid);

/* An abbreviated value is fine.
*/
return diff_abbrev_oid(oid, len);

does do the trick.

This change causes quite a few tests to fall over; however, they
all have truncated-something-longer-ellipses in their
raw-diff-output expected sections, and removing the ellipses
from there makes the tests pass again, :-)


The number of failures you report in the test suit suggests that someone 
somewhere will be expecting that notation, and that we may need a 
deprecation period, perhaps with an 'ellipsis' config variable whose default 
value can later be flipped, though that leaves a config value needing 
support forever!


Junio should be able to better advise on his preferred approach.



If we can agree that this is a way forward, i'll create & send
v2 of the patch series to the mailing list (it'll include the
fixed tests) and we'll see where we go from there.


--
Philip 

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Ann T Ropea]

Thanks for all the feedback provided!

I'd like to summarise what consensus we have reached so far and
then propose a way forward:

   * we'll use the term "ellipsis (pl. ellipses)" for what's
 been referred to as "3dots", "n-dots", "many dots" and so
 forth

   * we would like to use ellipses when attached to SHA-1
 values only for the purpose of specifying a symmetric
 difference (as per gitrevisions(7))

   * the usage of ellipses as a "here we truncated something
 longer" is a relic which should be phased out

To get there, preventing describe_detached_head from appending
an ellipsis to the SHA-1 values it prints is one important step.

This change does not cause any test to fall over.

The other important step is dealing with the "git diff --raw"
output which features ellipses in the relic-fashion no longer
desired.

It would appear that simplifying diff.c's diff_aligned_abbrev
routine to something like:

/* Do we want all 40 hex characters?
 */
if (len == GIT_SHA1_HEXSZ)
return oid_to_hex(oid);

/* An abbreviated value is fine.
 */
return diff_abbrev_oid(oid, len);

does do the trick.

This change causes quite a few tests to fall over; however, they
all have truncated-something-longer-ellipses in their
raw-diff-output expected sections, and removing the ellipses
from there makes the tests pass again, :-)

If we can agree that this is a way forward, i'll create & send
v2 of the patch series to the mailing list (it'll include the
fixed tests) and we'll see where we go from there.

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Junio C Hamano]

Ann T Ropea  writes:

> Lest we confuse the meticulous observer, we ought to retire the 3dots in
> the circumstances described above.

Yes, as I said in my response to 3/3, I think it is a good goal to
avoid n-dots used as a "here we truncated something longer" sign,
which was a very old convention that was invented without knowing
that we'd later come up with a syntax that would conflict with it.

For this particular output, I wonder if it is even better to follow
our own advice, though.  Documentation/SubmittingPatches says:

If you want to reference a previous commit in the history of a stable
branch, use the format "abbreviated sha1 (subject, date)",
with the subject enclosed in a pair of double-quotes, like this:

Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
noticed that ...

I dunno.

> Signed-off-by: Ann T Ropea 
> ---
>  builtin/checkout.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index fc4f8fd2ea29..59cc52e55855 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -404,7 +404,7 @@ static void describe_detached_head(const char *msg, 
> struct commit *commit)
>   struct strbuf sb = STRBUF_INIT;
>   if (!parse_commit(commit))
>   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
> - fprintf(stderr, "%s %s... %s\n", msg,
> + fprintf(stderr, "%s %s %s\n", msg,
>   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), 
> sb.buf);
>   strbuf_release();
>  }

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Philip Oakley]

From: "Junio C Hamano" 

Ann T Ropea  writes:


This could be confusing not only for novices; in either case, no range
should be insinuated by describe_detached_head.


We actually do not insinuate any range in these output.  These dots
denote "truncated at the end, instead of giving full length."

Another place these "many dots" appear is "git diff --raw", for
example.



 The fancy word for the three dots is an `ellipsis`
- the omission from speech or writing of a word or words that are 
superfluous or able to be understood from contextual clues.
- from the Ancient Greek: ἔλλειψις, élleipsis, "omission" or "falling 
short".


The user/reader confusion may still be there though.
 

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Junio C Hamano]

Ann T Ropea  writes:

> This could be confusing not only for novices; in either case, no range
> should be insinuated by describe_detached_head.

We actually do not insinuate any range in these output.  These dots
denote "truncated at the end, instead of giving full length."

Another place these "many dots" appear is "git diff --raw", for
example.

[PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Ann T Ropea]

When a committish, C, is immediately followed by 3dots (...) which are,
in turn, not followed by another committish, we are usually asking for
the revision range C...HEAD, which is known as the symmetric difference
of C and HEAD.

When describe_detached_head is invoked, it prints the committish
followed by 3dots as elaborated above when it indicates the current and
previous HEAD positions.  For example:

   # Randomly check out one of the first seven git.git commits
   # (Starting with a detached HEAD already)
   $ git checkout $(git rev-list master | tail -7 | shuf | head -1)
   Previous HEAD position was bfcf2d7874f7... checkout: describe_detached_head: 
remove 3dots after committish
   HEAD is now at 19b2860cba57... Use "-Wall -O2" for the compiler to get more 
warnings.

"Evaluating" this displayed pseudo-range for the current HEAD indication
resolves to the empty range (C...HEAD, where C equals HEAD).

For the previous HEAD indication, the results of the "evaluation" are
somewhat more difficult to predict: previous here refers to what the
reflog dictates (this is not necessarily the topological ancestor in the
DAG, i.e., HEAD^).  In the example above, the "range" resolves to almost
all commits in the author's clone of git.git.  Running the command again
causes the then previous to current HEAD position "range" to be a lot
smaller.

This could be confusing not only for novices; in either case, no range
should be insinuated by describe_detached_head.

Granted, this "evaluation" is at the moment, if at all, only performed
in the mind of the observer.  And, to be sure, the 3dots *are* intended
as a continuation indication for the abbreviated SHA-1 value.
Nevertheless, we should get rid of them, for the following reasons:

   * they would still be displayed if someone had their core.abbrev
 config value set to the max

   * when the built-in version of checkout was introduced by commit

782c2d65c240 ("Build in checkout", 2008-02-07)

 no 3dots were present in the legacy git-checkout.sh (see
 contrib/examples/git-checkout.sh)

   * when git-reset causes a new HEAD line to be printed (during a hard
 reset), neither builtin/reset.c nor contrib/examples/git-reset.sh
 mention 3dots

Lest we confuse the meticulous observer, we ought to retire the 3dots in
the circumstances described above.

Signed-off-by: Ann T Ropea 
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd2ea29..59cc52e55855 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -404,7 +404,7 @@ static void describe_detached_head(const char *msg, struct 
commit *commit)
struct strbuf sb = STRBUF_INIT;
if (!parse_commit(commit))
pp_commit_easy(CMIT_FMT_ONELINE, commit, );
-   fprintf(stderr, "%s %s... %s\n", msg,
+   fprintf(stderr, "%s %s %s\n", msg,
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), 
sb.buf);
strbuf_release();
 }
-- 
2.13.6