-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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.
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
show-ref: add --symbolic-name option #1684
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,7 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot | |
char *dest = NULL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Mon, Apr 08, 2024 at 05:38:11PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@gmail.com>
[snip]
> @@ -1928,6 +1928,7 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,
>
> const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> const char *refname,
> + char **referent,
> int resolve_flags,
> struct object_id *oid,
> int *flags)
I wonder whether this really should be a `char **`. You don't seem to be
free'ing returned pointers anywhere, so this should probably at least be
`const char **` to indicate that memory is owned by the ref store. But
that would require the ref store to inevitably release it, which may
easily lead to bugs when the caller expects a different lifetime.
So how about we instead make this a `struct strbuf *referent`? This
makes it quite clear who owns the memory. Furthermore, if the caller
wants to resolve multiple refs, it would allow them to reuse the buffer
for better-optimized allocation patterns.
[snip]
> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index 9f9797209a4..4c23b92414a 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -34,12 +34,14 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)
> }
>
> struct ref_entry *create_ref_entry(const char *refname,
> + const char *referent,
> const struct object_id *oid, int flag)
> {
> struct ref_entry *ref;
>
> FLEX_ALLOC_STR(ref, name, refname);
> oidcpy(&ref->u.value.oid, oid);
> + ref->u.value.referent = referent;
> ref->flag = flag;
> return ref;
> }
It's kind of hard to spot the actual changes inbetween all those changes
to callsites. Is it possible to split the patch up such that we have one
patch that changes all the callsites and one patch that does the actual
changes to implement this?
Patrick There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <ps@pks.im> writes:
>> const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>> const char *refname,
>> + char **referent,
>> int resolve_flags,
>> struct object_id *oid,
>> int *flags)
>
> I wonder whether this really should be a `char **`. You don't seem to be
> free'ing returned pointers anywhere, so this should probably at least be
> `const char **` to indicate that memory is owned by the ref store. But
> that would require the ref store to inevitably release it, which may
> easily lead to bugs when the caller expects a different lifetime.
>
> So how about we instead make this a `struct strbuf *referent`? This
> makes it quite clear who owns the memory. Furthermore, if the caller
> wants to resolve multiple refs, it would allow them to reuse the buffer
> for better-optimized allocation patterns.
Or return an allocated piece of memory via "char **". I think an
interface that _requires_ the caller to use strbuf is not nice, if
the caller is not expected to further _edit_ the returned contents
using the strbuf API. If it is likely that the caller would want to
perform further post-processing on the result, an interface based on
strbuf is nice, but I do not think it applies to this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Wed, Apr 10, 2024 at 08:26:13AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> >> const char *refname,
> >> + char **referent,
> >> int resolve_flags,
> >> struct object_id *oid,
> >> int *flags)
> >
> > I wonder whether this really should be a `char **`. You don't seem to be
> > free'ing returned pointers anywhere, so this should probably at least be
> > `const char **` to indicate that memory is owned by the ref store. But
> > that would require the ref store to inevitably release it, which may
> > easily lead to bugs when the caller expects a different lifetime.
> >
> > So how about we instead make this a `struct strbuf *referent`? This
> > makes it quite clear who owns the memory. Furthermore, if the caller
> > wants to resolve multiple refs, it would allow them to reuse the buffer
> > for better-optimized allocation patterns.
>
> Or return an allocated piece of memory via "char **". I think an
> interface that _requires_ the caller to use strbuf is not nice, if
> the caller is not expected to further _edit_ the returned contents
> using the strbuf API. If it is likely that the caller would want to
> perform further post-processing on the result, an interface based on
> strbuf is nice, but I do not think it applies to this case.
When iterating through refs this would incur one allocation per ref
though, whereas using a `struct strbuf` would only require a single one.
Patrick |
||
struct strbuf sb = STRBUF_INIT; | ||
struct ref_store *store = get_main_ref_store(repo); | ||
const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL, | ||
const char *refname = refs_resolve_ref_unsafe(store, "HEAD", NULL, 0, NULL, | ||
NULL); | ||
|
||
if (!refname) | ||
|
There was a problem hiding this comment.
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):
There was a problem hiding this comment.
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, John Cai wrote (reply to this):