Re: [PATCH 2/2] Add transport message for up-to-date references

Christopher Lindee [off-list ref] writes:

Subject: Re: [PATCH 2/2] Add transport message for up-to-date references

Same comment as [1/2].  Perhaps

    push: mark forced no-op pushes differently in the report

or something?

This code is in an if/else cascade of this shape:

	if we are deleting
		print [deleted]
	else if they did not have any
		print [new]
	else {
		if we forced
		then
			prepare to say forced
		else
			prepare to say an unforced update
		say "updated" in a certain way
	}

The above addition looks somewhat out of place.  I would understand
if the addition happened like so instead:

diff --git i/transport.c w/transport.c
index df518ead70..bacef94b33 100644
--- i/transport.c
+++ w/transport.c

@@ -666,6 +666,8 @@ static void print_ok_ref_status(struct ref *ref,
 			strbuf_addstr(&quickref, "...");
 			type = '+';
 			msg = "forced update";
+		} else if (oideq(old_oid, new_oid)) {
+			... prepare to say "same but forced no-op" ...
 		} else {
 			strbuf_addstr(&quickref, "..");
 			type = ' ';

to make the new logic flow more like so:

		if we forced
		then
			prepare to say forced
		else if we were forced to tell no-op push
			prepare to say we did no-op
		else
			prepare to say an unforced update
		say "updated" in a certain way

Junio C Hamano writes:

Christopher Lindee [off-list ref] writes:

quoted

Subject: Re: [PATCH 2/2] Add transport message for up-to-date references

Same comment as [1/2].  Perhaps

    push: mark forced no-op pushes differently in the report

or something?

Thanks!  I will correct this.

The shoehorned `if` was to avoid duplicating the `strbuf_addstr` and
`type` statements.  It sounds like code duplication is not a concern,
so I can make that change.  However, with this new logic flow, maybe
it would be better to have wholly unique values for the display, for
example:

   deadbeef...deadbeef  main -> main
  -deadbeef..deadbeef   main -> main
  =deadbeef..deadbeef   main -> main
  =deadbeef             main -> main
   deadbeef^!           main -> main
  ^deadbeef deadbeef    main -> main
   deadbeef==deadbeef   main -> main
   deadbeef..deadbeef   main == main

There's a fair amount of room for creativity here.  Of course, using
revisions is useful, but the existing output contains `+` for forced
updates, which is not valid in a revision, so there is clearly space
for novelty.

We may also want to consider what happens when both --force and this
new option are used at the same time.  When testing, the message was
always "up-to-date", but I realize now that a server might report it
as a forced update - it would be odd, but would it be impossible?  I
don't know enough about the push certificate records discussion from
the cover page to say that we wouldn't force an update on a noop.

Regards,
Chris.

Christopher Lindee writes:

Junio C Hamano writes:

quoted

Christopher Lindee [off-list ref] writes:

quoted

Subject: Re: [PATCH 2/2] Add transport message for up-to-date references

If the `--send-up-to-date` option in the previous commit is used, the
"Everything up-to-date!" message will never appear, even if all of the
refs are up to date.  Moreover, the output `deadbeef..deadbeef` appears
suspicious, almost as if a collision occurred.  To clarify that the hash
is, in fact, identical & to allow grepping for the phrase "up-to-date",
add a message to the output when the ref is transmitted, but no change
occurred.

Signed-off-by: Christopher Lindee <redacted>
---
 transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/transport.c b/transport.c
index 84deadd2b6..89802452ea 100644
--- a/transport.c
+++ b/transport.c

@@ -670,6 +670,8 @@ static void print_ok_ref_status(struct ref *ref,
                      strbuf_addstr(&quickref, "..");
                      type = ' ';
                      msg = NULL;
+                     if (oideq(old_oid, new_oid))
+                             msg = "up-to-date";
              }
              strbuf_add_unique_abbrev(&quickref, new_oid,
                                       DEFAULT_ABBREV);

This code is in an if/else cascade of this shape:

        if we are deleting
                print [deleted]
        else if they did not have any
                print [new]
        else {
                if we forced
                then
                        prepare to say forced
                else
                        prepare to say an unforced update
                say "updated" in a certain way
        }

The above addition looks somewhat out of place.  I would understand
if the addition happened like so instead:

diff --git i/transport.c w/transport.c
index df518ead70..bacef94b33 100644
--- i/transport.c
+++ w/transport.c

@@ -666,6 +666,8 @@ static void print_ok_ref_status(struct ref *ref,
                        strbuf_addstr(&quickref, "...");
                        type = '+';
                        msg = "forced update";
+               } else if (oideq(old_oid, new_oid)) {
+                       ... prepare to say "same but forced no-op" ...
                } else {
                        strbuf_addstr(&quickref, "..");
                        type = ' ';

to make the new logic flow more like so:

                if we forced
                then
                        prepare to say forced
                else if we were forced to tell no-op push
                        prepare to say we did no-op
                else
                        prepare to say an unforced update
                say "updated" in a certain way

The shoehorned `if` was to avoid duplicating the `strbuf_addstr` and
`type` statements.  It sounds like code duplication is not a concern,
so I can make that change.  However, with this new logic flow, maybe
it would be better to have wholly unique values for the display, for
example:

   deadbeef...deadbeef  main -> main
  -deadbeef..deadbeef   main -> main
  =deadbeef..deadbeef   main -> main
  =deadbeef             main -> main
   deadbeef^!           main -> main
  ^deadbeef deadbeef    main -> main
   deadbeef==deadbeef   main -> main
   deadbeef..deadbeef   main == main

There's a fair amount of room for creativity here.  Of course, using
revisions is useful, but the existing output contains `+` for forced
updates, which is not valid in a revision, so there is clearly space
for novelty.

It seems I just needed to look one function below to find precedent:

  static int print_one_push_report(struct ref *ref, const char *dest, int count,
                                   struct ref_push_report *report,
                                   int porcelain, int summary_width)
  {       
          ...
          case REF_STATUS_UPTODATE:
                  print_ref_status('=', "[up to date]", ref,
                                   ref->peer_ref, NULL,
                                   report, porcelain, summary_width);
                  break;

Is this a precedent we should follow, or is print_one_push_report()
special in some way?

Thanks,
Chris.