Skip to content

Preliminary fixes for reftable support #669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Preliminary fixes for reftable support #669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f244ed
lib-t6000.sh: write tag using git-update-ref
hanwen Jun 9, 2020
123d246
t3432: use git-reflog to inspect the reflog for HEAD
hanwen Jun 25, 2020
d4007c2
Treat BISECT_HEAD as a pseudo ref
hanwen May 20, 2020
6ca5b99
Cleanse reflog messages in the refs.c layer
gitster Jul 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions builtin/bisect--helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
static GIT_PATH_FUNC(git_path_head_name, "head-name")
static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
Expand Down Expand Up @@ -164,7 +163,7 @@ static int bisect_reset(const char *commit)
strbuf_addstr(&branch, commit);
}

if (!file_exists(git_path_bisect_head())) {
if (!ref_exists("BISECT_HEAD")) {
struct argv_array argv = ARGV_ARRAY_INIT;

argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
Expand Down
4 changes: 2 additions & 2 deletions git-bisect.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ TERM_GOOD=good

bisect_head()
{
if test -f "$GIT_DIR/BISECT_HEAD"
if git rev-parse --verify -q BISECT_HEAD > /dev/null
then
echo BISECT_HEAD
else
Expand Down Expand Up @@ -153,7 +153,7 @@ bisect_next() {
git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit

# Perform all bisection computation, display and checkout
git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
git bisect--helper --next-all $(git rev-parse --verify -q BISECT_HEAD > /dev/null && echo --no-checkout)
res=$?

# Check if we should exit because bisection is finished
Expand Down
50 changes: 41 additions & 9 deletions refs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ int delete_ref(const char *msg, const char *refname,
old_oid, flags);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Junio C Hamano via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Currently, the cleansing of the reflog message is done by the files
> backend, before the log is written out.  This is sufficient with the
> current code, as that is the only backend that writes reflogs.  But
> new backends can be added that write reflogs, and we'd want the
> resulting log message we would read out of "log -g" the same no
> matter what backend is used.

I _think_ there is no correctness impact, but this slightly changes
the semantics.  We used to take whatever random string from the
caller and at the lowest layer of the files backend, i.e.

>  	strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);
>  	if (msg && *msg)
> -		copy_reflog_msg(&sb, msg);
> +		strbuf_addstr(&sb, msg);

cleansed and added the message to strbuf.

With the updated code, normalize_reflog_message() is defined like so

> +static char *normalize_reflog_message(const char *msg)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (msg && *msg)
> +		copy_reflog_msg(&sb, msg);
> +	return strbuf_detach(&sb, NULL);
> +}

to always return a non-NULL but an empty string, even if the caller
gave us a NULL pointer.  We always pass a non-NULL string around,
and the updated low-level code avoids calling strbuf_addstr()
because msg is not NULL, but *msg is '\0'.

We _might_ be able to optimize by tweaking the normalizer a bit
further, perhaps like so:

    static char *normalize_reflog_message(const char *msg)
    {
            struct strbuf sb = STRBUF_INIT;

            if (msg && *msg)
                    copy_reflog_msg(&sb, msg);
            if (sb.len) {
                    return strbuf_detach(&sb, NULL);
            } else {
                    strbuf_release(&sb);
                    return NULL;
            }
    }

to avoid allocating 1 byte '\0' on the heap by strbuf_detach() when
sb here ends up being truly empty.

But it would be a premature optimization to worry about such a thing
at this point, I think.

Thanks.

}

void copy_reflog_msg(struct strbuf *sb, const char *msg)
static void copy_reflog_msg(struct strbuf *sb, const char *msg)
{
char c;
int wasspace = 1;
Expand All @@ -919,6 +919,15 @@ void copy_reflog_msg(struct strbuf *sb, const char *msg)
strbuf_rtrim(sb);
}

static char *normalize_reflog_message(const char *msg)
{
struct strbuf sb = STRBUF_INIT;

if (msg && *msg)
copy_reflog_msg(&sb, msg);
return strbuf_detach(&sb, NULL);
}

int should_autocreate_reflog(const char *refname)
{
switch (log_all_ref_updates) {
Expand Down Expand Up @@ -1124,7 +1133,7 @@ struct ref_update *ref_transaction_add_update(
oidcpy(&update->new_oid, new_oid);
if (flags & REF_HAVE_OLD)
oidcpy(&update->old_oid, old_oid);
update->msg = xstrdup_or_null(msg);
update->msg = normalize_reflog_message(msg);
return update;
}

Expand Down Expand Up @@ -1983,9 +1992,14 @@ int refs_create_symref(struct ref_store *refs,
const char *refs_heads_master,
const char *logmsg)
{
return refs->be->create_symref(refs, ref_target,
refs_heads_master,
logmsg);
char *msg;
int retval;

msg = normalize_reflog_message(logmsg);
retval = refs->be->create_symref(refs, ref_target, refs_heads_master,
msg);
free(msg);
return retval;
}

int create_symref(const char *ref_target, const char *refs_heads_master,
Expand Down Expand Up @@ -2370,10 +2384,16 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
return refs->be->initial_transaction_commit(refs, transaction, err);
}

int refs_delete_refs(struct ref_store *refs, const char *msg,
int refs_delete_refs(struct ref_store *refs, const char *logmsg,
struct string_list *refnames, unsigned int flags)
{
return refs->be->delete_refs(refs, msg, refnames, flags);
char *msg;
int retval;

msg = normalize_reflog_message(logmsg);
retval = refs->be->delete_refs(refs, msg, refnames, flags);
free(msg);
return retval;
}

int delete_refs(const char *msg, struct string_list *refnames,
Expand All @@ -2385,7 +2405,13 @@ int delete_refs(const char *msg, struct string_list *refnames,
int refs_rename_ref(struct ref_store *refs, const char *oldref,
const char *newref, const char *logmsg)
{
return refs->be->rename_ref(refs, oldref, newref, logmsg);
char *msg;
int retval;

msg = normalize_reflog_message(logmsg);
retval = refs->be->rename_ref(refs, oldref, newref, msg);
free(msg);
return retval;
}

int rename_ref(const char *oldref, const char *newref, const char *logmsg)
Expand All @@ -2396,7 +2422,13 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
int refs_copy_existing_ref(struct ref_store *refs, const char *oldref,
const char *newref, const char *logmsg)
{
return refs->be->copy_ref(refs, oldref, newref, logmsg);
char *msg;
int retval;

msg = normalize_reflog_message(logmsg);
retval = refs->be->copy_ref(refs, oldref, newref, msg);
free(msg);
return retval;
}

int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg)
Expand Down
2 changes: 1 addition & 1 deletion refs/files-backend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1629,7 +1629,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,

strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);
if (msg && *msg)
copy_reflog_msg(&sb, msg);
strbuf_addstr(&sb, msg);
strbuf_addch(&sb, '\n');
if (write_in_full(fd, sb.buf, sb.len) < 0)
ret = -1;
Expand Down
6 changes: 0 additions & 6 deletions refs/refs-internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,6 @@ enum peel_status {
*/
enum peel_status peel_object(const struct object_id *name, struct object_id *oid);

/*
* Copy the reflog message msg to sb while cleaning up the whitespaces.
* Especially, convert LF to space, because reflog file is one line per entry.
*/
void copy_reflog_msg(struct strbuf *sb, const char *msg);

/**
* Information needed for a single ref update. Set new_oid to the new
* value or to null_oid to delete the ref. To check the old value
Expand Down
5 changes: 2 additions & 3 deletions t/lib-t6000.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
: included from 6002 and others

mkdir -p .git/refs/tags

>sed.script

# Answer the sha1 has associated with the tag. The tag must exist under refs/tags
Expand All @@ -26,7 +24,8 @@ save_tag () {
_tag=$1
test -n "$_tag" || error "usage: save_tag tag commit-args ..."
shift 1
"$@" >".git/refs/tags/$_tag"

git update-ref "refs/tags/$_tag" $("$@")

echo "s/$(tag $_tag)/$_tag/g" >sed.script.tmp
cat sed.script >>sed.script.tmp
Expand Down
7 changes: 4 additions & 3 deletions t/t3432-rebase-fast-forward.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,16 @@ test_rebase_same_head_ () {
fi &&
oldhead=\$(git rev-parse HEAD) &&
test_when_finished 'git reset --hard \$oldhead' &&
cp .git/logs/HEAD expect &&
git reflog HEAD >expect &&
git rebase$flag $* >stdout &&
git reflog HEAD >actual &&
if test $what = work
then
old=\$(wc -l <expect) &&
test_line_count '-gt' \$old .git/logs/HEAD
test_line_count '-gt' \$old actual
elif test $what = noop
then
test_cmp expect .git/logs/HEAD
test_cmp expect actual
fi &&
newhead=\$(git rev-parse HEAD) &&
if test $cmp = same
Expand Down