threads / patch / 7545

patchrename_ref(): only print a warning when config-file update fails

Subject: [PATCH] rename_ref(): only print a warning when config-file update fails

## tl;dr

5 messages between Apr 6, 2007 and Apr 7, 2007. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Lars Hjemli· Apr 6, 2007, 08:33 UTC · lore

If git_config_rename_section() fails, rename_ref() used to return 1, which left HEAD pointing to an absent refs/heads file (since the actual renaming had already occurred).

Signed-off-by: Lars Hjemli <hjemli@gmail.com>
---
On 4/5/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 15 quoted lines
> Hi,
> 
> On Thu, 5 Apr 2007, Geert Bosch wrote:
> 
> > Make git_config_rename_section return success if no config file
> > exists.
> 
> I don't think this is correct. git_config_rename_section() _should_ return
> an error.
> 
> > Otherwise, renaming a branch would abort, leaving the repository in an
> > inconsistent state.
> 
> This should take the hint from --rename-section, and print a warning (or
> not).

I think both arguments makes sense. There really is no reason to abort the rename operation if the config file update fails (for any reason).

 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to refs.c +1 −1
diff --git a/refs.c b/refs.c
index f471152..2ac6384 100644
--- a/refs.c
+++ b/refs.c
@@ -835,7 +835,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		snprintf(oldsection, 1024, "branch.%s", oldref + 11);
 		snprintf(newsection, 1024, "branch.%s", newref + 11);
 		if (git_config_rename_section(oldsection, newsection) < 0)
-			return 1;
+			error("unable to update config-file");
 	}
 
 	return 0;

-- 
1.5.1.53.g77e6f
Geert Bosch· Apr 6, 2007, 10:35 UTC · re: Lars Hjemli · lore

Re: [PATCH] rename_ref(): only print a warning when config-file update fails

On Apr 6, 2007, at 04:33, Lars Hjemli wrote:
Show 7 quoted lines
> If git_config_rename_section() fails, rename_ref() used to return  
> 1, which
> left HEAD pointing to an absent refs/heads file (since the actual  
> renaming
> had already occurred).
>
> Signed-off-by: Lars Hjemli <hjemli@gmail.com>

This makes sense in addition to my patch, if the renaming fails for any of the other possible reasons.

   -Geert
Junio C Hamano· Apr 6, 2007, 20:35 UTC · re: Lars Hjemli · lore

Re: [PATCH] rename_ref(): only print a warning when config-file update fails

Lars Hjemli <hjemli@gmail.com> writes:
> If git_config_rename_section() fails, rename_ref() used to return 1, which
> left HEAD pointing to an absent refs/heads file (since the actual renaming
> had already occurred).

I wonder if rolling back the rename that was asked is an option. We would want to keep these low-level things atomic whenever possible.

Lars Hjemli· Apr 6, 2007, 23:53 UTC · re: Junio C Hamano · lore

Re: [PATCH] rename_ref(): only print a warning when config-file update fails

On 4/6/07, Junio C Hamano <junkio@cox.net> wrote:
Show 9 quoted lines
> Lars Hjemli <hjemli@gmail.com> writes:
>
> > If git_config_rename_section() fails, rename_ref() used to return 1, which
> > left HEAD pointing to an absent refs/heads file (since the actual renaming
> > had already occurred).
>
> I wonder if rolling back the rename that was asked is an
> option.  We would want to keep these low-level things atomic
> whenever possible.

I was wondering the same thing, i.e. "goto rollback" as an option for "error()". But I ended up thinking that rename_ref() shouldn't bother with the config file at all (thus my other patch).

-- 
larsh
Junio C Hamano· Apr 7, 2007, 00:14 UTC · re: Lars Hjemli · lore

Re: [PATCH] rename_ref(): only print a warning when config-file update fails

"Lars Hjemli" <hjemli@gmail.com> writes:
Show 7 quoted lines
>> I wonder if rolling back the rename that was asked is an
>> option.  We would want to keep these low-level things atomic
>> whenever possible.
>
> I was wondering the same thing, i.e. "goto rollback" as an option for
> "error()". But I ended up thinking that rename_ref() shouldn't bother
> with the config file at all (thus my other patch).
I agree that "other patch" is sensible regardless.

← back to recent threads