| Message ID | 59b91a206d9d7bf64825cb48c747730e28b10a79.1598662525.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix for git checkout @{u} (non-local) then git status | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > + > + /* > + * If ^{upstream} or ^{push} (or equivalent) is requested, and the > + * branch in question does not have such a reference, return -1 instead > + * of die()-ing. > + */ > + unsigned nonfatal_dangling_mark : 1; Micronit; I would have avoided "or equivalent" as the point of parenthetical comment was not to say these two modifiers upstream and push (and other forms that spell differently but invokes exactly one of these two features) are special, but to say that these two are merely examples, and any other ^{modifiers} we have or we will add in the future would honor this bit. Perhaps "(and the like)"? > }; > int repo_interpret_branch_name(struct repository *r, > const char *str, int len, > diff --git a/refs.c b/refs.c > index cf09cd039f..b6f1a2f452 100644 > --- a/refs.c > +++ b/refs.c > @@ -598,10 +598,13 @@ const char *git_default_branch_name(void) > * to name a branch. > */ > static char *substitute_branch_name(struct repository *r, > - const char **string, int *len) > + const char **string, int *len, > + int nonfatal_dangling_mark) > { > struct strbuf buf = STRBUF_INIT; > - struct interpret_branch_name_options options = { 0 } ; > + struct interpret_branch_name_options options = { > + .nonfatal_dangling_mark = nonfatal_dangling_mark > + }; > int ret = repo_interpret_branch_name(r, *string, *len, &buf, &options); > > if (ret == *len) { > @@ -615,9 +618,10 @@ static char *substitute_branch_name(struct repository *r, > } > > int repo_dwim_ref(struct repository *r, const char *str, int len, > - struct object_id *oid, char **ref) > + struct object_id *oid, char **ref, int nonfatal_dangling_mark) > { > - char *last_branch = substitute_branch_name(r, &str, &len); > + char *last_branch = substitute_branch_name(r, &str, &len, > + nonfatal_dangling_mark); > int refs_found = expand_ref(r, str, len, oid, ref); > free(last_branch); > return refs_found; > @@ -625,7 +629,7 @@ int repo_dwim_ref(struct repository *r, const char *str, int len, > > int dwim_ref(const char *str, int len, struct object_id *oid, char **ref) > { > - return repo_dwim_ref(the_repository, str, len, oid, ref); > + return repo_dwim_ref(the_repository, str, len, oid, ref, 0); > } > > int expand_ref(struct repository *repo, const char *str, int len, > @@ -666,7 +670,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len, > struct object_id *oid, char **log) > { > struct ref_store *refs = get_main_ref_store(r); > - char *last_branch = substitute_branch_name(r, &str, &len); > + char *last_branch = substitute_branch_name(r, &str, &len, 0); > const char **p; > int logs_found = 0; > struct strbuf path = STRBUF_INIT; Among these callers that reach substitute_branch_name(), how were those that can specify the new bit chosen? For example, what is the reasoning behind making dwim_ref() unable to ask the "do so gently" variant, while allowing repo_dwim_ref() to? I am NOT necessarily saying these two functions MUST be able to access the same set of features and the only difference between them MUST be kept to the current "repo_* variant can work on an arbitrary repository, while the variant without repo_* would work on the primary repository only". As long as there is a good reason to make their power diverge, it is OK---I just do not see why and I'd like to know. The same question about not allowing the gentler variant while drimming the reflog. Thanks.
> Jonathan Tan <jonathantanmy@google.com> writes: > > > + > > + /* > > + * If ^{upstream} or ^{push} (or equivalent) is requested, and the > > + * branch in question does not have such a reference, return -1 instead > > + * of die()-ing. > > + */ > > + unsigned nonfatal_dangling_mark : 1; > > Micronit; I would have avoided "or equivalent" as the point of > parenthetical comment was not to say these two modifiers upstream > and push (and other forms that spell differently but invokes exactly > one of these two features) are special, but to say that these two > are merely examples, and any other ^{modifiers} we have or we will > add in the future would honor this bit. Perhaps "(and the like)"? "(and the like)" sounds good. > Among these callers that reach substitute_branch_name(), how were > those that can specify the new bit chosen? I just did the minimal change to fix the bug in the test. > For example, what is the reasoning behind making dwim_ref() unable > to ask the "do so gently" variant, while allowing repo_dwim_ref() > to? > > I am NOT necessarily saying these two functions MUST be able to > access the same set of features and the only difference between them > MUST be kept to the current "repo_* variant can work on an arbitrary > repository, while the variant without repo_* would work on the > primary repository only". As long as there is a good reason to make > their power diverge, it is OK---I just do not see why and I'd like > to know. Maybe add the following to the end of the last paragraph of the commit message: (dwim_ref() is unchanged because I expect more and more code to use repo_dwim_ref(), and to reduce the size of the diff.) If you prefer not to make this change locally, let me know and I'll resend one with the updated commit message and the "(and the like)" change above. > The same question about not allowing the gentler variant while > drimming the reflog. Same as above - I only did the minimal change to fix the bug. Admittedly, if a reflog-accessing command could fail in the same way (dangling mark), we would need to fix repo_dwim_log() and then we could simplify substitute_branch_name() to not take the nonfatal_dangling_mark parameter (since all dangling marks would now be nonfatal), but I haven't looked beyond this.
Jonathan Tan <jonathantanmy@google.com> writes: >> For example, what is the reasoning behind making dwim_ref() unable >> to ask the "do so gently" variant, while allowing repo_dwim_ref() >> to? >> >> I am NOT necessarily saying these two functions MUST be able to >> access the same set of features and the only difference between them >> MUST be kept to the current "repo_* variant can work on an arbitrary >> repository, while the variant without repo_* would work on the >> primary repository only". As long as there is a good reason to make >> their power diverge, it is OK---I just do not see why and I'd like >> to know. > > Maybe add the following to the end of the last paragraph of the commit > message: > > (dwim_ref() is unchanged because I expect more and more code to use > repo_dwim_ref(), and to reduce the size of the diff.) If that is the reasoning, I totally disagree. We may be staring problems in submodules too long to think everybody should use the variant with repo_ prefix, but a single repository operations will continue to exist, and when you know you only need to access the current repository, not having to use the function with longer name without having to pass an extra parameter is a plus. I even wonder why dwim_ref() is not defined like so in a header: #define dwim_ref(s, l, o, r) \ repo_dwim_ref(the_repository, (s), (l), (o), (r)) Which would force a change like the one we are discussing to keep them in parallel and keep the promise that only difference between the two is that dwim_ref() works with the_repository and the other can take an arbitrary repository. Perhaps that can be a preliminary clean-up patch before these two patches ;-) Doing so would mean that coders have to remember one fewer thing than "dwim_ref(), not only cannot take a repository pointer, cannot be told to report error gently". The worst part is that we know that the difference will GROW, because the purpose of adding the extra option argument to the API is exactly to make it easy to do so. Reducing the size of the diff is a good justification only when the end result is the same. If it burdens future developers more, that is "I write less at the expense of forcing others to write more", which is not quite the same thing. Thanks.
diff --git a/cache.h b/cache.h index 4f16a57ba4..cee8aa5dc3 100644 --- a/cache.h +++ b/cache.h @@ -1569,6 +1569,13 @@ struct interpret_branch_name_options { * allowed, even ones to refs outside of those namespaces. */ unsigned allowed; + + /* + * If ^{upstream} or ^{push} (or equivalent) is requested, and the + * branch in question does not have such a reference, return -1 instead + * of die()-ing. + */ + unsigned nonfatal_dangling_mark : 1; }; int repo_interpret_branch_name(struct repository *r, const char *str, int len, diff --git a/refs.c b/refs.c index cf09cd039f..b6f1a2f452 100644 --- a/refs.c +++ b/refs.c @@ -598,10 +598,13 @@ const char *git_default_branch_name(void) * to name a branch. */ static char *substitute_branch_name(struct repository *r, - const char **string, int *len) + const char **string, int *len, + int nonfatal_dangling_mark) { struct strbuf buf = STRBUF_INIT; - struct interpret_branch_name_options options = { 0 } ; + struct interpret_branch_name_options options = { + .nonfatal_dangling_mark = nonfatal_dangling_mark + }; int ret = repo_interpret_branch_name(r, *string, *len, &buf, &options); if (ret == *len) { @@ -615,9 +618,10 @@ static char *substitute_branch_name(struct repository *r, } int repo_dwim_ref(struct repository *r, const char *str, int len, - struct object_id *oid, char **ref) + struct object_id *oid, char **ref, int nonfatal_dangling_mark) { - char *last_branch = substitute_branch_name(r, &str, &len); + char *last_branch = substitute_branch_name(r, &str, &len, + nonfatal_dangling_mark); int refs_found = expand_ref(r, str, len, oid, ref); free(last_branch); return refs_found; @@ -625,7 +629,7 @@ int repo_dwim_ref(struct repository *r, const char *str, int len, int dwim_ref(const char *str, int len, struct object_id *oid, char **ref) { - return repo_dwim_ref(the_repository, str, len, oid, ref); + return repo_dwim_ref(the_repository, str, len, oid, ref, 0); } int expand_ref(struct repository *repo, const char *str, int len, @@ -666,7 +670,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_id *oid, char **log) { struct ref_store *refs = get_main_ref_store(r); - char *last_branch = substitute_branch_name(r, &str, &len); + char *last_branch = substitute_branch_name(r, &str, &len, 0); const char **p; int logs_found = 0; struct strbuf path = STRBUF_INIT; diff --git a/refs.h b/refs.h index 29e28124cd..b94a7fd4f7 100644 --- a/refs.h +++ b/refs.h @@ -149,7 +149,8 @@ struct strvec; void expand_ref_prefix(struct strvec *prefixes, const char *prefix); int expand_ref(struct repository *r, const char *str, int len, struct object_id *oid, char **ref); -int repo_dwim_ref(struct repository *r, const char *str, int len, struct object_id *oid, char **ref); +int repo_dwim_ref(struct repository *r, const char *str, int len, + struct object_id *oid, char **ref, int nonfatal_dangling_mark); int repo_dwim_log(struct repository *r, const char *str, int len, struct object_id *oid, char **ref); int dwim_ref(const char *str, int len, struct object_id *oid, char **ref); int dwim_log(const char *str, int len, struct object_id *oid, char **ref); diff --git a/sha1-name.c b/sha1-name.c index a7a9de66c4..0b23b86ceb 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -809,7 +809,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len, if (len == r->hash_algo->hexsz && !get_oid_hex(str, oid)) { if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) { - refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref); + refs_found = repo_dwim_ref(r, str, len, &tmp_oid, &real_ref, 0); if (refs_found > 0) { warning(warn_msg, len, str); if (advice_object_name_warning) @@ -860,11 +860,11 @@ static int get_oid_basic(struct repository *r, const char *str, int len, if (!len && reflog_len) /* allow "@{...}" to mean the current branch reflog */ - refs_found = repo_dwim_ref(r, "HEAD", 4, oid, &real_ref); + refs_found = repo_dwim_ref(r, "HEAD", 4, oid, &real_ref, 0); else if (reflog_len) refs_found = repo_dwim_log(r, str, len, oid, &real_ref); else - refs_found = repo_dwim_ref(r, str, len, oid, &real_ref); + refs_found = repo_dwim_ref(r, str, len, oid, &real_ref, 0); if (!refs_found) return -1; @@ -1496,8 +1496,14 @@ static int interpret_branch_mark(struct repository *r, branch = branch_get(NULL); value = get_data(branch, &err); - if (!value) - die("%s", err.buf); + if (!value) { + if (options->nonfatal_dangling_mark) { + strbuf_release(&err); + return -1; + } else { + die("%s", err.buf); + } + } if (!branch_interpret_allowed(value, options->allowed)) return -1; diff --git a/t/t7508-status.sh b/t/t7508-status.sh index e81759319f..45e1f6ff68 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -846,6 +846,18 @@ test_expect_success 'status refreshes the index' ' test_cmp expect output ' +test_expect_success 'status shows detached HEAD properly after checking out non-local upstream branch' ' + test_when_finished rm -rf upstream downstream actual && + + test_create_repo upstream && + test_commit -C upstream foo && + + git clone upstream downstream && + git -C downstream checkout @{u} && + git -C downstream status >actual && + test_i18ngrep "HEAD detached at [0-9a-f]\\+" actual +' + test_expect_success 'setup status submodule summary' ' test_create_repo sm && ( cd sm && diff --git a/wt-status.c b/wt-status.c index 7ce58b8aae..ae16faf40d 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1569,7 +1569,7 @@ static void wt_status_get_detached_from(struct repository *r, return; } - if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 && + if (repo_dwim_ref(the_repository, cb.buf.buf, cb.buf.len, &oid, &ref, 1) == 1 && /* sha1 is a commit? match without further lookup */ (oideq(&cb.noid, &oid) || /* perhaps sha1 is a tag, try to dereference to a commit */
When a user checks out the upstream branch of HEAD, the upstream branch not being a local branch, and then runs "git status", like this: git clone $URL client cd client git checkout @{u} git status no status is printed, but instead an error message: fatal: HEAD does not point to a branch (This error message when running "git branch" persists even after checking out other things - it only stops after checking out a branch.) This is because "git status" reads the reflog when determining the "HEAD detached" message, and thus attempts to DWIM "@{u}", but that doesn't work because HEAD no longer points to a branch. Therefore, when calculating the status of a worktree, tolerate dangling marks. This is done by adding an additional parameter to repo_dwim_ref(). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- cache.h | 7 +++++++ refs.c | 16 ++++++++++------ refs.h | 3 ++- sha1-name.c | 16 +++++++++++----- t/t7508-status.sh | 12 ++++++++++++ wt-status.c | 2 +- 6 files changed, 43 insertions(+), 13 deletions(-)