From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-2.6 required=3.0 tests=AWL,BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD shortcircuit=no autolearn=no autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id D69891F404 for ; Mon, 29 Jan 2018 22:15:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751619AbeA2WP5 (ORCPT ); Mon, 29 Jan 2018 17:15:57 -0500 Received: from mout.gmx.net ([212.227.17.22]:50156 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751473AbeA2WP4 (ORCPT ); Mon, 29 Jan 2018 17:15:56 -0500 Received: from MININT-KR8J64V.europe.corp.microsoft.com ([37.201.193.1]) by mail.gmx.com (mrgmx103 [212.227.17.168]) with ESMTPSA (Nemesis) id 0Lzsf1-1esbq84Bn9-014xTn; Mon, 29 Jan 2018 23:15:52 +0100 Date: Mon, 29 Jan 2018 23:15:50 +0100 (STD) From: Johannes Schindelin X-X-Sender: virtualbox@MININT-6BKU6QN.europe.corp.microsoft.com To: Junio C Hamano cc: git@vger.kernel.org, Jacob Keller Subject: Re: [PATCH 2/8] sequencer: introduce the `merge` command In-Reply-To: Message-ID: References: <647382ac70bfb7035345304a32d08f4e7b51cd40.1516225925.git.johannes.schindelin@gmx.de> User-Agent: Alpine 2.21.1 (DEB 209 2017-03-23) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Provags-ID: V03:K0:HntHDHzlscRKeRenaZhK4+eHxR3nHuNXisVcGMYAzw4SoHR5GP+ hZnolfX76vFI+E2cObAH0qQDJDRftN272agxS1zE4rtm1OyeNxeUldxMKTegoZ3tKAIY14E r9uK+jgSyYXAh9Hbgg8KS104+jT1OLzimbxeTjNDIy+YwsMTEfZkj1eN2Gtuo7BDh1XG/Ak MK9cMeSbQkg3Bwag9iOkQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:msZ1A9xfrPA=:abkv/eecaVsgk2AwB2HIs6 SrKB5ie9geC9FXSv7kp10uCuWlrPRxPI3v7pkFBoTHWFqJlZ31LOr90ZneHzA4QNPLkGUZVsQ QKF36uh4m0wEGGxYNEEQWZ/rrzWjNJ43JIzMta2sg+2MHRQEW+kgRyowkDPYkA0lDpSdZ/eQH dlQuvfiOVQTEbLFpjOL8GzvX6etncfzM+VcQKtRzX52OT2E2UmTdr6JRkQq2TBhwqJMmVthxe bdIHIHBdxC0QjcDXzQjLvjJqusHB5hsfVzeGquBUsLka1z6g5sNu/nPTSn3q7G/MkQy0KLpUb z6aIFAC8SLaibas2D1vekhERB/wdzZpqePl1VWOUC5xUBIl2ipR5qd3/Ge6Y7MzHOfbxhTvMU Ggh5nXG4OQjPaKc2foHcTL82q/eahy5JrwnukkXLY7MZmAPe6lHcJ0+v9nqK1FNiSpU8A6mG3 bNO7Opwyhd7rDNgQRUSfH1njx9rCg3w8W8MrYfNlThep8IPIk24Pn/5+oB040M+bm54vf/nC8 ITiUMLdIyYNQ6OLGUHJcTPw4mahatwhnfi/mkQrZ1D0NfYM9C5zq6GYd/Wl+KDmNAfVybc5cP HgDsDECuxUj2vURlEIM0fHGSXqOJal4imSl5LHlLYcpnVqxTr4eunzBOsaslxecg5xVwXJIek BB81/unPLvAAIg16E1/3LMfi4KTcrgDdiAfiqz/Cuw36W5BoSSclUzx9euXPhN+u8zr1EMLKH x3zVNwvMB+g2FLWGb7mM2ymbY3g71Rzy5+cFYQ658vbzxqA3bf/15jR47FzvkC9zZWICESrJB DmVYOKQu/dgZi66x0V52hnkwt3+Daf7EM+VXdXo5aDI5spwPAo18oMR8Lec7tnS1ZTyGLl2 Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi Junio, On Mon, 22 Jan 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > end_of_object_name = (char *) bol + strcspn(bol, " \t\n"); > > + item->arg = end_of_object_name + strspn(end_of_object_name, " \t"); > > + item->arg_len = (int)(eol - item->arg); > > + > > saved = *end_of_object_name; > > + if (item->command == TODO_MERGE && *bol == '-' && > > + bol + 1 == end_of_object_name) { > > + item->commit = NULL; > > + return 0; > > + } > > + > > *end_of_object_name = '\0'; > > status = get_oid(bol, &commit_oid); > > *end_of_object_name = saved; > > > > - item->arg = end_of_object_name + strspn(end_of_object_name, " \t"); > > - item->arg_len = (int)(eol - item->arg); > > - > > Assigning to "saved" before the added "if we are doing merge and see > '-', do this special thing" is not only unnecessary, but makes the > logic in the non-special case harder to read. The four things > "saved = *eol; *eol = 0; do_thing_using(bol); *eol = saved;" is a > single logical unit; keep them together. True. This was a sloppily resolved merge conflict in one of the many rewrites, I guess. > > + if (*p) > > + len = strlen(p); > > + else { > > + strbuf_addf(&buf, "Merge branch '%.*s'", > > + merge_arg_len, arg); > > + p = buf.buf; > > + len = buf.len; > > + } > > So... "arg" received by this function can be a single non-whitespace > token, which is taken as the name of the branch being merged (in > this else clause). Or it can also be followed by a single liner > message for the merge commit. Presumably, this is for creating a > new merge (i.e. "commit==NULL" case), and preparing a proper log > message in the todo list is unrealistic, so this would be a > reasonable compromise. Those users who want to write proper log > message could presumably follow such "merge" insn with a "x git > commit --amend" or something, I presume, if they really wanted to. Precisely. > > + if (write_message(p, len, git_path_merge_msg(), 0) < 0) { > > + error_errno(_("Could not write '%s'"), > > + git_path_merge_msg()); > > + strbuf_release(&buf); > > + rollback_lock_file(&lock); > > + return -1; > > + } > > + strbuf_release(&buf); > > + } > > OK. Now we have prepared the MERGE_MSG file and are ready to commit. > > > + head_commit = lookup_commit_reference_by_name("HEAD"); > > + if (!head_commit) { > > + rollback_lock_file(&lock); > > + return error(_("Cannot merge without a current revision")); > > + } > > Hmph, I would have expected to see this a lot earlier, before > dealing with the log message. Leftover MERGE_MSG file after an > error will cause unexpected fallout to the end-user experience > (including what is shown by the shell prompt scripts), but if we do > this before the MERGE_MSG thing, we do not have to worry about > error codepath having to remove it. Fixed. > > + strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg); > > + merge_commit = lookup_commit_reference_by_name(ref_name.buf); > > + if (!merge_commit) { > > + /* fall back to non-rewritten ref or commit */ > > + strbuf_splice(&ref_name, 0, strlen("refs/rewritten/"), "", 0); > > + merge_commit = lookup_commit_reference_by_name(ref_name.buf); > > + } > > OK, so "abc" in the example in the log message is looked up first as > a label and then we take a fallback to interpret as an object name. Yes. And auto-generated labels are guaranteed not to be full hex hashes for that reason. > Hopefully allowed names in "label" would be documented clearly in > later steps (I am guessing that "a name that can be used as a branch > name can be used as a label name and vice versa" or something like > that). Well, I thought that it would suffice to say that these labels are available as refs/rewritten/