Re: [PATCH v2 2/7] replace: introduce --convert-graft-file

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 20, 2018, 15:26 UTC
Message-ID: <nycvar.QRO.7.76.6.1804201640560.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <xmqqd0yud6vr.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Fri, 20 Apr 2018, Junio C Hamano wrote:

Show 13 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > This option is intended to help with the transition away from the
> > now-deprecated graft file.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Documentation/git-replace.txt | 11 +++++--
> >  builtin/replace.c             | 59 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 66 insertions(+), 4 deletions(-)
> 
> I expected you to remove convert-grafts-to-replace-refs.sh from the
> contrib/ section in the same patch, actually.

Sorry, I was still under the impression that contrib/ was somewhat off limits when replacing scripts by C code (it used to be the rule, but I did see that the contrib/examples/*.sh files went poof).

Will change.

> FWIW, I think it is a much better approach to give the first-class UI
> for transition like this patch does than "go fish for a good way to
> transition yourself, we may have something useful in contrib/", which is
> what we had so far.

Obviously I agree. It just makes for such a vastly superior user experience to get an explanation that yes, something has been deprecated, but Do Not Panic, this is what you do to get out of this fix.

In the same vein, I considered some real hackery that would make the deprecation notice in commit.c less annoying: I wanted to use some tentative date in the future as cut-off, and then use some arithmetic based on the time left until that date to show the deprecation notice increasingly more often.

However, this would have made it really frustrating for users seeing the notice fly by, say, in a lengthy script's output, and then trying to reproduce the message. So as much fun as it would have been to come up with that formula, I refrained from it.

Show 20 quoted lines

> > +	while (strbuf_getline(&buf, fp) != EOF) {
> > +		int i = 0, j;
> > +
> > +		while (i != buf.len) {
> > +			char save;
> > +
> > +			for (j = i; j < buf.len && !isspace(buf.buf[j]); j++)
> > +				; /* look further */
> > +			save = buf.buf[j];
> > +			buf.buf[j] = '\0';
> > +			argv_array_push(&args, buf.buf + i);
> > +			buf.buf[j] = save;
> 
> It's a shame that we do not have a helper that splits the contents
> of a strbuf at SP and shove the result into an argv_array(). [*1*]
> 
> *1* There is one that splits into an array of strbuf but the point
> of splitting is often that these split pieces are the final thing we
> want, and placing them in separate strbuf (whose strength is that
> contents are easily manipulatable) is pointless.

FWIW I considered introducing such a helper. But I really want to have the full line to show to the user, so I went with the current version.

Based on your comment, I realized that since argv_array_push() duplicates the string *anyway*, I could have implemented argv_array_split().

Briefly deterred by the insight that some readers will invariably think that this function performs de-quoting, Unix shell style, I almost decided against that.

But only almost.

If anybody needs a de-quoting version of argv_array_split(), or a version that uses a different delimiter than white-space, my version should provide a neat starting point (new parameters should be added for those purposes, of course, since we really need a non-de-quoting version in --convert-graft-file).

So the next iteration of this patch series will sport a shiny new argv_array_split(). Enjoy.

Show 12 quoted lines

> > +
> > +			while (j < buf.len && isspace(buf.buf[j]))
> > +				j++;
> > +			i = j;
> 
> One difference I notice while comparing this with what is done by
> contrib/convert-grafts-to-replace-refs.sh is that this does not
> skip a line that begins with # or SP.  I offhand do not know what
> the point of skipping a line that begins with a SP, but I suspect
> that skipping a line that begins with "#" is a desirable thing to
> do, because commit.c::read_graft_line() does know that such a line
> is a valid comment.

Riiiight! I meant to look at the parser to verify that I do the same, but forgot (I had the incorrect recollection that the graft file cannot contain comments or empty lines).

So I remedied that now. Including correct handling of empty lines.

Show 11 quoted lines

> > +		}
> > +
> > +		if (create_graft(args.argc, args.argv, force))
> > +			strbuf_addf(&err, "\n\t%s", buf.buf);
> > +
> > +		argv_array_clear(&args);
> > +		strbuf_reset(&buf);
> 
> Strictly speaking, this reset is redundant, as getline() will always
> stuff the line into a fresh buffer (and after the loop there
> correctly is a release).

Good point. I did assume that strbuf_getline() would append, and I was wrong. I dropped the needless strbuf_reset() call.

Show 12 quoted lines

> > +	}
> > +
> > +	strbuf_release(&buf);
> > +	argv_array_clear(&args);
> > +
> > +	if (!err.len)
> > +		return unlink_or_warn(graft_file);
> > +	warning(_("could not convert the following graft(s):\n%s"), err.buf);
> > +	strbuf_release(&err);
> 
> commit.c::read_graft_file() seems to ignore a broken graft line and
> salvages other lines, and this one follows suit, which is good.

Thanks.

Show 7 quoted lines

> The remaining die() I pointed out in 1/2 can safely be turned into
> return error() for this caller (I didn't check for existing callers,
> though) and would automatically do the right thing.  The real
> consumer of the graft file, commit.c::read_graft_line(), shows an
> error when oid cannot be parsed, and the above code, when
> create_graft() is updated to return an error instead of dying, would
> append the problematic record to buf.buf in the code above.

It is a *lot* worse than just one remaining die(), unfortunately: create_graft() calls replace_parents() and check_mergetags(), both without checking their return value. Because neither return anything. Because they all die (cue Dash from The Incredibles: We're gonna DIIIIIE!). Also, replace_object_oid(), while it returns an int indicating an error, chooses to die() in plenty of places. After following down to for_each_mergetag(), export_object() and check_ref_valid(), I decided to just go ahead and libify the entire builtin/replace.c, which seemed to be the vastly more efficient route, and I really wish we would start deprecating die(). Thanks for sending me down that rabbit hole ;-)

Note: there is a call to get_commit_buffer() which still die()s if it
cannot read the object or if it is not a commit. If we care enough about
the concocted scenario where somebody would have installed a graft file
with a line that references, say, a tree object instead of a commit object
in its first oid, we need to do something about that: the graft line would
simply have been ignored, as grafts were only used when a corresponding
commit was parsed (which would never happen for the concocted example I
gave). I am rather certain, though, that I do not want to care about such
a scenario: a user would have to go out of their way to get into that
situation.

Ciao, Dscho