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  wrote:
> 
> Jeff King  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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King  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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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 
> > ---
> >  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html