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

Re: [PATCH 3/3] gitweb: make gitweb_check_feature a boolean wrapper

From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date
Nov 30, 2008, 01:31 UTC
Message-ID
<cb7bb73a0811291731g7f8770f7p89e924c00d2ab004@mail.gmail.com>
In-Reply-To
<7vej0uysbs.fsf@gitster.siamese.dyndns.org>
On Sun, Nov 30, 2008 at 1:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> "Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
>
>> On Sat, Nov 29, 2008 at 10:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>  * Again, this is to demonstrate how I would have liked to see your
>>>   patches as a reviewable series.  Notice how this naturally justifies
>>>   the dropping of parentheses in many lines that begin with "my", and
>>>   makes it easier to review?  You can review the patch easily by knowing
>>>   that callers that have s/get/check/ are now safe to use scalar context.
>>
>> Yes, I get the point: do less things under the hood. I also still
>> think that three patches with code going ping-pong are way too much,
>> though 8-P
>>
>> For what it's worth, you get my Ack: to your patchset 8-)
>
> Well, I am not interested in taking credits for this series.  I am very
> much interested in reducing my future workload by showing people how an
> easily reviewable series should look like.
>
> So if you still think it is "way too much", I did not succeed what I tried
> to do X-<.

At least as far as this patch goes, from my point of view, the 'clueless/careless' usages of gitweb_check_feature are the conceptually (although not code-wise) correct ones, so they shouldn't be the ones touched by the patch, considering that in addition to the futureproofing, (implicit) fixing those usages is one of the main points of the patch itself.

OTOH, I _do_ get the point about ease of review. But for example (and for future reference for myself): would you say that it would have been enough to have a cleaner commit message explicitly mentioning the fact that formerly incorrect usages of the gitweb_check_feature() were left intact because they were now correct?

Something like this http://git.oblomov.eu/git/patch/gitweb/check-feature-v3
-- 
Giuseppe "Oblomov" Bilotta
Previous: Junio C HamanoNext: Giuseppe Bilotta
Message 13 of 26 in “[PATCHv2 0/2] fixes to gitweb feature check code”
  1. Giuseppe BilottaNov 28, 2008
  2. 1/2 gitweb: fixes to gitweb feature check codeGiuseppe Bilotta, Nov 28, 2008
  3. 2/2 gitweb: clean up git_check_feature() callsGiuseppe Bilotta, Nov 28, 2008
  4. 2/2 gitweb: clean up gitweb_check_feature() callsGiuseppe Bilotta, Nov 29, 2008
  5. Jakub NarebskiNov 29, 2008
  6. 2/2 gitweb: clean up gitweb_check_feature() callsGiuseppe Bilotta, Nov 29, 2008
  7. Junio C HamanoNov 29, 2008
  8. Junio C HamanoNov 29, 2008
  9. 2/3 gitweb: rename gitweb_check_feature to gitweb_get_featureJunio C Hamano, Nov 29, 2008
  10. 3/3 gitweb: make gitweb_check_feature a boolean wrapperJunio C Hamano, Nov 29, 2008
  11. Giuseppe BilottaNov 29, 2008
  12. Junio C HamanoNov 30, 2008
  13. Giuseppe BilottaNov 30, 2008
  14. gitweb: fixes to gitweb feature check codeGiuseppe Bilotta, Nov 30, 2008
  15. Jakub NarebskiDec 2, 2008
  16. [PATCHv3bis] gitweb: fixes to gitweb feature check codeGiuseppe Bilotta, Dec 2, 2008
  17. Junio C HamanoDec 3, 2008
  18. Junio C HamanoDec 2, 2008
  19. Jakub NarebskiDec 3, 2008
  20. Giuseppe BilottaNov 29, 2008
  21. Junio C HamanoNov 29, 2008
  22. Jakub NarebskiNov 29, 2008
  23. Jakub NarebskiNov 29, 2008
  24. Jakub NarebskiNov 30, 2008
  25. Jakub NarebskiNov 29, 2008
  26. Giuseppe BilottaNov 29, 2008

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.