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

Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 15, 2021, 17:59 UTC
Message-ID
<xmqqeeausi6g.fsf@gitster.g>
In-Reply-To
<nycvar.QRO.7.76.6.2108142344570.59@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> My only comment is that I would find the diff to `upload-pack.c` much
> easier to parse if the `arg` variable hadn't been renamed.
If we only look at the patch text, perhaps.

What is left in the resulting file is what people will be reading in the coming years, and because instead of just one string (whose name did not matter and a short-and-sweet arg was a perfect name for it), we now have to deal with two strings (one is the full and real name, and the other is the name without namespace prefix) we need to be clear which is which, the rename is an essential part of the patch to keep the result easier to understand.

When I apply a patch to my tree to review it (e-mailed patch review is merely an efficient way to transmit the change---I do not expect that I can always review all non-trivial patches without stepping out of my MUA). My review, after noticing typos, style glitches and design issues, of code and its correctness will begin after that.

"git show" can be driven with different options and in some cases use of certaion options helps seeing what is going on quite well. "--diff-algorithm" shifts which preimage lines match which postimage lines and sometimes helps reduce the clutter greatly. "--word-diff" is a great tool to see where in reflowed documentation patch actual changes are. "--color-moved" lets reviewer skip pure code movement, various options to "ignore space" sometimes helps declutter the patch. etc.

I wonder if we already have an option (or if there isn't, if we can design such a new option cleanly), that would help in this case. Essentially, what you want is an "I know what used to be arg is renamed to refname_nons in this patch; take advantage of that knowledge and show a diff that is less cluttered" option.

Thanks for a comment, anyway.
Previous: Johannes SchindelinNext: Kim Altintop
Message 25 of 35 in “upload-pack.c: treat want-ref relative to namespace”
  1. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Jul 30, 2021
  2. Kim AltintopJul 30, 2021
  3. Junio C HamanoJul 30, 2021
  4. Kim AltintopJul 30, 2021
  5. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Jul 31, 2021
  6. Jonathan TanAug 2, 2021
  7. Kim AltintopAug 4, 2021
  8. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 4, 2021
  9. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 4, 2021
  10. 0/3 upload-pack: treat want-ref relative to namespaceKim Altintop, Aug 9, 2021
  11. 1/3 t5730: introduce fetch command helperKim Altintop, Aug 9, 2021
  12. Junio C HamanoAug 9, 2021
  13. Kim AltintopAug 9, 2021
  14. Jonathan NiederAug 9, 2021
  15. Junio C HamanoAug 9, 2021
  16. Kim AltintopAug 9, 2021
  17. Junio C HamanoAug 9, 2021
  18. Jonathan NiederAug 9, 2021
  19. Kim AltintopAug 10, 2021
  20. 2/3 upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 9, 2021
  21. 3/3 docs: clarify the interaction of transfer.hideRefs and namespacesKim Altintop, Aug 9, 2021
  22. Kim AltintopAug 10, 2021
  23. 0/3 upload-pack: treat want-ref relative to namespaceKim Altintop, Aug 13, 2021
  24. Johannes SchindelinAug 14, 2021
  25. Junio C HamanoAug 15, 2021
  26. Kim AltintopAug 15, 2021
  27. Johannes SchindelinAug 16, 2021
  28. 1/3 t5730: introduce fetch command helperKim Altintop, Aug 13, 2021
  29. 2/3 upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 13, 2021
  30. 3/3 docs: clarify the interaction of transfer.hideRefs and namespacesKim Altintop, Aug 13, 2021
  31. Junio C HamanoAug 4, 2021
  32. Kim AltintopAug 4, 2021
  33. Eric SunshineAug 4, 2021
  34. Junio C HamanoAug 4, 2021
  35. Junio C HamanoAug 4, 2021

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.