git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv2 27/56] string-list: Add API to remove an item from an unsorted list

From
Johannes Sixt <j6t@kdbg.org>
Date
Aug 13, 2011, 09:08 UTC
Message-ID
<4E463F01.5060301@kdbg.org>
In-Reply-To
<CABPp-BFsqDhVnxDa9u3tV0yZky_az0MTCT_N5y+Oqm84+fOyvQ@mail.gmail.com>
Am 13.08.2011 00:14, schrieb Elijah Newren:
Show 15 quoted lines
> 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
Previous: Elijah NewrenNext: Elijah Newren
Message 32 of 65 in “[PATCHv2 00/57] Re-roll of en/merge-recursive from pu”
  1. Elijah NewrenAug 12, 2011
  2. 01/56 t6042: Add a testcase where git deletes an untracked fileElijah Newren, Aug 12, 2011
  3. 02/56 t6042: Add failing testcase for rename/modify/add-source conflictElijah Newren, Aug 12, 2011
  4. 03/56 t6042: Add a pair of cases where undetected renames cause issuesElijah Newren, Aug 12, 2011
  5. 04/56 t6042: Add a testcase where undetected rename causes silent file deletionElijah Newren, Aug 12, 2011
  6. 05/56 t6042: Add tests for content issues with modify/rename/directory conflictsElijah Newren, Aug 12, 2011
  7. 06/56 t6042: Ensure rename/rename conflicts leave index and workdir in sane stateElijah Newren, Aug 12, 2011
  8. 07/56 t6042: Add failing testcases for rename/rename/add-{source,dest} conflictsElijah Newren, Aug 12, 2011
  9. 08/56 t6036: Add differently resolved modify/delete conflict in criss-cross testElijah Newren, Aug 12, 2011
  10. 09/56 t6036: criss-cross with weird content can fool git into clean mergeElijah Newren, Aug 12, 2011
  11. 10/56 t6036: tests for criss-cross merges with various directory/file conflictsElijah Newren, Aug 12, 2011
  12. 11/56 t6036: criss-cross w/ rename/rename(1to2)/modify+rename/rename(2to1)/modifyElijah Newren, Aug 12, 2011
  13. 12/56 t6036: criss-cross + rename/rename(1to2)/add-source + modify/modifyElijah Newren, Aug 12, 2011
  14. 13/56 t6022: Remove unnecessary untracked files to make test cleanerElijah Newren, Aug 12, 2011
  15. 14/56 t6022: New tests checking for unnecessary updates of filesElijah Newren, Aug 12, 2011
  16. 15/56 t6022: Add testcase for merging a renamed file with a simple changeElijah Newren, Aug 12, 2011
  17. 16/56 merge-recursive: Make BUG message more legible by adding a newlineElijah Newren, Aug 12, 2011
  18. 17/56 merge-recursive: Correct a commentElijah Newren, Aug 12, 2011
  19. 18/56 merge-recursive: Mark some diff_filespec struct arguments constElijah Newren, Aug 12, 2011
  20. 19/56 merge-recursive: Consolidate different update_stages functionsElijah Newren, Aug 12, 2011
  21. 20/56 merge-recursive: Remember to free generated unique path namesElijah Newren, Aug 12, 2011
  22. 21/56 merge-recursive: Avoid working directory changes during recursive caseElijah Newren, Aug 12, 2011
  23. 22/56 merge-recursive: Fix recursive case with D/F conflict via add/add conflictElijah Newren, Aug 12, 2011
  24. 23/56 merge-recursive: Fix sorting order and directory change assumptionsElijah Newren, Aug 12, 2011
  25. 24/56 merge-recursive: Fix code checking for D/F conflicts still being presentElijah Newren, Aug 12, 2011
  26. 25/56 merge-recursive: Save D/F conflict filenames instead of unlinking themElijah Newren, Aug 12, 2011
  27. 26/56 merge-recursive: Split was_tracked() out of would_lose_untracked()Elijah Newren, Aug 12, 2011
  28. 27/56 string-list: Add API to remove an item from an unsorted listElijah Newren, Aug 12, 2011
  29. Johannes SixtAug 12, 2011
  30. Alex RiesenAug 12, 2011
  31. Elijah NewrenAug 12, 2011
  32. Johannes SixtAug 13, 2011
  33. 28/56 merge-recursive: Allow make_room_for_path() to remove D/F entriesElijah Newren, Aug 12, 2011
  34. 29/56 merge-recursive: Split update_stages_and_entry; only update stages at endElijah Newren, Aug 12, 2011
  35. 30/56 merge-recursive: Fix deletion of untracked file in rename/delete conflictsElijah Newren, Aug 12, 2011
  36. 31/56 merge-recursive: Make dead code for rename/rename(2to1) conflicts undeadElijah Newren, Aug 12, 2011
  37. 32/56 merge-recursive: Add comments about handling rename/add-source casesElijah Newren, Aug 12, 2011
  38. 33/56 merge-recursive: Improve handling of rename target vs. directory additionElijah Newren, Aug 12, 2011
  39. 34/56 merge-recursive: Consolidate process_entry() and process_df_entry()Elijah Newren, Aug 12, 2011
  40. 35/56 merge-recursive: Cleanup and consolidation of rename_conflict_infoElijah Newren, Aug 12, 2011
  41. 36/56 merge-recursive: Provide more info in conflict markers with file renamesElijah Newren, Aug 12, 2011
  42. 37/56 merge-recursive: When we detect we can skip an update, actually skip itElijah Newren, Aug 12, 2011
  43. 38/56 merge-recursive: Fix modify/delete resolution in the recursive caseElijah Newren, Aug 12, 2011
  44. 39/56 merge-recursive: Introduce a merge_file convenience functionElijah Newren, Aug 12, 2011
  45. 40/56 merge-recursive: Fix rename/rename(1to2) resolution for virtual merge baseElijah Newren, Aug 12, 2011
  46. 41/56 merge-recursive: Small cleanups for conflict_rename_rename_1to2Elijah Newren, Aug 12, 2011
  47. 42/56 merge-recursive: Defer rename/rename(2to1) handling until process_entryElijah Newren, Aug 12, 2011
  48. 43/56 merge-recursive: Record more data needed for merging with dual renamesElijah Newren, Aug 12, 2011
  49. 44/56 merge-recursive: Create function for merging with branchname:file markersElijah Newren, Aug 12, 2011
  50. 45/56 merge-recursive: Consider modifications in rename/rename(2to1) conflictsElijah Newren, Aug 12, 2011
  51. 46/56 merge-recursive: Make modify/delete handling code reusableElijah Newren, Aug 12, 2011
  52. 47/56 merge-recursive: Have conflict_rename_delete reuse modify/delete codeElijah Newren, Aug 12, 2011
  53. 48/56 merge-recursive: add handling for rename/rename/add-dest/add-destElijah Newren, Aug 12, 2011
  54. 49/56 merge-recursive: Fix working copy handling for rename/rename/add/addElijah Newren, Aug 12, 2011
  55. 50/56 t3030: fix accidental success in symlink renameElijah Newren, Aug 12, 2011
  56. 51/56 t6022: Add testcase for spurious "refusing to lose untracked" messagesElijah Newren, Aug 12, 2011
  57. 52/56 merge-recursive: Fix spurious 'refusing to lose untracked file...' messagesElijah Newren, Aug 12, 2011
  58. 53/56 t6022: Additional tests checking for unnecessary updates of filesElijah Newren, Aug 12, 2011
  59. 54/56 merge-recursive: Avoid unnecessary file rewritesElijah Newren, Aug 12, 2011
  60. 55/56 t6036: criss-cross + rename/rename(1to2)/add-dest + simple modifyElijah Newren, Aug 12, 2011
  61. 56/56 merge-recursive: Fix virtual merge base for rename/rename(1to2)/add-destElijah Newren, Aug 12, 2011
  62. Junio C HamanoAug 12, 2011
  63. Elijah NewrenAug 12, 2011
  64. Elijah NewrenAug 13, 2011
  65. 57/57 merge-recursive: Don't re-sort a list whose order we depend uponElijah Newren, Aug 13, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.