All of lore.kernel.org
 help / color / mirror / Atom feed
From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Peter Kaestle <peter.kaestle@nokia.com>,
	Git mailing list <git@vger.kernel.org>,
	Stefan Beller <stefanbeller@gmail.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [RFC 1/2] submodules: test for fetch of non-init subsub-repo
Date: Wed, 11 Nov 2020 12:35:53 -0500	[thread overview]
Message-ID: <37DD13D4-FBE4-4DB7-85F5-824E850BA9AE@gmail.com> (raw)
In-Reply-To: <xmqqk0uuct94.fsf@gitster.c.googlers.com>

Hi Junio,

> Le 9 nov. 2020 à 12:52, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> Peter Kaestle <peter.kaestle@nokia.com> writes:
> 
>> This test case triggers a regression, which was introduced by
>> a62387b3fc9f5aeeb04a2db278121d33a9caafa7 in following setup:
> 
> Also, it probably is a better arrangement, after explaining how the
> current system does not work in the log message, to have the code
> fix in the same patch and add test to ensure the bug will stay
> fixed, in a single patch.  That way, you do not have to start with
> expect_failure and then flip the polarity to expect_success, which
> is a horrible style for reviewers to understand the code fix because
> the second "fix" step does not actually show the effect of what got
> fixed in the patch (the test change shows the flip of the polarity
> of the test plus only a few context lines and does not show what
> behaviour change the "fix" causes).

I had learned by browsing the list that this was the preferred way to
submit patches for bug fixes and regressions for this project, but I had
not yet read a good justification as to *why* it was preferred. 

Thanks for spelling it out; I think a quick paragraph about this somewhere
in SubmittingPatches would be a good addition for new contributors.

Cheers,

Philippe.


  parent reply	other threads:[~2020-11-11 17:35 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-03 14:23 [REGRESSION FIX 0/2] Handling regression introduced by a62387b Peter Kaestle
2020-11-03 14:23 ` [REGRESSION FIX 1/2] submodules: test for fetch of non-init subsub-repo Peter Kaestle
2020-11-03 14:23 ` [REGRESSION FIX 2/2] Revert "submodule.c: fetch in submodules git directory instead of in worktree" Peter Kaestle
2020-11-09  8:33 ` [RFC 0/2] Handling regression introduced by a62387b Peter Kaestle
2020-11-09  8:33   ` [RFC 1/2] submodules: test for fetch of non-init subsub-repo Peter Kaestle
2020-11-09 17:52     ` Junio C Hamano
2020-11-11 12:45       ` Peter Kästle
2020-11-11 17:22         ` Philip Oakley
2020-11-12 16:00           ` [RFCv2] submodules: fix of regression on fetching " Peter Kaestle
2020-11-11 17:35       ` Philippe Blain [this message]
2020-11-11 19:27         ` [RFC 1/2] submodules: test for fetch " Junio C Hamano
2020-11-11 19:24     ` Philippe Blain
2020-11-09  8:33   ` [RFC 2/2] Revert "submodule.c: fetch in submodules git directory instead of in worktree" Peter Kaestle
2020-11-10 15:08     ` Johannes Schindelin

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=37DD13D4-FBE4-4DB7-85F5-824E850BA9AE@gmail.com \
    --to=levraiphilippeblain@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=peter.kaestle@nokia.com \
    --cc=stefanbeller@gmail.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.