list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Siddharth Kannan <>
To: Junio C Hamano <>
Subject: Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
Date: Mon, 6 Feb 2017 18:10:26 +0000	[thread overview]
Message-ID: <20170206181026.GA4010@ubuntu-512mb-blr1-01.localdomain> (raw)
In-Reply-To: <>

Hey Junio, I did some more digging into the codepath:

On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
> A correct solution needs to know if the argument is at the position
> where a revision (or revision range) is expected and then give the
> tip of the previous branch when it sees "-" (and other combinations
> this patch tries to cover).  In other words, the parser always knows
> what it is parsing, and if and only if it is parsing a rev, react to
> "-" and think "ah, the user wants me to use the tip of the previous
> branch".
> But the code that knows that it expects to see a revision already
> exists, and it is the real parser.  In the above snippet,
> setup_revisions() is the one that does the real parsing of argv[].
> The code there knows when it wants to see a rev, and takes argv[i]
> and turns into an object to call add_pending_object().  That codepath
> may not yet know that "-" means the tip of the previous branch, and
> that is where the change needs to go.

Inside setup_revisions, it tries to parse arguments and options. In
there, is this line of code:

    if (*arg == '-') {

Once control enters this branch, it will either parse the argument as
an option / pseudo-option, or simply leave this argument as is in the
argv[] array and move forward with the other arguments.

So, first I need to teach setup_revisions that something starting with
a "-" might be a revision or a revision range.

After this, going further down the codepath, in

The argument is parsed to find out if it is of the form
rev1...rev2 or rev1..rev2 or just rev1, etc.

(a) -> If `..` or `...` was found, then two pointers "this" and "next"
now hold the from and to revisions, and the function
get_sha1_committish is called on them. In case both were found to be
committish, then the char pointers now hold the sha1 in them, they are
parsed into objects.

(b) -> Else look for "r1^@", "r1^!" (This could be "-^@", "-^!") To
get r1, again the function get_sha1_committish is called with only r1
as the parameter.

(c) -> Else look for "r1^-"

(d) -> Else look for the argument using the same get_sha1_committish
function (It any "^" was present in it, it has already been noted and

Cases (a), (b) and (d) can be handled by putting this inside
get_sha1_committish. (Further discussion about that below)

Case (c) is a bit confusing. This could be something like "-^-", and
something like "^-" could mean "Not commits on previous branch" or it
could mean "All commits on this branch except for the parent of HEAD"
Please tell me if this is confusing or undesired altogether.
Personally, I feel that people who have been using "^-" would be
very confused if it's behaviour changed.

So, all the code till now points at adding the patch for "-" = "@{-1}"
inside get_sha1_committish or downstream from there.

-> get_sha1_with_context 
-> get_sha1_with_context_1
-> get_sha1_1 
  -> peel_onion -> calling get_sha1_basic again with the ref
  only (after peeling) 
  -> get_sha1_basic -> includes parsing of "@{-N}" type revs. So, 
  this indicates that if we can convert the "-" appropriately 
  before this point, then it would be good.
  -> get_short_sha1

So, this patch reduces to the following 2 tasks:

1. Teach setup_revisions that something starting with "-" can be an
argument as well
2. Teach get_sha1_basic that "-" means the tip of the previous branch
perhaps by replacing it with "@{-1}" just before the reflog parsing is

> A correct solution will be a lot more involved, of course, and I
> think it will be larger than a reasonable microproject for people
> new to the codebase.

So true :) I had spent a fair bit of time already on my previous patch,
and I thought I might as well complete my research into this, and send
this write-up to the mailing list, so that I could write a patch some
time later. In case you would prefer for me to not work on this
anymore because I am new to the codebase, I will leave it at this.

- Siddharth Kannan

  parent reply	other threads:[~2017-02-06 18:10 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-05 12:57 Siddharth Kannan
2017-02-05 14:55 ` Pranit Bauva
2017-02-06  0:15 ` Junio C Hamano
2017-02-06  2:27   ` Siddharth Kannan
2017-02-06 18:10   ` Siddharth Kannan [this message]
2017-02-06 23:09     ` Junio C Hamano
2017-02-07 19:14       ` Siddharth Kannan
2017-02-08 14:40         ` Matthieu Moy
2017-02-08 17:23           ` Siddharth Kannan
2017-02-09 12:25             ` Matthieu Moy
2017-02-09 18:21               ` Siddharth Kannan

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:

  List information:

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

  git send-email \
    --in-reply-to=20170206181026.GA4010@ubuntu-512mb-blr1-01.localdomain \ \ \ \ \ \ \ \ \

* 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 inbox:

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).