On Wed, May 29, 2024 at 10:25:47AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > > + char refspec_str[] = "refs/heads/*"; > > > > memset(&refspec, 0, sizeof(refspec)); > > refspec.force = 0; > > refspec.pattern = 1; > > - refspec.src = refspec.dst = "refs/heads/*"; > > + refspec.src = refspec.dst = refspec_str; > > get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0); > > matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"), > > fetch_map, 1); > > I have no objections to changes along this line, but this makes me > wonder if we somehow have ways to ensure that after everything is > done, refspec_str[], empty_str[], and the like in other hunks remain > to have their initial contents. Stepping back a bit, without > applying this series, if we told the compiler to store these > constant strings in read-only segment, any attempt to write into > refspec.src or refspec.dst string would have been caught at runtime > as an error. With the patch, that would no longer work. The piece > of memory in refspec_str[] is a fair game to be overwritten by > anybody. True. Ideally, we wouldn't have to do these acrobatics here in the first place. That would likely require quite a lot more work and go beyond the scope of this patch series. > Which makes me wonder why these refspec.src and refspec.dst members > are not "const char *" pointers in the first place? Obviously we do > not expect "refs/heads/*" to be overwritten after storing the pointer > to it in these members and making the get_fetch_map() call. Well, we do. Not in `get_fetch_map()`, but in `query_refspecs()`. It does weird stuff where it writes the result into either `src` or `dst` depending on which of these fields is provided by the caller. Which means that one of the fields would typically be a constant, whereas the other one will be allocated. Really, once you start looking you find all kinds of weird interfaces where we cannot quite make up our mind whether fields are controlled by the caller or by the callee. This patch series certainly surfaces quite some places that left me scratching my head. Patrick
Attachment:
signature.asc
Description: PGP signature
