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: Philippe Blain via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH] range-diff: show submodule changes irrespective of diff.submodule
Date: Thu, 02 Jun 2022 10:36:44 -0700	[thread overview]
Message-ID: <xmqqwndznf0z.fsf@gitster.g> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2206021724240.349@tvgsbejvaqbjf.bet> (Johannes Schindelin's message of "Thu, 2 Jun 2022 17:36:40 +0200 (CEST)")

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Force the submodule diff format to its default ("short") when invoking
>> 'git log' to generate the patches for each range, such that submodule
>> changes are always shown.
>
> Full disclosure: I do not see much value in range-diffs in the presence of
> submodules. Nothing in the design of range-diffs is prepared for
> submodules.
>
> But since `--submodules=short` does not change anything when running
> `range-diff` in repositories without submodules, I don't mind this change.

IOW, "I wrote it for the purpose of doing X, I do not care those who
have been using it for doing Y, I am OK with changing behaviour on
them".

Philippe, do you have a good guess on other users and workflows that
may benefit from the current behaviour?  I suspect in the longer term
this might have to become configurable, and I am having a hard time
judging if (1) a temporary regression (to them) is acceptable or (2)
the new feature to also show submodule changes is not urgent enough
that it may be better to make it configurable from day one, instead
of using a different hardcoded and only setting like this patch does.

> This test case is very clear and concise, even without my suggested
> changes. Therefore, if you want to keep the patch as-is, I am fine with
> that, too.
>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks for a review.

Will queue.

  reply	other threads:[~2022-06-02 17:36 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-30 13:09 [PATCH] range-diff: show submodule changes irrespective of diff.submodule Philippe Blain via GitGitGadget
2022-05-30 13:46 ` Ævar Arnfjörð Bjarmason
2022-05-31  2:22   ` Philippe Blain
2022-06-02 15:36 ` Johannes Schindelin
2022-06-02 17:36   ` Junio C Hamano [this message]
2022-06-06 20:33     ` Philippe Blain
2022-06-07  1:26       ` Junio C Hamano
2022-06-06 20:18   ` Philippe Blain
2022-06-06 20:59 ` [PATCH v2] " Philippe Blain via GitGitGadget

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=xmqqwndznf0z.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=levraiphilippeblain@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 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.