Re: [PATCH v8 06/16] sequencer: introduce the `merge` command

From: Phillip Wood <phillip.wood@talktalk.net>
Date: Apr 23, 2018, 15:54 UTC
Message-ID: <ec58f482-ffde-1959-ff4a-9b128905ccb0@talktalk.net>
In-Reply-To: <nycvar.QRO.7.76.6.1804231405230.15174@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>

On 23/04/18 13:20, Johannes Schindelin wrote:

Show 34 quoted lines

> Hi Phillip,
> 
> On Sat, 21 Apr 2018, Phillip Wood wrote:
> 
>> On 21/04/18 11:33, Johannes Schindelin wrote:
>>> This patch is part of the effort to reimplement `--preserve-merges` with
>>> a substantially improved design, a design that has been developed in the
>>> Git for Windows project to maintain the dozens of Windows-specific patch
>>> series on top of upstream Git.
>>>
>>> The previous patch implemented the `label` and `reset` commands to label
>>> commits and to reset to labeled commits. This patch adds the `merge`
>>> command, with the following syntax:
>>
>> The two patches seem to have been fused together in this series.
> 
> Indeed. I have yet to investigate further how that happened, it could be a
> bug in my series after all.
> 
>> If the reset command fails because it would overwrite untracked files it
>> says
>>
>> error: Untracked working tree file 'b' would be overwritten by merge.
>>
>> Followed by the hint to edit the todo file. Saying 'merge' rather 'reset' is
>> possibly confusing to users. Perhaps it could call
>> setup_unpack_trees_porcelain(), though that would need to be extended to
>> handle 'reset'.
> 
> Yes, and it changes global state :-(
> 
> Maybe we can leave it as-is for now? After all, it should be clear to the
> user what is happening. The most important part is the "Untracked working
> tree file"...

I'm fine with leaving it, I've might get round to doing a small series to clean things up slightly in a few weeks. At the moment setup_unpack_trees_porcelain() leaks memory as it is called for each merge and allocates new strings each time. It would also be nice if the error messages reflected the command, so it said 'cherry-pick', 'revert' or 'reset' rather than 'merge'

Show 15 quoted lines

>> Also it currently refuses to overwrite ignored files which is either
>> annoying or safe depending on one's point of view.
> 
> Let me think about that. My gut feeling says: if it is easy to do, then
> let's nuke ignored files, but keep untracked files. But I do not think
> that the unpack-trees machinery was taught to know about .gitignore...
> 
> Seeing as `label` and `reset` really are mostly about revisions we see
> along the lines, I think that the common case will *not* overwrite any
> untracked files, ever. You would have to use `reset` on a
> not-previously-seen commit and/or add `exec` commands designed to
> interfere with the `reset`.
> 
> So I tend to want to not bother with discerning between untracked and
> ignored files here.

I don't think it's a pressing concern. In the past I once had a patch series that removed some tracked files in favor of having the build system generate them and added them to .gitignore. Each time I rebased I had to manually remove them at some stage which was annoying but that is quite a rare occurrence.

Best Wishes

Phillip

> Ciao,
> Dscho
>