| Message ID | 20241121225757.3877852-4-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | set-head/fetch remote/HEAD updates | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: [snip] > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index 38eb14d591..1809e3426a 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -830,10 +830,12 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, > return ret; > > ret = reftable_stack_read_ref(stack, refname, &ref); > - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) > + if (ret) > + ret = -1; > + else if (ref.value_type == REFTABLE_REF_SYMREF) > strbuf_addstr(referent, ref.value.symref); > - else > - ret = -1; > + else > + ret = NOT_A_SYMREF; > I was building my series on top of this, and noticed whitespace issues here. A simple way to check your series is to run: $ git log --check --pretty=format:"---% h% s" > reftable_ref_record_release(&ref); > return ret; > -- > 2.47.0.298.g52a96ec17b
On Fri Nov 22, 2024 at 11:37, karthik nayak <karthik.188@gmail.com> wrote: > Bence Ferdinandy <bence@ferdinandy.com> writes: > > [snip] > >> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >> index 38eb14d591..1809e3426a 100644 >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -830,10 +830,12 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, >> return ret; >> >> ret = reftable_stack_read_ref(stack, refname, &ref); >> - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) >> + if (ret) >> + ret = -1; >> + else if (ref.value_type == REFTABLE_REF_SYMREF) >> strbuf_addstr(referent, ref.value.symref); >> - else >> - ret = -1; >> + else >> + ret = NOT_A_SYMREF; >> > > I was building my series on top of this, and noticed whitespace issues > here. A simple way to check your series is to run: > > $ git log --check --pretty=format:"---% h% s" I ran this on v15 and it didn't produce any output. I read what --check is in the manpages, although the format is a bit cryptic for me. What does that do exactly? Anyhow if there was no output for v15 I should be fine, right? > >> reftable_ref_record_release(&ref); >> return ret; >> -- >> 2.47.0.298.g52a96ec17b
On Fri Nov 22, 2024 at 11:53, Bence Ferdinandy <bence@ferdinandy.com> wrote: > > On Fri Nov 22, 2024 at 11:37, karthik nayak <karthik.188@gmail.com> wrote: >> Bence Ferdinandy <bence@ferdinandy.com> writes: >> >> [snip] >> >>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >>> index 38eb14d591..1809e3426a 100644 >>> --- a/refs/reftable-backend.c >>> +++ b/refs/reftable-backend.c >>> @@ -830,10 +830,12 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, >>> return ret; >>> >>> ret = reftable_stack_read_ref(stack, refname, &ref); >>> - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) >>> + if (ret) >>> + ret = -1; >>> + else if (ref.value_type == REFTABLE_REF_SYMREF) >>> strbuf_addstr(referent, ref.value.symref); >>> - else >>> - ret = -1; >>> + else >>> + ret = NOT_A_SYMREF; >>> >> >> I was building my series on top of this, and noticed whitespace issues >> here. A simple way to check your series is to run: >> >> $ git log --check --pretty=format:"---% h% s" > > I ran this on v15 and it didn't produce any output. I read what --check is in > the manpages, although the format is a bit cryptic for me. What does that do > exactly? > > Anyhow if there was no output for v15 I should be fine, right? But then again: this patch is exactly like v14 so I'm not so sure about that ...
On Fri Nov 22, 2024 at 11:37, karthik nayak <karthik.188@gmail.com> wrote: > Bence Ferdinandy <bence@ferdinandy.com> writes: > > [snip] > >> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >> index 38eb14d591..1809e3426a 100644 >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -830,10 +830,12 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, >> return ret; >> >> ret = reftable_stack_read_ref(stack, refname, &ref); >> - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) >> + if (ret) >> + ret = -1; >> + else if (ref.value_type == REFTABLE_REF_SYMREF) >> strbuf_addstr(referent, ref.value.symref); >> - else >> - ret = -1; >> + else >> + ret = NOT_A_SYMREF; >> > > I was building my series on top of this, and noticed whitespace issues > here. A simple way to check your series is to run: Found it, thanks for catching, I had tabs and whitespaces mixed. git log --check didn't find a problem with that though. > >> reftable_ref_record_release(&ref); >> return ret; >> -- >> 2.47.0.298.g52a96ec17b
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > On Fri Nov 22, 2024 at 11:37, karthik nayak <karthik.188@gmail.com> wrote: >> Bence Ferdinandy <bence@ferdinandy.com> writes: >> >> [snip] >> >>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >>> index 38eb14d591..1809e3426a 100644 >>> --- a/refs/reftable-backend.c >>> +++ b/refs/reftable-backend.c >>> @@ -830,10 +830,12 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, >>> return ret; >>> >>> ret = reftable_stack_read_ref(stack, refname, &ref); >>> - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) >>> + if (ret) >>> + ret = -1; >>> + else if (ref.value_type == REFTABLE_REF_SYMREF) >>> strbuf_addstr(referent, ref.value.symref); >>> - else >>> - ret = -1; >>> + else >>> + ret = NOT_A_SYMREF; >>> >> >> I was building my series on top of this, and noticed whitespace issues >> here. A simple way to check your series is to run: >> >> $ git log --check --pretty=format:"---% h% s" > > I ran this on v15 and it didn't produce any output. Did you already post v15? I only see v14 > I read what --check is in > the manpages, although the format is a bit cryptic for me. What does that do > exactly? > It ensures that commits don't have conflict markers and that there are no trailing whitespaces and spaces followed by tabs by default. Also this is included in the CI checks (see ci/check-whitespace.sh), so if you use either GitLab or GitHub you should see these shown as errors on the CI. You'll have to raise a MR/PR to trigger the CI I believe. On a sidenote, do you think we should modify the manpage? I found it comprehensible, but would be nice to clarify anything cryptic. > Anyhow if there was no output for v15 I should be fine, right? > At the least you should see `git log`'s output, but if there are issues they should be shown inline. So when you say 'no output' do you mean you see absolutely no output? >> >>> reftable_ref_record_release(&ref); >>> return ret; >>> -- >>> 2.47.0.298.g52a96ec17b > > > > > -- > bence.ferdinandy.com
On Fri Nov 22, 2024 at 12:30, karthik nayak <karthik.188@gmail.com> wrote: > "Bence Ferdinandy" <bence@ferdinandy.com> writes: > >> On Fri Nov 22, 2024 at 11:37, karthik nayak <karthik.188@gmail.com> wrote: >>> Bence Ferdinandy <bence@ferdinandy.com> writes: >>> >>> [snip] >>> >>>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >>>> index 38eb14d591..1809e3426a 100644 >>>> --- a/refs/reftable-backend.c >>>> +++ b/refs/reftable-backend.c >>>> @@ -830,10 +830,12 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, >>>> return ret; >>>> >>>> ret = reftable_stack_read_ref(stack, refname, &ref); >>>> - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) >>>> + if (ret) >>>> + ret = -1; >>>> + else if (ref.value_type == REFTABLE_REF_SYMREF) >>>> strbuf_addstr(referent, ref.value.symref); >>>> - else >>>> - ret = -1; >>>> + else >>>> + ret = NOT_A_SYMREF; >>>> >>> >>> I was building my series on top of this, and noticed whitespace issues >>> here. A simple way to check your series is to run: >>> >>> $ git log --check --pretty=format:"---% h% s" >> >> I ran this on v15 and it didn't produce any output. > > Did you already post v15? I only see v14 Not yet, but I'll be sending it in a pinch. > >> I read what --check is in >> the manpages, although the format is a bit cryptic for me. What does that do >> exactly? >> > > It ensures that commits don't have conflict markers and that there are > no trailing whitespaces and spaces followed by tabs by default. > > Also this is included in the CI checks (see ci/check-whitespace.sh), so > if you use either GitLab or GitHub you should see these shown as errors > on the CI. You'll have to raise a MR/PR to trigger the CI I believe. I've been running the CI by pushing to a fork since Taylor first caught an error I didn't see locally and it never flagged. Now that you mention it, I'll also add log --check to my CI-s. > > On a sidenote, do you think we should modify the manpage? I found it > comprehensible, but would be nice to clarify anything cryptic. No, --check was quite clear, the `--pretty=format:"---% h% s"` part was what was cryptic. > >> Anyhow if there was no output for v15 I should be fine, right? >> > > At the least you should see `git log`'s output, but if there are issues > they should be shown inline. So when you say 'no output' do you mean you > see absolutely no output? Absolutely no output: https://asciinema.org/a/lsqp4e1bNst6cFWw9M2jX1IqC But I figured out why: the whitespace and the tabs were not mixed on the line, just across lines. As I read it, that is not an error to have tabs on one line and spaces on the next. Anyhow that should be now cleared up, thanks. Gotta say, I was expecting to learn about internals doing this, but I also ended up picking up a couple of usage things as well, like --range-diff for format patch and such. Thanks, Bence
"Bence Ferdinandy" <bence@ferdinandy.com> writes: >> At the least you should see `git log`'s output, but if there are issues >> they should be shown inline. So when you say 'no output' do you mean you >> see absolutely no output? > > Absolutely no output: > https://asciinema.org/a/lsqp4e1bNst6cFWw9M2jX1IqC > > But I figured out why: the whitespace and the tabs were not mixed on the line, > just across lines. As I read it, that is not an error to have tabs on one line > and spaces on the next. Our .gitattribute starts like so: * whitespace=!indent,trail,space *.[ch] whitespace=indent,trail,space diff=cpp so, unless otherwise specified, we frown upon trailing whitespace and space before tab and indenting with non tab is permitted, but C source and header files have further care about "indent" (short for "indent-with-non-tab". So mixed or not, if you indented with spaces and not tabs, that would be noticed. > Anyhow that should be now cleared up, thanks. Gotta say, I was expecting to > learn about internals doing this, but I also ended up picking up a couple of > usage things as well, like --range-diff for format patch and such. I usually have "--whitespace=fix" so if you did "git log" on the commits I made out of your patches, it is not surprising if your "log --check" was silent. I re-applied your v14 with "git am -s --whitespace=nowarn" and here is what I saw. commit 75a6a3e6597d5f3959eb269122e8c5f4e4baac0e Author: Bence Ferdinandy <bence@ferdinandy.com> Date: Thu Nov 21 23:55:03 2024 +0100 refs: standardize output of refs_read_symbolic_ref When the symbolic reference we want to read with refs_read_symbolic_ref is actually not a symbolic reference, the files and the reftable backends return different values (1 and -1 respectively). Standardize the returned values so that 0 is success, -1 is a generic error and -2 is that the reference was actually non-symbolic. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> refs/reftable-backend.c:833: indent with spaces. + if (ret) refs/reftable-backend.c:834: indent with spaces. + ret = -1; refs/reftable-backend.c:835: indent with spaces. + else if (ref.value_type == REFTABLE_REF_SYMREF) refs/reftable-backend.c:837: indent with spaces. + else refs/reftable-backend.c:838: indent with spaces. + ret = NOT_A_SYMREF;
sorry about the other thread, I saw "extend whitespace checks" and I thought based on what Kartik wrote that `log --check` should have caught it, which is why I thought it might be appropriate there. On Mon Nov 25, 2024 at 03:56, Junio C Hamano <gitster@pobox.com> wrote: > "Bence Ferdinandy" <bence@ferdinandy.com> writes: > >>> At the least you should see `git log`'s output, but if there are issues >>> they should be shown inline. So when you say 'no output' do you mean you >>> see absolutely no output? >> >> Absolutely no output: >> https://asciinema.org/a/lsqp4e1bNst6cFWw9M2jX1IqC >> >> But I figured out why: the whitespace and the tabs were not mixed on the line, >> just across lines. As I read it, that is not an error to have tabs on one line >> and spaces on the next. > > Our .gitattribute starts like so: > > * whitespace=!indent,trail,space > *.[ch] whitespace=indent,trail,space diff=cpp > > so, unless otherwise specified, we frown upon trailing whitespace > and space before tab and indenting with non tab is permitted, but C > source and header files have further care about "indent" (short for > "indent-with-non-tab". > > So mixed or not, if you indented with spaces and not tabs, that > would be noticed. So `git log --check --pretty=format:"---% h% s"` was _not_ supposed to catch this? I ran the CI with this patch again: https://github.com/ferdinandyb/git/actions/runs/12031250376 and it's all green, so wherever this _should_ have been caught: it didn't work. > >> Anyhow that should be now cleared up, thanks. Gotta say, I was expecting to >> learn about internals doing this, but I also ended up picking up a couple of >> usage things as well, like --range-diff for format patch and such. > > I usually have "--whitespace=fix" so if you did "git log" on the > commits I made out of your patches, it is not surprising if your > "log --check" was silent. > > I re-applied your v14 with "git am -s --whitespace=nowarn" and here > is what I saw. > > commit 75a6a3e6597d5f3959eb269122e8c5f4e4baac0e > Author: Bence Ferdinandy <bence@ferdinandy.com> > Date: Thu Nov 21 23:55:03 2024 +0100 > > refs: standardize output of refs_read_symbolic_ref > > When the symbolic reference we want to read with refs_read_symbolic_ref > is actually not a symbolic reference, the files and the reftable > backends return different values (1 and -1 respectively). Standardize > the returned values so that 0 is success, -1 is a generic error and -2 > is that the reference was actually non-symbolic. > > Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > > refs/reftable-backend.c:833: indent with spaces. > + if (ret) > refs/reftable-backend.c:834: indent with spaces. > + ret = -1; > refs/reftable-backend.c:835: indent with spaces. > + else if (ref.value_type == REFTABLE_REF_SYMREF) > refs/reftable-backend.c:837: indent with spaces. > + else > refs/reftable-backend.c:838: indent with spaces. > + ret = NOT_A_SYMREF; So this did find it. Maybe something is misconfigured in the CI? Best, Bence
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > sorry about the other thread, I saw "extend whitespace checks" and I thought > based on what Kartik wrote that `log --check` should have caught it, which is > why I thought it might be appropriate there. > With v14, running `git log --check --pretty=format:"---% h% s" master..` gives me: --- f73b2c577b fetch set_head: handle mirrored bare repositories --- c0c84fb7f9 fetch: set remote/HEAD if it does not exist --- c366911074 refs: add create_only option to refs_update_symref_extended --- c47704d4df refs: add TRANSACTION_CREATE_EXISTS error --- 22e7a9bcae remote set-head: better output for --auto --- 25d9944eb2 remote set-head: refactor for readability --- 01fe46c842 refs: atomically record overwritten ref in update_symref --- fed56bc6cc refs: standardize output of refs_read_symbolic_ref refs/reftable-backend.c:833: indent with spaces. + if (ret) refs/reftable-backend.c:834: indent with spaces. + ret = -1; refs/reftable-backend.c:835: indent with spaces. + else if (ref.value_type == REFTABLE_REF_SYMREF) refs/reftable-backend.c:837: indent with spaces. + else refs/reftable-backend.c:838: indent with spaces. + ret = NOT_A_SYMREF; --- d5534d6c67 t/t5505-remote: test failure of set-head --- a77b3b7774 t/t5505-remote: set default branch to main > On Mon Nov 25, 2024 at 03:56, Junio C Hamano <gitster@pobox.com> wrote: >> "Bence Ferdinandy" <bence@ferdinandy.com> writes: >> >>>> At the least you should see `git log`'s output, but if there are issues >>>> they should be shown inline. So when you say 'no output' do you mean you >>>> see absolutely no output? >>> >>> Absolutely no output: >>> https://asciinema.org/a/lsqp4e1bNst6cFWw9M2jX1IqC >>> >>> But I figured out why: the whitespace and the tabs were not mixed on the line, >>> just across lines. As I read it, that is not an error to have tabs on one line >>> and spaces on the next. >> >> Our .gitattribute starts like so: >> >> * whitespace=!indent,trail,space >> *.[ch] whitespace=indent,trail,space diff=cpp >> >> so, unless otherwise specified, we frown upon trailing whitespace >> and space before tab and indenting with non tab is permitted, but C >> source and header files have further care about "indent" (short for >> "indent-with-non-tab". >> >> So mixed or not, if you indented with spaces and not tabs, that >> would be noticed. > > So `git log --check --pretty=format:"---% h% s"` was _not_ supposed to catch > this? > > I ran the CI with this patch again: > https://github.com/ferdinandyb/git/actions/runs/12031250376 > > and it's all green, so wherever this _should_ have been caught: it didn't work. I'm not an expert in GitHub actions, but if you look at `.github/workflows/check-whitespace.yml`, it says it is only triggered for `pull_request`. Did you raise a pull request? From your link, I don't see the whitespace job being triggered, so `it didn't work` would be inaccurate, since it didn't even trigger the job ;) [snip]
karthik nayak <karthik.188@gmail.com> writes: > With v14, running `git log --check --pretty=format:"---% h% s" master..` > gives me: > ... > --- fed56bc6cc refs: standardize output of refs_read_symbolic_ref > refs/reftable-backend.c:833: indent with spaces. > + if (ret) > refs/reftable-backend.c:834: indent with spaces. > + ret = -1; > ... Thanks, the above matches what I saw in my message Bence responded to (to which you are responding). I was unsure if/how the status of each "diff --check" is propagated up to the driving "git log", but the job is reading from the output of the command and not using the exit status of "git log --check", so even if "git log --check" did not signal a check failure with its status, it did not matter ;-) A quick local check says "git log --check" does exit with 2 if there is an offending commit in the range, so we should be OK. By the way, I appreciate the cuteness of "% s" but I do not see the point of "% h", as I do not think of a situation where '%h' can ever be empty. It seems that this was carried from the first day when GitHub CI learned the whitespace checks 32c83afc (ci: github action - add check for whitespace errors, 2020-09-22).
On Tue Nov 26, 2024 at 17:53, karthik nayak <karthik.188@gmail.com> wrote: > "Bence Ferdinandy" <bence@ferdinandy.com> writes: > >> sorry about the other thread, I saw "extend whitespace checks" and I thought >> based on what Kartik wrote that `log --check` should have caught it, which is >> why I thought it might be appropriate there. >> > > With v14, running `git log --check --pretty=format:"---% h% s" master..` > gives me: > > --- f73b2c577b fetch set_head: handle mirrored bare repositories > > --- c0c84fb7f9 fetch: set remote/HEAD if it does not exist > > --- c366911074 refs: add create_only option to refs_update_symref_extended > > --- c47704d4df refs: add TRANSACTION_CREATE_EXISTS error > > --- 22e7a9bcae remote set-head: better output for --auto > > --- 25d9944eb2 remote set-head: refactor for readability > > --- 01fe46c842 refs: atomically record overwritten ref in update_symref > > --- fed56bc6cc refs: standardize output of refs_read_symbolic_ref > refs/reftable-backend.c:833: indent with spaces. > + if (ret) > refs/reftable-backend.c:834: indent with spaces. > + ret = -1; > refs/reftable-backend.c:835: indent with spaces. > + else if (ref.value_type == REFTABLE_REF_SYMREF) > refs/reftable-backend.c:837: indent with spaces. > + else > refs/reftable-backend.c:838: indent with spaces. > + ret = NOT_A_SYMREF; > > --- d5534d6c67 t/t5505-remote: test failure of set-head > > --- a77b3b7774 t/t5505-remote: set default branch to main So at least now I know why I'm confused :) I copied your exact command (well, with upstream/master..), and I still have zero output. Since .gitattributes is committed to the repository, what else could be the difference? [snip] >> >> So `git log --check --pretty=format:"---% h% s"` was _not_ supposed to catch >> this? >> >> I ran the CI with this patch again: >> https://github.com/ferdinandyb/git/actions/runs/12031250376 >> >> and it's all green, so wherever this _should_ have been caught: it didn't work. > > I'm not an expert in GitHub actions, but if you look at > `.github/workflows/check-whitespace.yml`, it says it is only triggered > for `pull_request`. Did you raise a pull request? From your link, I > don't see the whitespace job being triggered, so `it didn't work` would > be inaccurate, since it didn't even trigger the job ;) Well that make sense, the commit introducing it also talks about pull requests. On the other hand: why does this run only on a pull request? The main.yml runs on both pull requests and pushes and since the project doesn't really use pull requests, it would make more sense to also run this on push? The check-style.yml is similar.
On Tue Nov 26, 2024 at 21:02, Junio C Hamano <gitster@pobox.com> wrote: > karthik nayak <karthik.188@gmail.com> writes: > >> With v14, running `git log --check --pretty=format:"---% h% s" master..` >> gives me: >> ... >> --- fed56bc6cc refs: standardize output of refs_read_symbolic_ref >> refs/reftable-backend.c:833: indent with spaces. >> + if (ret) >> refs/reftable-backend.c:834: indent with spaces. >> + ret = -1; >> ... > > Thanks, the above matches what I saw in my message Bence responded > to (to which you are responding). I was unsure if/how the status of > each "diff --check" is propagated up to the driving "git log", but > the job is reading from the output of the command and not using the > exit status of "git log --check", so even if "git log --check" did > not signal a check failure with its status, it did not matter ;-) A > quick local check says "git log --check" does exit with 2 if there > is an offending commit in the range, so we should be OK. Ok, so `git diff --check master` at least does produce the output for me, and now that you mentioned the exit code, I checked that even though I get zero output from git log --check, the exit code is indeed 2. So now I just don't get why I don't see any output ...
"Bence Ferdinandy" <bence@ferdinandy.com> writes: > Ok, so `git diff --check master` at least does produce the output for me, and > now that you mentioned the exit code, I checked that even though I get zero > output from git log --check, the exit code is indeed 2. So now I just don't get > why I don't see any output ... I was about to say "perhaps you are accidentally giving a wrong revision range, like 'git checkout master && git log --check master..'?" but then the command should exit with 0 for an empty range, so that is not it. Do let us know when you find out why ;-) Thanks.
diff --git a/refs.h b/refs.h index 108dfc93b3..22c2b45b8b 100644 --- a/refs.h +++ b/refs.h @@ -83,6 +83,17 @@ int refs_read_ref_full(struct ref_store *refs, const char *refname, int refs_read_ref(struct ref_store *refs, const char *refname, struct object_id *oid); +#define NOT_A_SYMREF -2 + +/* + * Read the symbolic ref named "refname" and write its immediate referent into + * the provided buffer. Referent is left empty if "refname" is not a symbolic + * ref. It does not resolve the symbolic reference recursively in case the + * target is also a symbolic ref. + * + * Returns 0 on success, -2 if the "refname" is not a symbolic ref, + * -1 otherwise. + */ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname, struct strbuf *referent); diff --git a/refs/files-backend.c b/refs/files-backend.c index 0824c0b8a9..4cc43c32f2 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -596,10 +596,9 @@ static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refn unsigned int type; ret = read_ref_internal(ref_store, refname, &oid, referent, &type, &failure_errno, 1); - if (ret) - return ret; - - return !(type & REF_ISSYMREF); + if (!ret && !(type & REF_ISSYMREF)) + return NOT_A_SYMREF; + return ret; } int parse_loose_ref_contents(const struct git_hash_algo *algop, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 2313c830d8..1399fee61c 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -673,6 +673,11 @@ struct ref_storage_be { ref_iterator_begin_fn *iterator_begin; read_raw_ref_fn *read_raw_ref; + + /* + * Please refer to `refs_read_symbolic_ref()` for the expected + * behaviour. + */ read_symbolic_ref_fn *read_symbolic_ref; reflog_iterator_begin_fn *reflog_iterator_begin; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 38eb14d591..1809e3426a 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -830,10 +830,12 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store, return ret; ret = reftable_stack_read_ref(stack, refname, &ref); - if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF) + if (ret) + ret = -1; + else if (ref.value_type == REFTABLE_REF_SYMREF) strbuf_addstr(referent, ref.value.symref); - else - ret = -1; + else + ret = NOT_A_SYMREF; reftable_ref_record_release(&ref); return ret;
When the symbolic reference we want to read with refs_read_symbolic_ref is actually not a symbolic reference, the files and the reftable backends return different values (1 and -1 respectively). Standardize the returned values so that 0 is success, -1 is a generic error and -2 is that the reference was actually non-symbolic. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> --- Notes: v13: new patch v14: - simplified the logic in reftables backend (thanks Junio) - update the comment in refs.h (thanks Patrick) - rewrote comment in refs-internal.h to point to the one in refs.h - created NOT_A_SYMREF=-2 constant refs.h | 11 +++++++++++ refs/files-backend.c | 7 +++---- refs/refs-internal.h | 5 +++++ refs/reftable-backend.c | 8 +++++--- 4 files changed, 24 insertions(+), 7 deletions(-)