git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Adam Dinwoodie <adam@dinwoodie.org>
Cc: git@vger.kernel.org, Christian Couder <christian.couder@gmail.com>
Subject: Re: [RFC PATCH v2 0/2] bisect: add a single command for editing logs
Date: Wed, 22 Nov 2017 14:35:28 +0900	[thread overview]
Message-ID: <xmqqshd6ub8f.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <cover.1511200589.git.adam@dinwoodie.org> (Adam Dinwoodie's message of "Mon, 20 Nov 2017 18:24:37 +0000")

Adam Dinwoodie <adam@dinwoodie.org> writes:

> - It's possible to start a bisect session with a command like `git
>   bisect @ @~10`.  This will lead to the bisect log including the `@`
>   and `@~10` literally, and the interpretation of those values changes
>   depending on the current HEAD.  As a result, if you do a `git bisect
>   edit` after starting a bisect like that, but don't actually edit the
>   file, you'll nonetheless be in a different state.

This is a tangent, but for writing to the general public, please do
spell out HEAD, not the line noise synonym "@" that confuses readers.

>   I can see a few ways of coping with that:
>
>   1. Change the existing `git bisect start` behaviour to run arguments
>      through `git rev-parse` before recording them.  It appears `git
>      bisect good` et al. already do that, but it is a change in
>      behaviour that I guess could impact badly on other people using
>      `git bisect log`-based workflows.

The issue is not just HEAD but also for anything fruid, i.e. the
name of a branch, a search result ":/pattern", etc., and if we want
to allow restarting a previously failed bisect session from a
midpoint, we should be recording things in absolute terms as early
as possible.  I'd think it was an oversight the "log" thing did not
do so.

>   2. Do a full `git bisect reset` before replaying the log, so the
>      revisions will be parsed in the same way as they were originally.
>      I'd be slightly sad about that, as it seems an unnecessary
>      inefficiency, but it may well be the simplest approach.

It is not just inefficient, but would require there is no a local
change; I thought that the current system allows you to have a local
modification to a path that is not involved in the bisect session
and losing that property would be sad.

> - There aren't yet any tests or documentation changes; I wanted to get
>   commentary on the initial code changes before I spent time on those
>   parts.

There are some chicken-and-egg around this area.  For some changes,
without a doc update and test addition, it is harder to judge if a
reviewer can agree with the proposed change, as there is only a high
level description "we allow editing" and the lowest level changes to
the actual code, without anything in between that describes the
guiding principle and design decision that lead to the patch.

I'll need to see if the changes in the patch is clear/trivial enough
to see where you are trying to go to see if it is the case for this
patch, though, so read the above paragraph as a general guideline.



  reply	other threads:[~2017-11-22  5:35 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-08 13:59 [RFC PATCH] rebisect: add script for easier bisect log editing Adam Dinwoodie
2017-11-08 16:12 ` Christian Couder
2017-11-08 16:15   ` Christian Couder
2017-11-08 16:50     ` Adam Dinwoodie
2017-11-08 16:32   ` Adam Dinwoodie
2017-11-20 18:24 ` [RFC PATCH v2 0/2] bisect: add a single command for editing logs Adam Dinwoodie
2017-11-22  5:35   ` Junio C Hamano [this message]
2017-11-20 18:24 ` [RFC PATCH v2 1/2] bisect: split out replay file parsing Adam Dinwoodie
2017-11-20 18:24 ` [RFC PATCH v2 2/2] bisect: add "edit" command Adam Dinwoodie

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqshd6ub8f.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=adam@dinwoodie.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).