Re: [PATCH 1/2] branch: force-copy a branch to itself via @{-1} is a no-op

From: Rubén Justo <rjusto@gmail.com>
Date: Nov 20, 2022, 09:34 UTC
Message-ID: <b70d55e8-efb6-a69e-f568-e0d6aef48e61@gmail.com>
In-Reply-To: <221118.868rk8hpk4.gmgdl@evledraar.gmail.com>

On 18-nov-2022 04:58:54, Ævar Arnfjörð Bjarmason wrote:

Show 15 quoted lines

> > @@ -584,13 +584,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
> >  	strbuf_release(&logmsg);
> >  
> >  	strbuf_addf(&oldsection, "branch.%s", interpreted_oldname);
> > -	strbuf_release(&oldref);
> >  	strbuf_addf(&newsection, "branch.%s", interpreted_newname);
> > -	strbuf_release(&newref);
> >  	if (!copy && git_config_rename_section(oldsection.buf, newsection.buf) < 0)
> >  		die(_("Branch is renamed, but update of config-file failed"));
> > -	if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> > +	if (copy && strcmp(interpreted_oldname, interpreted_newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> 
> We try to stay under 79 chars, see CodingGuidelines. The pre-image was
> already violating this, but the new one is really long. I think it would
> be good to just wrap this after the last && while at it.

Yeah, thought about that. I preferred to not doing it mainly because my plan is to move out that strcmp() from there, but also wrapping that line induces to wrapping the previous one too, and there are many lines in that file above 79... I already have a series[1] to follow the CodingGuideLines in branch.c, currently focused in error messages but, maybe this change makes more sense there. Dunno.

Show 13 quoted lines

> 
> >  		die(_("Branch is copied, but update of config-file failed"));
> > +	strbuf_release(&oldref);
> > +	strbuf_release(&newref);
> >  	strbuf_release(&oldsection);
> >  	strbuf_release(&newsection);
> 
> This moving around of destructors isn't needed, and is just some
> unrelated cleanup. Your change here only needs to be:
>
> 	-       if (copy && strcmp(oldname, newname) && git_config_copy_section(oldsection.buf, newsection.buf) < 0)
> 	+       if (copy && strcmp(interpreted_oldname, interpreted_newname) &&
> 	+           git_config_copy_section(oldsection.buf, newsection.buf) < 0)

'interpreted_oldname' is a pointer to the 'oldref' buffer, and it is used in the next comparison, so the release for 'oldref' needs to be done later (same for interpreted_newname and newref).

Maybe you are thinking in another change... I also thought comparing '{old,new}section.buf, the section names in the configuration, but I preferred to use interpreted_{old,new}name. It looks more clear what we are doing and in future commits that section names might not be composed at that point yet.