git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shourya Shukla <shouryashukla.oo@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: kaartic.sivaraam@gmail.com, git@vger.kernel.org,
	christian.couder@gmail.com, gitster@pobox.com,
	liu.denton@gmail.com, pc44800@gmail.com, stefanbeller@gmail.com
Subject: Re: [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C
Date: Wed, 15 Jul 2020 20:23:36 +0530	[thread overview]
Message-ID: <20200715145336.GA18071@konoha> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2007120230400.50@tvgsbejvaqbjf.bet>

Hello Johannes!

On 12/07 02:46, Johannes Schindelin wrote:
> > > >>> +	cp_rev.git_cmd = 1;
> > > >>> +	argv_array_pushl(&cp_rev.args, "rev-parse", "-q", "--verify",
> > > >>> +			 argc ? argv[0] : "HEAD", NULL);
> > > >>
> > > >> Oy. Why not simply call `get_oid()`? No need to spawn a new process.
> > > >
> > > > Then everytime we need 'rev-parse', we simply call 'get_oid()'? That
> > > > will save us a ton of processes?
> > > >
> > > > But I think we do need to capture the output of 'git rev-parse --verify
> > > > ....' so I think it will backfire to use 'get_oid()' or am I just being
> > > > too dumb and not catching on something?
> > > >
> > >
> > > I'll leave this for others to answer.
> >
> > I will resolve this one after Dscho answers then.
> 
> The `rev-parse` command _would_ essentially call `get_oid()` and print the
> result (after converting it to hex, which you don't need, because the
> caller actually would parse the hex string anyway).
> 
> You can verify my claim by following the code:
> https://github.com/git/git/blob/v2.27.0/builtin/rev-parse.c#L956-L982 (it
> is slightly harder to follow because the `--verify` option makes sure that
> only one rev is shown, which means that the `get_oid_with_context()` call
> is separated from the corresponding `show_rev()` call).
> 
> So there really is no need to capture the output of that `rev-parse`
> command.

Yep.

> It is a different story, of course, when capturing the output of Git
> commands _that are run in a submodule_. Those currently still need to be
> spawned.

Oh alright, since we don't need to capture the output and rather use the
command for a particular functionality it offers to, in our just fetch
the OID of head, we use a helper function which the command actually
uses to make things happen instead of spawning a process. Is this
correct?

> > > >>> +
> > > >>> +	if (!capture_command(&cp_rev, &sb, 0)) {
> > > >>> +		strbuf_strip_suffix(&sb, "\n");
> > > >>> +		if (argc) {
> > > >>> +			argv++;
> > > >>> +			argc--;
> > > >>> +		}
> > > >>> +	} else if (!argc || !strcmp(argv[0], "HEAD")) {
> > > >>> +		/* before the first commit: compare with an empty tree */
> > > >>> +		struct stat st;
> > > >>> +		struct object_id oid;
> > > >>> +		if (fstat(0, &st) < 0 || index_fd(&the_index, &oid, 0, &st, 2,
> > > >>> +						  prefix, 3))
> > > >>> +			die("Unable to add %s to database", oid.hash);
> > > >>
> > > >> Umm. The original reads:
> > > >>
> > > >>                 # before the first commit: compare with an empty tree
> > > >>                 head=$(git hash-object -w -t tree --stdin </dev/null)
> > > >>
> > > >> It does not actually read from `stdin`. It reads from `/dev/null`,
> > > >> redirected to the input. And what it _actually_ does is to generate the
> > > >> OID of the empty tree.
> > > >>
> > > >> But we already _have_ the OID of the empty tree! It's
> > > >> `the_hash_algo->empty_tree`.
> > > >
> > > > I did not know this 'the_hash_algo'. I will use it. Thanks! :)
> > > >
> > > >> I hope that this is covered by the test suite. Please check that. The test
> > > >> would succeed with your version, but only because tests are run with
> > > >> `stdin` redirected from `/dev/null` by default.
> > > >
> > > > I guess yes. My work passed because the tests are written this way.
> > > >
> > > >>> +		strbuf_addstr(&sb, oid_to_hex(&oid));
> > > >>> +		if (argc) {
> > > >>> +			argv++;
> > > >>> +			argc--;
> > > >>> +		}
> > > >>> +	} else {
> > > >>> +		strbuf_addstr(&sb, "HEAD");
> > > >>> +	}
> > > >>
> > > >> The conversion to C would make for a fine excuse to simplify the logic.
> > > >
> > > > This was kind of like the 'shift' in shell. What equivalent do you
> > > > suggest?
> > > >
> > >
> > > I think that's just a general comment after the other comments found
> > > just above about simplifying things.
> >
> > Alright. But I do have to simplify the logic right?
> 
> You do not _have_ to. But it would make for a good opportunity to do that,
> I think, as the code is really hard to follow.
> 
> The idea here is actually not at all hard to understand, though: use
> the speficied rev (falling back to `HEAD`) to compare to, unless it is a
> yet-unborn `HEAD` in which case you compare to the empty tree.
> 
> It is very, very similar in spirit to the code in `do_pick_commit()` in
> `sequencer.c`:
> https://github.com/git/git/blob/v2.27.0/sequencer.c#L1765-L1774
> 
> The only difference is that you will also have to fall back to using
> `HEAD` if the argument in question turns out not to refer to a revision
> but is actually the first pathspec.

Alright. So do you suggest a code segment like this?

--------8<---------------------

module_summary() {
struct object_id *head_oid = NULL;
.....
.....
if (get_oid(argc ? argv[0] : "HEAD", head_oid)) {
			if (argc) {
			argv++;
			argc--;
		}
	} else if (!argc || !strcmp(argv[0], "HEAD")) {
		/* before the first commit: compare with an empty tree */
		oidcpy(head_oid, the_hash_algo->empty_tree);
		if (argc) {
			argv++;
			argc--;
		}
	} else {
		get_oid("HEAD", head_oid);
	}
.....
.....
}

*Passing head_oid as a pointer into the fucntion below*

compute_summary_module_list(..., struct object_id *head_oid,...) {
	.....
    .....
    if (head_oid) {
		argv_array_push(&diff_args, oid_to_hex(head_oid));
	}
    .....
    .....
}

----------->8-------------

When I try this, I get a:

    BUG: diff-lib.c:526: run_diff_index must be passed exactly one tree

And this occurs when we are inside the first if-statement. I do not
understand how we are passing multiple trees or making it seem as if
there are multiple trees? Also, even when we do not enter the first if,
we still get a segmentation fault. Why so?

Is this logic correct? I took some inspiration from the links you sent
above as well as some advice from Christian and Kaartic on this.

  reply	other threads:[~2020-07-15 14:53 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-02 19:24 [GSoC][PATCH 0/4] submodule: port 'summary' from Shell " Shourya Shukla
2020-07-02 19:24 ` [PATCH 1/4] submodule: amend extra line feed between callback struct and macro Shourya Shukla
2020-07-03 14:57   ` Johannes Schindelin
2020-07-03 15:37     ` Philip Oakley
2020-07-04 15:21       ` Shourya Shukla
2020-07-04 15:39         ` Đoàn Trần Công Danh
2020-07-05 10:52           ` Philip Oakley
2020-07-02 19:24 ` [PATCH 2/4] submodule: rename helper functions to avoid ambiguity Shourya Shukla
2020-07-02 19:24 ` [PATCH 3/4] diff: change scope of the function count_lines() Shourya Shukla
2020-07-03 15:07   ` Johannes Schindelin
2020-07-04 15:28     ` Shourya Shukla
2020-07-04 21:46       ` Christian Couder
2020-07-02 19:24 ` [PATCH 4/4] submodule: port submodule subcommand 'summary' from shell to C Shourya Shukla
2020-07-03 20:46   ` Johannes Schindelin
2020-07-05 17:34     ` Shourya Shukla
2020-07-06  9:16       ` Kaartic Sivaraam
2020-07-06 11:15         ` Shourya Shukla
2020-07-12  0:46           ` Johannes Schindelin
2020-07-15 14:53             ` Shourya Shukla [this message]
2020-07-15 18:41               ` Shourya Shukla
2020-08-10 13:24                 ` Johannes Schindelin

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

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

  git send-email \
    --in-reply-to=20200715145336.GA18071@konoha \
    --to=shouryashukla.oo@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --cc=pc44800@gmail.com \
    --cc=stefanbeller@gmail.com \
    --subject='Re: [PATCH 4/4] submodule: port submodule subcommand '\''summary'\'' from shell to C' \
    /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

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 NNTP newsgroup(s).