From: shejialuo Date: Sun, 22 Sep 2024 15:53:14 GMT Subject: Re: [PATCH v4 4/5] ref: add symref content check for files backend Message-ID: In-Reply-To: On Wed, Sep 18, 2024 at 01:19:13PM -0700, Junio C Hamano wrote: > shejialuo writes: > > Expect that people do not read the body of the message as completing > a paragrpah the title started. I.e. ... > > > We have already introduced the checks for regular refs. There is no need > > to check the consistency of the target which the symref points to. > > Instead, we just need to check the content of the symref itself. > > ... this needs a bit of preamble, like > > We have code that check regular ref contents, but we do not yet > check contents of symbolic refs. > Thanks, I will improve this in the next version. > > A regular file is accepted as a textual symref if it begins with > > "ref:", followed by zero or more whitespaces, followed by the full > > refname, followed only by whitespace characters. We always write > > a single SP after "ref:" and a single LF after the refname, but > > third-party reimplementations of Git may have taken advantage of the > > looser syntax. Put it more specific, we accept the following contents > > of the symref: > > > > 1. "ref: refs/heads/master " > > 2. "ref: refs/heads/master \n \n" > > 3. "ref: refs/heads/master\n\n" > > > > Thus, we could reuse "refMissingNewline" and "trailingRefContent" > > FSCK_INFOs to do the same retroactive tightening as we introduce for > > regular references. > > > > But we do not allow any other trailing garbage. The followings are bad > > symref contents which will be reported as fsck error by "git-fsck(1)". > > This description needs to be updated, as it is unclear if you are > talking about errors we already detect, or if you are planning to > update fsck to notice and report these errors. > Yes, When I was writing this part, I felt a little painful to express my words. I have thought how could I express the connection between the current patch and the previous one. > > And we will remember the untrimmed length of the "referent" and call > > "strbuf_rtrim()" on "referent". Then, we will call "check_refname_format" > > to check whether the trimmed referent format is valid. If not, we will > > report to the user that the symref points to referent which has invalid > > format. If it is valid, we will compare the untrimmed length and trimmed > > length, if they are not the same, we need to warn the user there is some > > trailing garbage in the symref content. > > That is an implementation detail of what you did. But if the > implementation were buggy and did not exactly what you intended to > do, the above description gives no information to help others to fix > it up so that it works as you intended it to work, because you do > not explain it. > > So what did you want to achieve in the third step (the first being > "limit to refs/ hiararchy", the second being "no incomplete lines > allowed")? > > Third, we want to make sure that the contents of a textual > symref MUST have a single LF after the target refname and > NOTHING ELSE. > > or something. > From the above comments, I need to organize the commit message of this patch to make things clear here. > "a directory" -> "an existing directory"? > > I am not comfortable to see the word "directory" used in this > proposed log message, as some refs could be stored in the packed > backend and are referenced by the symbolic ref you are inspecting > (this comment also refers to the "refs/ directory" you mentioned > earlier as "the first check"). > > Lastly, a symbolic ref MUST either point to an existing ref, > or if the referent does not exist, it MUST NOT be a leading > subpath for another existing ref (e.g., when "refs/heads/main" > exists, a symbolic ref that points at "refs/heads" is a no-no). > > or something (but again, I am open to a phrasing better than > "subpath"). > > Design question. What do we want to do when we have no loose refs > under the "refs/heads/historical/" hiearchy, (i.e. all of them are > in packed-refs file) hence ".git/refs/heads/historical" directory > does not exist on the filesystem. And a symbolic ref points at > "refs/heads/historical". Shouldn't we give the same error whether > the .git/refs/heads/historical directory exist or not, as long as > the refs/heads/historical/main branch exists (in the packed-refs > backend)? > I guess I need to think carefully here. Actually, my intention is that I want to concentrate on the loose refs and then take consideration about the packed refs. However, from what you have said above, it seems I could not do this. They are connected. But at current, I am not so familiar with packed refs behavior, I could not answer all the questions above. I decide to understand what packed-ref done. So, this series may be stalled sometime until I have a good knowledge and re-think the design here. > > +`escapeReferent`:: > > + (ERROR) The referent of a symref is outside the "ref" directory. > > I am not sure starting this as ERROR is wise. Users and third-party > tools make creative uses of the system and I cannot offhand think of > an argument why it should be forbidden to create a symbolic link to > our own HEAD or to some worktree-specific ref in another worktree. > Do we allow this cross-access (hack)? It might cause some trouble from my perspective. > > + if (referent->buf[referent->len - 1] != '\n') { > > As you initialized "len" to "referent->len-1" earlier, wouldn't it > more natural to use it here? That would match the incrementing of > len++ later in this block. > Yes, exactly. > > + ret = fsck_report_ref(o, report, > > + FSCK_MSG_REF_MISSING_NEWLINE, > > + "missing newline"); > > + len++; > > + } > > Having said that, the above should be simplified more like: > > * declare but not initialize "len". better yet, declare "orig_len" > and leave it uninitialized. > > * do not touch "len++" in the above block (actually, you can > discard the above "if(it does not end with LF)" block, see > below). > > * instead grab "referent->len" in "len" (or "orig_len") immediately > before you first modify referent, i.e. before strbuf_rtrim() call. > > orig_len = referent->len; > orig_last_byte = referent->buf[orig_len - 1]; > I agree. > > + strbuf_rtrim(referent); > > + if (check_refname_format(referent->buf, 0)) { > > + ret = fsck_report_ref(o, report, > > + FSCK_MSG_BAD_REFERENT_NAME, > > + "points to refname with invalid format"); > > Similar to an earlier step, the message does not give any more > information than the enum. Wouldn't the user who got this error > want to learn what referent->buf said and which part of it was bad > in the same message, instead of having to look it up on their own > after fsck finishes? > Yes, I agree. I will improve this. > > + goto out; > > + } > > At this point we know check_refname_format() is happy with what is > left after rtrimming the referent. There are four cases: > > - rtrim() did not trim anything (orig_len == referent->len); the file > lacked the terminating LF. > > - rtrim() trimmed one byte (orig_len - 1 == referent->len) and > the byte was not LF (orig_last_byte != '\n'). The file lacked > the terminating LF. > > - rtrim() trimmed exactly one byte (orig_len - 1 == referent->len) > and the byte was LF (orig_last_byte == '\n'). There is no error. > > - all other cases, i.e., rtrim() trimmed two or more bytes. The > file had trailing whitespaces after a valid referent that passed > check_refname_format(). > That's so clear. My implementation is not good compared with this. > So in short, > > if (referent->len == orig_len || > referent->len == orig_len - 1 && orig_last_byte != '\n') { > FSCK_MSG_REF_MISSING_NEWLINE; > } else if (referent->len < orig_len - 1) { > FSCK_MSG_REF_TRAILING_WHITESPACE; > } > > can replace the next block you wrote, and we can also remove the > earlier "it is an error if it does not end with '\n'", I think. > > > + if (len != referent->len) { > > + ret = fsck_report_ref(o, report, > > + FSCK_MSG_TRAILING_REF_CONTENT, > > + "trailing garbage in ref"); > > As check_refname_format() was happy, the difference between orig_len > and referent->len are only coming from trailing whitespaces, i.e. it > is not that it had arbitrary garbage. Shouldn't we be more explicit > about that? > Yes, I made a lot of mistakes when calling the "fsck_report_ref". I will report the exact garbage content to the user. > > + /* > > + * Dangling symrefs are common and so we don't report them. > > + */ > > + if (lstat(referent_path->buf, &st)) { > > + if (errno != ENOENT) { > > + ret = error_errno(_("unable to stat '%s'"), > > + referent_path->buf); > > + } > > + goto out; > > + } > > + > > + /* > > + * We cannot distinguish whether "refs/heads/a" is a directory or not by > > + * using "check_refname_format(referent->buf, 0)". Instead, we need to > > + * check the file type of the target. > > + */ > > + if (S_ISDIR(st.st_mode)) { > > + ret = fsck_report_ref(o, report, > > + FSCK_MSG_BAD_REFERENT_FILETYPE, > > + "points to the directory"); > > + goto out; > > + } > > If referent_path->buf refers to "refs/heads/historical/", and all > the branches under the hierarchy have been sent to packed-refs, > then this check will not trigger. > Yes, because "refs/heads/historical" will not appear in the filesystem. > I wonder if this check is the right thing to enforce in the first > place, though. > > As far as the end user is concerned, refs/heads/historical/master > branch stil exists, and there is no refs/heads/historical branch, so > such a symbolic ref, for all intents and purposes, is the same as > any other dangling symbolic refs, no? > > Of course, "git update-ref SUCH_A_SYMREF HEAD" will complain because > there is refs/heads/historical, with something like > > "refs/heads/historical/master" exists, cannot create "refs/heads/historical" > > but that is to be expected. If you remove the last branch in the > refs/heads/historical hierarchy, you should be able to do such an > update-ref to instanciate refs/heads/historical as a regular ref. > I am a little shocked here. I do this in action and find the directory will be automatically converted to a regular file in the filesystem. So, I agree with you here. We should never check this, because we allow symref to point to a directory. As long as there is no loose refs and packed refs under this directory, we could use "git update-ref" for this symref. Thanks, > > @@ -3484,12 +3553,24 @@ static int files_fsck_refs_content(struct ref_store *ref_store, > > "trailing garbage in ref"); > > goto cleanup; > > } > > + } else { > > + strbuf_addf(&referent_path, "%s/%s", > > + ref_store->gitdir, referent.buf); > > + /* > > + * the referent may contain the spaces and the newline, need to > > + * trim for path. > > + */ > > + strbuf_rtrim(&referent_path); > > I doubt this is a good design. We have referent, and the symbolic > ref checker knows that the true referent refname may be followed by > whitespaces, so instead of inventing referent _path here, it would > be a better design to let the files_fsck_symref_target() to decide > what file to open and check based on referent, no? Give it the > refstore or refstore's gitdir and have the concatenation with the > rtrimmed contents in the referent->buf after it inspected it > instead, perhaps? > Yes, I agree with you here. We should use "files_fsck_symref_target" to do this. ---- From this review, I think I need to understand more behaviors about files backend and packed backend. Thanks for your so dedicated reviews. I may spend more time to send the next version. And there may be some delay. Thanks, Jialuo