Re: [PATCH v3 0/7] push: add "--[no-]force-if-includes"

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 16, 2020, 12:47 UTC
Message-ID: <nycvar.QRO.7.76.6.2009161437170.56@tvgsbejvaqbjf.bet>
In-Reply-To: <20200913145413.18351-1-shrinidhi.kaushik@gmail.com>

Hi Srinidhi,

On Sun, 13 Sep 2020, Srinidhi Kaushik wrote:

Show 18 quoted lines

> Add a new option: "--force-if-includes" to `git-push` where forced
> updates are allowed only if the tip of the remote-tracking ref has
> been integrated locally, by verifying if the tip of the remote-tracking
> ref -- on which a local branch has based on -- is reachable from at
> least one of the "reflog" entries of the branch about to be updated
> by force on the remote.
>
> This option can also be used with `--force-with-lease` with setups
> where the remote-tracking refs of the repository are implicitly
> updated in the background to help prevent unintended remote
> overwrites.
>
> If a local branch is based on a remote ref for a rewrite, and if that
> remote-tracking ref is updated by a push from another repository after
> it has been checked out locally, force updating that branch to remote
> with `--force-with-lease[=<refname>[:expect]]` without specifying the
> "<refname>" or "<expect>" values, can cause the update that happened
> in-between the checkout and forced push to be lost.

Thank you for working on this! I gave this an incomplete look-over, and offered some suggestions that you hopefully find useful.

> Changes since v2:
>   * Removed configuration option for setting "--force-if-includes"
>     with "--force-with-lease".
>   * Broke up the patch into smaller commits.

While the commits all seem to be compiling individually, I am not really a fan of introducing a function without a caller that shows how it is supposed to work. I'd rather see some incremental story, and in this case, I think if _I_ were to submit this patch series, I would probably have only two commits: one that extends the already-existing code path to turn that `use_tracking` flag into that `enum`, and the second patch which wires up the option, adds the documentation and the tests.

However, please do not let my tastes dictate how you want to present the work, although I hope that my suggestion inspires you ;-)

Ciao, Dscho