git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] rebase: accept indented comments (fixes regression)

From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date
Sep 30, 2015, 19:17 UTC
Message-ID
<vpqwpv7hg41.fsf@grenoble-inp.fr>
In-Reply-To
<xmqqd1wz9402.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> With Git <2.0.6, 'git rebase' used to accept lines starting with
>> whitespaces followed with '#' as a comment. This was broken by
>> 804098b (git rebase -i: add static check for commands and SHA-1,
>> 2015-06-29), which introduced additional checks on the TODO-list using
>> "git stripspaces" which only strips comments starting at the first
>> column.
>
> I cannot help thinking that this is sidestepping the real issue.
>
> The real issue, I think, is that the new check tokenises the input
> differently from how the expand_todo_ids -> transform_todo_ids
> callchain parses it.  The problem Nazri Ramliy noticed about the new
> check that does not ignore the indentation is merely one aspect of
> it.
Right.
Show 6 quoted lines
> Stripping leading whilespaces with sed may ignore indented anything
> and help Nazri's script, but 804098b tightened checks to disallow
> other things that we historically allowed, e.g. if you replace SP
> between "pick" and the commit object name with an HT, the new check
> will not notice that HT is also a perfectly good token separator
> character and barfs.
Indeed. I'm adding a test for that too.
> I am actually tempted to say that we should revert 804098b, which is
> the simplest fix.

I think the commit has value, and reverting it makes the "drop" command essentially useless.

> As far as I can tell, the hand-rolled parsing is there only in oder
> to report the incoming $line as-is.

Indeed, I remember finding the parsing code weird when I reviewed it, and the reason was to provide the exact line.

> It is much easier to just identify with which line number the location
> of the problem, and show it when it is necessary from the original
> source, and we do not care about performance in the error codepath.
Agreed.
> Perhaps something along these lines instead, with your new tests
> added in?
Sounds good, yes. I'll send a patch with this and my updated tests.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Previous: Junio C HamanoNext: Junio C Hamano
Message 4 of 13 in “rebase: accept indented comments (fixes regression)”
  1. rebase: accept indented comments (fixes regression)Matthieu Moy, Sep 30, 2015
  2. Remi Galan AlfonsoSep 30, 2015
  3. Junio C HamanoSep 30, 2015
  4. Matthieu MoySep 30, 2015
  5. Junio C HamanoSep 30, 2015
  6. Junio C HamanoSep 30, 2015
  7. Matthieu MoySep 30, 2015
  8. Junio C HamanoSep 30, 2015
  9. 1/2 rebase-i: explicitly accept tab as separator in commandsMatthieu Moy, Oct 1, 2015
  10. 2/2 rebase-i: loosen over-eager check_bad_cmd checkMatthieu Moy, Oct 1, 2015
  11. rebase-i: loosen over-eager check_bad_cmd checkMatthieu Moy, Sep 30, 2015
  12. Eric SunshineSep 30, 2015
  13. rebase-i: loosen over-eager check_bad_cmd checkMatthieu Moy, Sep 30, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.