git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Rubén Justo" <rjusto@gmail.com>, "Git List" <git@vger.kernel.org>
Subject: Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed
Date: Sat, 08 Oct 2022 10:46:54 -0700	[thread overview]
Message-ID: <xmqq1qrib435.fsf@gitster.g> (raw)
In-Reply-To: <CAPig+cRxy5C+CqUOzmhe16j+hssxsygha3huVga8tLJ+imM4Hw@mail.gmail.com> (Eric Sunshine's message of "Sat, 8 Oct 2022 13:10:59 -0400")

Eric Sunshine <sunshine@sunshineco.com> writes:

>> Yeah, I thought about that.  What convinced me to use "git stripspace" was
>> that maybe that '\n' tail could be removed sometime from the description
>> setting and this will be fine with that.  I haven't found any reason for
>> that '\n' and it bugs me a little seeing it in the config :-)
>
> That reasoning occurred to me, as well, and I'd have no objection to
> git-stripspace if that's the motivation for using it. I don't feel
> strongly one way or the other, and my previous email was intended
> primarily to point out the lightweight alternatives in case you hadn't
> considered them. Feel free to use git-stripspace if you feel it is the
> more appropriate choice.

I do not think I would agree with the line of reasoning.

It all depends on why we anticipate that the terminating LF may go
away someday, but if it is because we may do so by mistake and
without a good reason when making some unrelated changes to the
implementation of "git branch --edit-desc", we would want to know
about it, and such a loose check that would miss it is definitely
unwelcome.  It is very likely that not just "git merge" but people's
external scripts depend on the presence of final LF especially when
the description has only one line, so unless we are doing
deliberately so, we should prepare to catch such a change.

If it is because we may gain a consensus that the description string
(which by the way can well consist of multiple lines) is better
without the LF on its final line, and we are "fixing" the code to do
so very much on purpose, it would be good to have a test to protect
such a change from future unintended breakages.  Adding a loose test
that won't break across such a change today may be OK, but whoever
is making such a change in the future has to make sure there is a
test that is not loose to protect the change.  And it would very
likely to be done by adding a new test, instead of noticing that
this loosely written test can be tightened to serve the purpose.

So if we start with a tight test that expects the exact number of
LFs at the end, we would be better off in that case, too.

  reply	other threads:[~2022-10-08 17:47 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-05 14:34 [PATCH 0/2] branch: support for at-refs like @{-1} in --edit-description, --set-upstream-to and --unset-upstream Rubén Justo via GitGitGadget
2022-09-05 14:34 ` [PATCH 1/2] branch: refactor edit_description command switch case Rubén Justo via GitGitGadget
2022-09-05 14:34 ` [PATCH 2/2] branch: support for at-refs like @{-1} Rubén Justo via GitGitGadget
2022-09-07  9:45 ` [PATCH v2 0/2] branch: support for shortcuts like @{-1}, completed Rubén Justo
2022-09-07  9:52   ` [PATCH v2 1/2] branch: refactor "edit_description" code path Rubén Justo
2022-09-07 20:25     ` Junio C Hamano
2022-09-07 21:24       ` Rubén Justo
2022-09-08  4:32         ` Rubén Justo
2022-09-07  9:53   ` [PATCH v2 2/2] branch: support for shortcuts like @{-1} completed Rubén Justo
2022-09-08  4:47   ` [PATCH v3 0/2] branch: support for shortcuts like @{-1}, completed Rubén Justo
2022-09-08  4:51     ` [PATCH v3 1/2] branch: refactor "edit_description" code path Rubén Justo
2022-09-08 20:57       ` [PATCH] branch: error codes for "edit_description" Rubén Justo
2022-09-08 21:45         ` Junio C Hamano
2022-09-08  4:53     ` [PATCH v3 2/2] branch: support for shortcuts like @{-1} completed Rubén Justo
2022-10-08  1:00     ` [PATCH v4] branch: support for shortcuts like @{-1}, completed Rubén Justo
2022-10-08  3:17       ` Eric Sunshine
2022-10-08  4:23         ` Junio C Hamano
2022-10-08  7:07         ` Rubén Justo
2022-10-08  7:23           ` Eric Sunshine
2022-10-08  9:12             ` Rubén Justo
2022-10-08 17:10               ` Eric Sunshine
2022-10-08 17:46                 ` Junio C Hamano [this message]
2022-10-08 20:48                   ` Rubén Justo
2022-10-08 23:28                 ` Rubén Justo
2022-10-09  6:46                   ` Eric Sunshine
2022-10-09 19:33                     ` Junio C Hamano
2022-10-09 22:27                       ` Rubén Justo
2022-10-08  4:17       ` Junio C Hamano
2022-10-08  9:04         ` Rubén Justo
2022-10-08 22:32       ` [PATCH v5] " Rubén Justo
2022-10-09  5:37         ` Junio C Hamano
2022-10-09 19:11         ` Junio C Hamano
2022-10-09 21:26           ` Rubén Justo
2022-10-10  0:38             ` Junio C Hamano
2022-10-10  6:05               ` Rubén Justo
2022-10-10 16:55                 ` Junio C Hamano
2022-10-10 18:08                   ` Rubén Justo
2022-10-10 23:24         ` [PATCH v6] " Rubén Justo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq1qrib435.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=rjusto@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).