All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Kim Altintop <kim@eagain.st>,
	git@vger.kernel.org, jonathantanmy@google.com,
	bwilliams.eng@gmail.com, jrnieder@gmail.com,
	sunshine@sunshineco.com
Subject: Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace
Date: Sun, 15 Aug 2021 10:59:19 -0700	[thread overview]
Message-ID: <xmqqeeausi6g.fsf@gitster.g> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2108142344570.59@tvgsbejvaqbjf.bet> (Johannes Schindelin's message of "Sat, 14 Aug 2021 23:46:49 +0200 (CEST)")

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.

  reply	other threads:[~2021-08-15 18:01 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 13:59 [PATCH] upload-pack.c: " Kim Altintop
2021-07-30 14:04 ` Kim Altintop
2021-07-30 18:57 ` Junio C Hamano
2021-07-30 21:08   ` Kim Altintop
2021-07-31 20:36 ` [PATCH v2] " Kim Altintop
2021-08-02 21:06   ` Jonathan Tan
2021-08-04 20:36     ` Kim Altintop
2021-08-04 20:42   ` [PATCH v3] " Kim Altintop
2021-08-04 21:00     ` [PATCH v4] " Kim Altintop
2021-08-09 17:56       ` [PATCH 0/3] upload-pack: " Kim Altintop
2021-08-09 17:56         ` [PATCH 1/3] t5730: introduce fetch command helper Kim Altintop
2021-08-09 19:16           ` Junio C Hamano
2021-08-09 21:18             ` Kim Altintop
2021-08-09 19:40           ` Jonathan Nieder
2021-08-09 21:43             ` Junio C Hamano
2021-08-09 21:56             ` Kim Altintop
2021-08-09 22:03               ` Junio C Hamano
2021-08-09 23:01                 ` Jonathan Nieder
2021-08-10  9:44                   ` Kim Altintop
2021-08-09 17:57         ` [PATCH 2/3] upload-pack.c: treat want-ref relative to namespace Kim Altintop
2021-08-09 17:57         ` [PATCH 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces Kim Altintop
2021-08-10  9:49           ` Kim Altintop
2021-08-13  6:23         ` [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace Kim Altintop
2021-08-14 21:46           ` Johannes Schindelin
2021-08-15 17:59             ` Junio C Hamano [this message]
2021-08-15 19:35             ` Kim Altintop
2021-08-16 12:39               ` Johannes Schindelin
2021-08-13  6:23         ` [PATCH v6 1/3] t5730: introduce fetch command helper Kim Altintop
2021-08-13  6:23         ` [PATCH v6 2/3] upload-pack.c: treat want-ref relative to namespace Kim Altintop
2021-08-13  6:23         ` [PATCH v6 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces Kim Altintop
2021-08-04 21:15     ` [PATCH v3] upload-pack.c: treat want-ref relative to namespace Junio C Hamano
2021-08-04 22:04       ` Kim Altintop
2021-08-04 22:17         ` Eric Sunshine
2021-08-04 22:17         ` Junio C Hamano
2021-08-04 22:23         ` Junio C Hamano

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=xmqqeeausi6g.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=bwilliams.eng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=kim@eagain.st \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace' \
    /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

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.