[pacman-dev] [PATCH 2/2] Move logging of sysupgrade start, add log when done

[Allan McRae]

On 17/12/12 00:25, jjacky wrote:
> 
> 
> On 12/16/12 15:05, Allan McRae wrote:
>> On 13/12/12 23:19, Olivier Brunel wrote:
>>> The "starting sysupgrade" message was logged quite soon, making it be added
>>> even when nothing was actually done, because there was nothing to do, the user
>>> didn't confirm, or asked to only download packages.
>>>
>>> The log message is now added only when (before) committing the transaction. And
>>> we also log a message at the end (in case of success or failure).
>>>
>>> Signed-off-by: Olivier Brunel <i.am.jack.mail at gmail.com>
>>> ---
>>>  src/pacman/sync.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/pacman/sync.c b/src/pacman/sync.c
>>> index f8fce7f..77fd573 100644
>>> --- a/src/pacman/sync.c
>>> +++ b/src/pacman/sync.c
>>> @@ -793,7 +793,6 @@ static int sync_trans(alpm_list_t *targets)
>>>  
>>>  	if(config->op_s_upgrade) {
>>>  		printf(_(":: Starting full system upgrade...\n"));
>>> -		alpm_logaction(config->handle, "starting full system upgrade\n");
>>
>> I really do not like that moving this prints effectively the same
>> message at two different times.  Also, the line below really is the
>> start of the system upgrade.
> 
> By two different times, you mean to the user and in the log? In such a
> case, this first message could be something else, e.g. "preparing full
> system upgrade" or something to that effect, and moving the "starting"
> along with the log one.
> 
> What I don't like with the current situation, is that the message (in
> the log) says "starting sysupgrade" while it might not really be the
> case, because of a conflict, user didn't confirmed, or there was nothing
> to do.
> That's why I wanted the message to do logged before the operation was
> confirmed, and the sysupgrade will effectively be attempted/started.

This is where I completely disagree.  I see the upgrade operation
starting when pacman checks what is currently installed and whether
there is a new version of it.

If there is a conflict in the packages, what failed?  The system
upgrade...  so the system upgrade must have started...

>>
>>>  		if(alpm_sync_sysupgrade(config->handle, config->op_s_upgrade >= 2) == -1) {
>>>  			pm_printf(ALPM_LOG_ERROR, "%s\n", alpm_strerror(alpm_errno(config->handle)));
>>>  			trans_release();
>>> @@ -879,6 +878,9 @@ int sync_prepare_execute(void)
>>>  		goto cleanup;
>>>  	}
>>>  
>>> +	if(config->op_s_upgrade) {
>>> +		alpm_logaction(config->handle, "starting full system upgrade\n");
>>> +	}
>>>  	if(alpm_trans_commit(config->handle, &data) == -1) {
>>>  		alpm_errno_t err = alpm_errno(config->handle);
>>>  		pm_printf(ALPM_LOG_ERROR, _("failed to commit transaction (%s)\n"),
>>> @@ -911,11 +913,16 @@ int sync_prepare_execute(void)
>>>  			default:
>>>  				break;
>>>  		}
>>> +		alpm_logaction(config->handle, "failed to commit transaction (%s)\n",
>>> +				alpm_strerror(err));
>>
>> Why only this error?  There are other just as valid error conditions to log.
> 
> I wanted to log something after the "start" message, to either say it
> was successful, or failed. Other errors occur before the start message
> is logged, that's why they're not logged.
> 
>>
>>>  		/* TODO: stderr? */
>>>  		printf(_("Errors occurred, no packages were upgraded.\n"));
>>>  		retval = 1;
>>>  		goto cleanup;
>>>  	}
>>> +	alpm_logaction(config->handle, (config->op_s_upgrade)
>>> +			? "full system upgrade completed\n"
>>> +			: "installation completed\n");
> 
> Just realized, that this probably should account for another case:
> "download completed"
> 
>>>  
>>>  	/* Step 4: release transaction resources */
>>>  cleanup:
>>>
>>
>>
> 
> 
>