From: Johannes Sixt <j6t@kdbg.org>
To: Elijah Newren <newren@gmail.com>
Cc: gitster@pobox.com, git@vger.kernel.org, Jim Foucar <jgfouca@sandia.gov>
Subject: Re: [PATCHv2 27/56] string-list: Add API to remove an item from an unsorted list
Date: Sat, 13 Aug 2011 11:08:17 +0200 [thread overview]
Message-ID: <4E463F01.5060301@kdbg.org> (raw)
In-Reply-To: <CABPp-BFsqDhVnxDa9u3tV0yZky_az0MTCT_N5y+Oqm84+fOyvQ@mail.gmail.com>
Am 13.08.2011 00:14, schrieb Elijah Newren:
> On Fri, Aug 12, 2011 at 1:00 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Am 8/12/2011 7:20, schrieb Elijah Newren:
>>> Here's an attempt for a delete_item API (note: only compile-tested).
>>
>> Seriously? You haven't even tested this patch, and still don't mark it
>> with RFC?
> ...
> However, I am unclear what you mean by not even testing the patch,
> though. I couldn't find any unit-test harness or any other kind of
> testsuite for the string_list API. I did review the code to make sure
> it looked right to me, added a use of your new function, and ran the
> standard testsuite in addition to my "re-merge all merges from
> git.git" testcase. I even single stepped through the code in a
> debugger for good measure. What testing did you want to see in
> particular?
This kind of testing is fine. And I appologize for asking a provocative
question. Of course, I know that you did test the new function suitably,
otherwise you wouldn't have submitted the series.
So, the question is rather, why did that sentence remain in the commit
message? The commit message should not be deceptive (and in particular
not blindly copy-pasted from an email that throws out a patch in the
hopes that somebody picks it up and massages into a good shape - I
thought it was clear that my patch was not a proper patch submission).
Think about someone browses history constrained by pathspec
'string-list.c'. This person will see this commit without any hint about
the merge-recursive series or that the new API is used in the next
commit, and will have to ask: "Why the heck is did someone introduce
this code and didn't even test it?"
-- Hannes
next prev parent reply other threads:[~2011-08-13 9:10 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-08-12 5:19 [PATCHv2 00/57] Re-roll of en/merge-recursive from pu Elijah Newren
2011-08-12 5:19 ` [PATCHv2 01/56] t6042: Add a testcase where git deletes an untracked file Elijah Newren
2011-08-12 5:19 ` [PATCHv2 02/56] t6042: Add failing testcase for rename/modify/add-source conflict Elijah Newren
2011-08-12 5:19 ` [PATCHv2 03/56] t6042: Add a pair of cases where undetected renames cause issues Elijah Newren
2011-08-12 5:19 ` [PATCHv2 04/56] t6042: Add a testcase where undetected rename causes silent file deletion Elijah Newren
2011-08-12 5:19 ` [PATCHv2 05/56] t6042: Add tests for content issues with modify/rename/directory conflicts Elijah Newren
2011-08-12 5:19 ` [PATCHv2 06/56] t6042: Ensure rename/rename conflicts leave index and workdir in sane state Elijah Newren
2011-08-12 5:19 ` [PATCHv2 07/56] t6042: Add failing testcases for rename/rename/add-{source,dest} conflicts Elijah Newren
2011-08-12 5:19 ` [PATCHv2 08/56] t6036: Add differently resolved modify/delete conflict in criss-cross test Elijah Newren
2011-08-12 5:19 ` [PATCHv2 09/56] t6036: criss-cross with weird content can fool git into clean merge Elijah Newren
2011-08-12 5:19 ` [PATCHv2 10/56] t6036: tests for criss-cross merges with various directory/file conflicts Elijah Newren
2011-08-12 5:19 ` [PATCHv2 11/56] t6036: criss-cross w/ rename/rename(1to2)/modify+rename/rename(2to1)/modify Elijah Newren
2011-08-12 5:19 ` [PATCHv2 12/56] t6036: criss-cross + rename/rename(1to2)/add-source + modify/modify Elijah Newren
2011-08-12 5:19 ` [PATCHv2 13/56] t6022: Remove unnecessary untracked files to make test cleaner Elijah Newren
2011-08-12 5:19 ` [PATCHv2 14/56] t6022: New tests checking for unnecessary updates of files Elijah Newren
2011-08-12 5:19 ` [PATCHv2 15/56] t6022: Add testcase for merging a renamed file with a simple change Elijah Newren
2011-08-12 5:19 ` [PATCHv2 16/56] merge-recursive: Make BUG message more legible by adding a newline Elijah Newren
2011-08-12 5:19 ` [PATCHv2 17/56] merge-recursive: Correct a comment Elijah Newren
2011-08-12 5:19 ` [PATCHv2 18/56] merge-recursive: Mark some diff_filespec struct arguments const Elijah Newren
2011-08-12 5:19 ` [PATCHv2 19/56] merge-recursive: Consolidate different update_stages functions Elijah Newren
2011-08-12 5:19 ` [PATCHv2 20/56] merge-recursive: Remember to free generated unique path names Elijah Newren
2011-08-12 5:19 ` [PATCHv2 21/56] merge-recursive: Avoid working directory changes during recursive case Elijah Newren
2011-08-12 5:19 ` [PATCHv2 22/56] merge-recursive: Fix recursive case with D/F conflict via add/add conflict Elijah Newren
2011-08-12 5:19 ` [PATCHv2 23/56] merge-recursive: Fix sorting order and directory change assumptions Elijah Newren
2011-08-12 5:19 ` [PATCHv2 24/56] merge-recursive: Fix code checking for D/F conflicts still being present Elijah Newren
2011-08-12 5:19 ` [PATCHv2 25/56] merge-recursive: Save D/F conflict filenames instead of unlinking them Elijah Newren
2011-08-12 5:19 ` [PATCHv2 26/56] merge-recursive: Split was_tracked() out of would_lose_untracked() Elijah Newren
2011-08-12 5:20 ` [PATCHv2 27/56] string-list: Add API to remove an item from an unsorted list Elijah Newren
2011-08-12 7:00 ` Johannes Sixt
2011-08-12 9:27 ` Alex Riesen
2011-08-12 22:14 ` Elijah Newren
2011-08-13 9:08 ` Johannes Sixt [this message]
2011-08-12 5:20 ` [PATCHv2 28/56] merge-recursive: Allow make_room_for_path() to remove D/F entries Elijah Newren
2011-08-12 5:20 ` [PATCHv2 29/56] merge-recursive: Split update_stages_and_entry; only update stages at end Elijah Newren
2011-08-12 5:20 ` [PATCHv2 30/56] merge-recursive: Fix deletion of untracked file in rename/delete conflicts Elijah Newren
2011-08-12 5:20 ` [PATCHv2 31/56] merge-recursive: Make dead code for rename/rename(2to1) conflicts undead Elijah Newren
2011-08-12 5:20 ` [PATCHv2 32/56] merge-recursive: Add comments about handling rename/add-source cases Elijah Newren
2011-08-12 5:20 ` [PATCHv2 33/56] merge-recursive: Improve handling of rename target vs. directory addition Elijah Newren
2011-08-12 5:20 ` [PATCHv2 34/56] merge-recursive: Consolidate process_entry() and process_df_entry() Elijah Newren
2011-08-12 5:20 ` [PATCHv2 35/56] merge-recursive: Cleanup and consolidation of rename_conflict_info Elijah Newren
2011-08-12 5:20 ` [PATCHv2 36/56] merge-recursive: Provide more info in conflict markers with file renames Elijah Newren
2011-08-12 5:20 ` [PATCHv2 37/56] merge-recursive: When we detect we can skip an update, actually skip it Elijah Newren
2011-08-12 5:20 ` [PATCHv2 38/56] merge-recursive: Fix modify/delete resolution in the recursive case Elijah Newren
2011-08-12 5:20 ` [PATCHv2 39/56] merge-recursive: Introduce a merge_file convenience function Elijah Newren
2011-08-12 5:20 ` [PATCHv2 40/56] merge-recursive: Fix rename/rename(1to2) resolution for virtual merge base Elijah Newren
2011-08-12 5:20 ` [PATCHv2 41/56] merge-recursive: Small cleanups for conflict_rename_rename_1to2 Elijah Newren
2011-08-12 5:20 ` [PATCHv2 42/56] merge-recursive: Defer rename/rename(2to1) handling until process_entry Elijah Newren
2011-08-12 5:20 ` [PATCHv2 43/56] merge-recursive: Record more data needed for merging with dual renames Elijah Newren
2011-08-12 5:20 ` [PATCHv2 44/56] merge-recursive: Create function for merging with branchname:file markers Elijah Newren
2011-08-12 5:20 ` [PATCHv2 45/56] merge-recursive: Consider modifications in rename/rename(2to1) conflicts Elijah Newren
2011-08-12 5:20 ` [PATCHv2 46/56] merge-recursive: Make modify/delete handling code reusable Elijah Newren
2011-08-12 5:20 ` [PATCHv2 47/56] merge-recursive: Have conflict_rename_delete reuse modify/delete code Elijah Newren
2011-08-12 5:20 ` [PATCHv2 48/56] merge-recursive: add handling for rename/rename/add-dest/add-dest Elijah Newren
2011-08-12 5:20 ` [PATCHv2 49/56] merge-recursive: Fix working copy handling for rename/rename/add/add Elijah Newren
2011-08-12 5:20 ` [PATCHv2 50/56] t3030: fix accidental success in symlink rename Elijah Newren
2011-08-12 5:20 ` [PATCHv2 51/56] t6022: Add testcase for spurious "refusing to lose untracked" messages Elijah Newren
2011-08-12 5:20 ` [PATCHv2 52/56] merge-recursive: Fix spurious 'refusing to lose untracked file...' messages Elijah Newren
2011-08-12 5:20 ` [PATCHv2 53/56] t6022: Additional tests checking for unnecessary updates of files Elijah Newren
2011-08-12 5:20 ` [PATCHv2 54/56] merge-recursive: Avoid unnecessary file rewrites Elijah Newren
2011-08-12 5:20 ` [PATCHv2 55/56] t6036: criss-cross + rename/rename(1to2)/add-dest + simple modify Elijah Newren
2011-08-12 5:20 ` [PATCHv2 56/56] merge-recursive: Fix virtual merge base for rename/rename(1to2)/add-dest Elijah Newren
2011-08-12 5:48 ` [PATCHv2 00/57] Re-roll of en/merge-recursive from pu Junio C Hamano
2011-08-12 21:59 ` Elijah Newren
2011-08-13 2:37 ` Elijah Newren
2011-08-13 2:23 ` [PATCHv2 57/57] merge-recursive: Don't re-sort a list whose order we depend upon Elijah Newren
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=4E463F01.5060301@kdbg.org \
--to=j6t@kdbg.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jgfouca@sandia.gov \
--cc=newren@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 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).