[PATCH] transport, send-pack: append period to up-to-date message

[PATCH] transport, send-pack: append period to up-to-date message

[Yong Bakos]

Appending a period to "Everything up-to-date" makes the output message
consistent with similar output in builtin/merge.c.

Signed-off-by: Yong Bakos <ybakos@humanoriented.com>
---
 builtin/send-pack.c | 2 +-
 transport.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 1ff5a67..67d9304 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -294,7 +294,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	}

 	if (!ret && !transport_refs_pushed(remote_refs))
-		fprintf(stderr, "Everything up-to-date\n");
+		fprintf(stderr, "Everything up-to-date.\n");

 	return ret;
 }
diff --git a/transport.c b/transport.c
index 095e61f..53d5405 100644
--- a/transport.c
+++ b/transport.c
@@ -942,7 +942,7 @@ int transport_push(struct transport *transport,
 		if (porcelain && !push_ret)
 			puts("Done");
 		else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
-			fprintf(stderr, "Everything up-to-date\n");
+			fprintf(stderr, "Everything up-to-date.\n");

 		return ret;
 	}
--
2.7.2

Re: [PATCH] transport, send-pack: append period to up-to-date message

[Stefan Beller]

On Tue, May 24, 2016 at 1:51 PM, Yong Bakos <junk@humanoriented.com> wrote:
> Appending a period to "Everything up-to-date" makes the output message
> consistent with similar output in builtin/merge.c.
>
> Signed-off-by: Yong Bakos <ybakos@humanoriented.com>
> ---
>  builtin/send-pack.c | 2 +-
>  transport.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index 1ff5a67..67d9304 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c

While consistency is a good idea in general, I wonder how that applies here.
git-send-pack is a low level (i.e. plumbing) command.

       The interface (input, output, set of options and the semantics) to
       these low-level commands are meant to be a lot more stable than
       Porcelain level commands, because these commands are primarily for
       scripted use. The interface to Porcelain commands on the other hand are
       subject to change in order to improve the end user experience.

So if another porcelain exists and compares the output string
exactly, this would be a regression for them. That is why I'd refrain
from updating these strings

However these two strings are only showing up in these 2 places, so
maybe instead
we'd want to see a test documenting existing behavior?

Thanks,
Stefan

Re: [PATCH] transport, send-pack: append period to up-to-date message

[Jeff King]

On Tue, May 24, 2016 at 02:21:00PM -0700, Stefan Beller wrote:

> On Tue, May 24, 2016 at 1:51 PM, Yong Bakos <junk@humanoriented.com> wrote:
> > Appending a period to "Everything up-to-date" makes the output message
> > consistent with similar output in builtin/merge.c.
> >
> > Signed-off-by: Yong Bakos <ybakos@humanoriented.com>
> > ---
> >  builtin/send-pack.c | 2 +-
> >  transport.c         | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 1ff5a67..67d9304 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> 
> While consistency is a good idea in general, I wonder how that applies here.
> git-send-pack is a low level (i.e. plumbing) command.
> 
>        The interface (input, output, set of options and the semantics) to
>        these low-level commands are meant to be a lot more stable than
>        Porcelain level commands, because these commands are primarily for
>        scripted use. The interface to Porcelain commands on the other hand are
>        subject to change in order to improve the end user experience.
> 
> So if another porcelain exists and compares the output string
> exactly, this would be a regression for them. That is why I'd refrain
> from updating these strings

I think messages to stderr are generally fair game for changing, even in
plumbing. In many cases they are also translated (and I would argue that
these messages probably should be translated, too).

That being said, CodingGuidelines says:

   - Do not end error messages with a full stop.

This isn't an error message exactly, but I think it's in the same vein.
I will note that we have not historically been consistent here, though
(as evidenced by the noted message in git-merge).

-Peff

Re: [PATCH] transport, send-pack: append period to up-to-date message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think messages to stderr are generally fair game for changing, even in
> plumbing. In many cases they are also translated (and I would argue that
> these messages probably should be translated, too).

I think I agree.  My first reaction to this thread indeed was "Why
isn't this marked for translation?"; as to the change proposed by
the patch itself, my reaction actually was "Meh" ;-)

> That being said, CodingGuidelines says:
>
>    - Do not end error messages with a full stop.

Thanks for pointing it out---I forgot about that one.

I do not think there was a concrete reason why they should not end
with a full stop, other than "be consistent with existing ones that
do not end with a full stop", though.

> This isn't an error message exactly, but I think it's in the same vein.
> I will note that we have not historically been consistent here, though
> (as evidenced by the noted message in git-merge).

Re: [PATCH] transport, send-pack: append period to up-to-date message

[Yong Bakos]

On May 25, 2016, at 5:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeff King <peff@peff.net> writes:
> 
>> I think messages to stderr are generally fair game for changing, even in
>> plumbing. In many cases they are also translated (and I would argue that
>> these messages probably should be translated, too).
> 
> I think I agree.  My first reaction to this thread indeed was "Why
> isn't this marked for translation?"; as to the change proposed by
> the patch itself, my reaction actually was "Meh" ;-)

Aw come one, Junio, you mean one "." doesn't excite you? :)

> 
>> That being said, CodingGuidelines says:
>> 
>>   - Do not end error messages with a full stop.
> 
> Thanks for pointing it out---I forgot about that one.
> 
> I do not think there was a concrete reason why they should not end
> with a full stop, other than "be consistent with existing ones that
> do not end with a full stop", though.
> 
>> This isn't an error message exactly, but I think it's in the same vein.
>> I will note that we have not historically been consistent here, though
>> (as evidenced by the noted message in git-merge).

Indeed, most of the output during a pull/merge/push workflow breaks the
"don't end with a full stop" guideline. I believe that this patch,
despite its candidacy for the "most insignificant patch award," is a
sane one.

Thanks for reviewing,
yong