Re: [PATCH] remote: handle negative refspecs in git remote show
- From
Jacob Keller <jacob.keller@gmail.com>
- Date
- Jun 14, 2022, 06:09 UTC
- Message-ID
- <CA+P7+xos3vpPjb3m_BkR4Qp3OPf+R+-A1B=jTD3-Q6FeO4RpMg@mail.gmail.com>
- In-Reply-To
- <Yqfec9yvT3LKomNK@nand.local>
On Mon, Jun 13, 2022 at 6:03 PM Taylor Blau <me@ttaylorr.com> wrote:
Show 34 quoted lines
>
> On Mon, Jun 13, 2022 at 05:32:51PM -0700, Jacob Keller wrote:
> > Fix this by checking negative refspecs inside of get_ref_states. For
> > each ref which matches a negative refspec, copy it into a "skipped" list
> > and remove it from the fetch map. This allows us to show the following
> > output instead:
>
> Seems sensible.
>
> > + /* handle negative refspecs first */
> > + for (tail = &fetch_map; *tail; ) {
> > + ref = *tail;
> > +
> > + if (omit_name_by_refspec(ref->name, &states->remote->fetch)) {
> > + string_list_append(&states->skipped, abbrev_branch(ref->name));
> > +
> > + /* Matched a negative refspec, so remove this ref from
> > + * consideration for being a new or tracked ref.
> > + */
> > + *tail = ref->next;
> > + free(ref->peer_ref);
> > + free(ref);
> > + } else {
> > + tail = &ref->next;
> > + }
> > + }
> > +
>
> Not being overly familiar with the "git remote show" code, this
> implementation looks very reasonable to me. If we see a negative
> refspec, we remove it from the fetch_map list and append it to the
> skipped list. Otherwise, we increment our pointer, and continue along
> until we reach the end of the list.
>The specific way the loop works is similar to other ref looping code but it feels a little odd to me. Still, it seems to be the right approach overall.
Show 13 quoted lines
> > +test_expect_success 'show with negative refspecs' ' > > + test_when_finished "git -C test config --fixed-value --unset remote.origin.fetch ^refs/heads/main" && > > + ( > > + cd test && > > + git config --add remote.origin.fetch ^refs/heads/main && > > Doing "git config --unset" outside of the subshell could be avoided by > ditching the subshell altogether, perhaps with something like: > > test_config -C test remote.origin.fetch ^refs/heads/main && > git -C test remote show origin >actual && > test_cmp test/expect actual >
I still think that removing the subshell is a good idea here. I'll investigate this.
I also wonder if it would be difficult to enable "--add" semantics for test_config.
> Thanks, > Taylor