Re: [PATCH 2/2] Add transport message for up-to-date references
- From
- Christopher Lindee <christopher.lindee@webpros.com>
- Date
- Mar 15, 2024, 06:09 UTC
- Message-ID
- <SA1PR14MB46913A0B1CB3F3D39ADA6A5F8D282@SA1PR14MB4691.namprd14.prod.outlook.com>
- In-Reply-To
- <xmqqttl94qxg.fsf@gitster.g>
Junio C Hamano writes:
Show 9 quoted lines
> Christopher Lindee <christopher.lindee@webpros.com> 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?
Thanks! I will correct this.
Show 70 quoted lines
> > 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 <christopher.lindee@webpros.com>
> > ---
> > 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 wayThe 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.