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

["Rubén Justo" <rjusto@gmail.com>]

On 18-nov-2022 04:58:54, Ævar Arnfjörð Bjarmason wrote:
> > @@ -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.

> 
> >  		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.